Batch rewrite support for psql 15 Merge statement (#2579)

[mmimica]

See #2579.

Adds parser support for MERGE statements. Checks its specific to enable "batchReWrite" feature originally written for INSERT statements.

[vlsi]

Thanks for putting the fix together.

However, I wonder what would be the behavior
a) if the syntax does not match exactly. For instance merge into ... using (select * from values(?,?)). Would it still work?
b) What happens if the bind variable is used in when block. For instance when matched then insert (id, name) values (src.id, ?). Would it still replace?

Would you please update the documentation to list the supported and unsupported syntaxes? It would be nice to have them tested as well (I mean the tests should use end-to-end MERGE + batch API).

[mmimica]

Hi.

I believe the correct answers are
a) yes
b) no

I've pushed a couple of more test-cases that confirm it.

I admit the solution is a bit hackish, but so is the whole feature.

[davecramer]

@vlsi can we merge this ?

[davecramer]

Is it possible to test when MERGE won't work ?

[vlsi]

We should definitely test and document cases when rewrite is not activated

[vlsi]

For reference, this should be assume so the test is displayed as skipped rather than successful.

[vlsi]

I'm inclined to add pgjdbc-jfrunit subproject with https://github.com/moditect/jfrunit tests.
That would be executed with Java 17+ only, however, it would enable us to assert on the number of network roundtrips, so we could properly assert that batching works as expected.

[davecramer]

Not sure the effort justifies, but I'll leave that up to you.

[sehrope]

-1 to adding more dependencies for something like that.

If you want to test if the batching is done in a single command you could use defaults on the table to infer how many actual commands were used to populate the data. That'd work on across all JDK and PG versions too:

CREATE TABLE t (
 a int PRIMARY KEY,
 c_timestamp timestamptz DEFAULT clock_timestamp(), -- instatanenous wall time
 s_timestamp timestamptz DEFAULT statement_timestamp(), -- statement start time
 tx_timestamp timestamptz DEFAULT transaction_timestamp() -- transactionst start time
);

BEGIN;
INSERT INTO t (a) VALUES (1);
INSERT INTO t (a) VALUES (2), (3);
INSERT INTO t (a) SELECT generate_series(4, 100);
COMMIT;

SELECT
  COUNT(DISTINCT c_timestamp),
  COUNT(DISTINCT s_timestamp),
  COUNT(DISTINCT tx_timestamp)
FROM t;

This results in:

 count | count | count 
-------+-------+-------
   100 |     3 |     1

The only thing that we care about is the number of statements which would be the number of distinct statement timestamps. While it's theoretically possible for back to back statements to have the same timestamp, that's not going to happen in practice as we're talking about microsecond resolution. That's plenty for something that's just part of a test suite.

[vlsi]

Is there a query to measure the number of network roundtrips and bytes sent/received?

[sehrope]

I'm not aware of any functions for network traffic exposed by the server. There's also a bit of Heisenberg principle in asking the server for that information.

You could use a custom socket factory that tracks that kind of information. At least of the total bytes. Round trips are tricky as you wouldn't really know whether it's being buffered.

In any case, I can't imagine it'd be cleaner than the statement timestamp approach. I suggest going that route if you want to verify the number of individual server statements actually executed.

[mmimica]

There is also SELECT xact_commit FROM pg_stat_database. It's global to the database and affected by background processes, but it should glow at a lower rate with bulk operations.

[vlsi]

@mmimica do you think you could add tests that would cover the following?
a) ensure the rewrite did happen (e.g. via statement_timestamp)
b) ensure the rewrite does not happen for unsupported cases, and they still work

[vlsi]

14 will be a part of "expected: ... got:..." message anyway.
Please clarify in the message parameter the reasons why you expect certain things to happen.

[vlsi]

Suppose the test fails with "Number of inserted rows. expected: 14 got: 15".
Does it look like a good bug report to you?

Frankly speaking, I would not say that message clarifies the reasons why 14 was expected.
It would be much better if you put a message that clarifies why you expect certain things.

As an alternative, it might work even better if you select full table contents, and verify it exactly. Then the failure message would shed some light on which rows were not expected, and so on.

[vlsi]

I've rebased the PR, however, I have not reviewed the code and tests yet.