Re: Consider pipeline implicit transaction as a transaction block

Thanks for the review!

On Mon, Nov 25, 2024 at 7:39 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> This breaks an existing property of psql. One example: \edit where we
> should keep the existing query buffer rather than discard it if a
> failure happens. This patch forcibly removes the contents of the
> query buffer, and a failure could happen because of an incorrect
> option given to this meta-command.

Good point, I've removed the change.

> This relies on 0001 for the case where \gx fails and you want to make
> sure that the follow-up command is able to work in the same pipeline.
> I'd suggest to add a \reset after the \gx in this test sequence,
> without 0001. At this point we have IMO more advantages in
> maintaining the existing properties rather than enforce it, especially
> as we have \reset.

Definitely, I've updated the test to use \reset.

> Why is stuff specific to psql being mentioned on the libpq page? That
> does not seem adapted to me here. Documenting the expectations and
> the assumptions one would rely on in the context of a pipeline and
> what the backend thinks of it as an internal transaction is a good
> idea.

I've realised the behavior was already documented in the protocol-flow
doc[1] with an existing mention on the difference between the first
and following commands of a pipeline.

> It is sad to not have at least 2~3 tests that we could use for the
> back-branches for the ERROR cases and not the first command cases with
> minimal sequences in pgbench? I'm OK with your proposal with psql on
> HEAD, but backpatching without a few tests is going to make the
> maintenance of stable branches more complicated in the long run as we
> would navigate blindly. So I'd try to fix the problems first, then
> work on any new proposal once we are in a clean operational state.
> And I'd recommend to propose the new feature on a new, separate,
> thread to attract a better audience for the reviews this would
> require.

I've definitely sidetracked myself with the pipeline support in psql.
I've reworked and reordered the patch set to focus on the issue and
backporting.

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.

0002: Pipeline support in psql. I'm still attaching the patch here as
it makes testing pipelining behavior much easier but this is more as a
reference. I will create a dedicated thread and commitfest entry for
it.

[1]: https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-FLOW-PIPELINING