Thanks for the review! On Mon, Nov 25, 2024 at 7:39 AM Michael Paquier <mich...@paquier.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
v05-0002-Add-pipeline-support-in-psql.patch
Description: Binary data
v05-0001-Consider-pipeline-implicit-transaction-as-a-tran.patch
Description: Binary data
