On Mon, Nov 25, 2024 at 02:35:25PM +0100, Anthonin Bonnefoy wrote: > 0001: Use implicit transaction block for the implicit pipeline > transaction. I've added tests in pgbench that covered the same checks > I did in psql. I've avoided using \syncpipeline since it was > introduced in 17. I've also slightly modified the protocol-flow > pipelining doc to provide more details on the implicit transaction > block and how the first message after a sync can be different. I've > added vacuum as first and second command of a pipeline in the tests > since it was mentioned in the doc.
That looks acceptable and backpatchable to me. It is a bit sad that pgbench does not capture the backend errors so as the output of the tests is more predictable, but this is not new and I'm OK to expand that with your proposal based on psql in v5-0002 with these new meta-commands. The patch provides positive and negative tests for REINDEX, VACUUM, LOCK and SET LOCAL, which should be plenty enough. There is also a negative check for subtransactions, which should never happen in implicit transaction blocks. This last one is a must-have, thanks. You are right to not use \syncpipeline for the base fix, still I am tempted to add one extra test with a sequence like this one for only one of the commands, just to check that the implicit transaction state is enforced after a sync. Say this one with a WARNING check in the output: \startpipeline SELECT 1; \syncpipeline SET LOCAL foo = bar; \endpipeline Tweaks of the tests across multiple stable branches happen all the time, and adding one specific to 17~ is no big issue. I'm in the middle of it but I'm lacking the steam to do so today. Will likely finish tomorrow. -- Michael
signature.asc
Description: PGP signature
