Thread: Re: Consider pipeline implicit transaction as a transaction block
On Wed, 30 Oct 2024 at 10:15, Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> wrote: > The attached patch adds the detection of implicit transactions started > by a pipeline in CheckTransactionBlock, avoiding warnings when > commands like `set local` are called within a pipeline, and making the > detection of transaction block coherent with what's done in > IsInTransactionBlock and PreventInTransactionBlock. +1 seems like a reasonable change. > The XACT_FLAGS_PIPELINING will only be set after the first command, so > the warning about `set local` happening outside of a transaction block > will still be generated. However, I'm not sure if it's something > fixable (or worth fixing?). This would require to know beforehand that > there are multiple executes before the sync message, which doesn't > seem doable. Yeah, I don't really see a way around that apart from not throwing this warning at all when the client is using the extended protocol. Postgres would need to be clairvoyant to know to really know if it should show it for the first message.
On Wed, Nov 27, 2024 at 1:42 AM Michael Paquier <michael@paquier.xyz> wrote: > I've edited the whole, added this extra test based on \syncpipeline in > 17~, kept the remaining tests in 14~ where pgbench is able to handle > them, and backpatched that down to 13. Let's see now what we can do > with the psql bits. Thanks! > Anthonin, now that the original problem is solved, could you create a > new thread with your new proposal for psql? That would attract a > better audience for reviews. Yes, I will rebase and start the dedicated thread for the pipeline support in psql.
On Wed, 27 Nov 2024 at 01:42, Michael Paquier <michael@paquier.xyz> wrote: > I've edited the whole, added this extra test based on \syncpipeline in > 17~, kept the remaining tests in 14~ where pgbench is able to handle > them, and backpatched that down to 13. Let's see now what we can do > with the psql bits. FYI: it turns out this change broke one of the tests on our pg_duckdb repo[1] because the error message that PreventInTranasctionBlock throws is now different: E AssertionError: Regex pattern did not match. E Regex: 'DuckDB queries cannot be executed within a pipeline' E Input: 'DuckDB queries cannot run inside a transaction block' I personally don't think that's particularly bad, or revert-worthy, but the previous error was a bit clearer IMO. I don't see how we can still show it with the new code though. [1]: https://github.com/duckdb/pg_duckdb/actions/runs/12052926038/job/33607381526?pr=453#step:15:51
On Tue, Nov 26, 2024 at 7:42 PM Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Nov 26, 2024 at 04:24:58PM +0900, Michael Paquier wrote: > > 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. > > I've edited the whole, added this extra test based on \syncpipeline in > 17~, kept the remaining tests in 14~ where pgbench is able to handle > them, and backpatched that down to 13. Let's see now what we can do > with the psql bits. I'm very surprised that this was back-patched. I think we should revert it from the back-branches before it gets into a minor release. It seems like a clear definitional change, which has no business in a minor release. -- Robert Haas EDB: http://www.enterprisedb.com
On 2024-11-27 15:41:14 -0500, Robert Haas wrote: > On Tue, Nov 26, 2024 at 7:42 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Nov 26, 2024 at 04:24:58PM +0900, Michael Paquier wrote: > > > 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. > > > > I've edited the whole, added this extra test based on \syncpipeline in > > 17~, kept the remaining tests in 14~ where pgbench is able to handle > > them, and backpatched that down to 13. Let's see now what we can do > > with the psql bits. > > I'm very surprised that this was back-patched. I think we should > revert it from the back-branches before it gets into a minor release. > It seems like a clear definitional change, which has no business in a > minor release. +1
Robert Haas <robertmhaas@gmail.com> writes:
> I'm very surprised that this was back-patched. I think we should
> revert it from the back-branches before it gets into a minor release.
> It seems like a clear definitional change, which has no business in a
> minor release.
I was troubled by that too. Maybe this can be painted as a bug fix
but it seems very questionable --- and even if it is, is it worth
the risk of unexpected side-effects? I'd rather see something that
touches wire-protocol behavior go through a normal beta test cycle.
regards, tom lane
On Wed, Nov 27, 2024 at 03:54:24PM -0500, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > I'm very surprised that this was back-patched. I think we should > > revert it from the back-branches before it gets into a minor release. > > It seems like a clear definitional change, which has no business in a > > minor release. > > I was troubled by that too. Maybe this can be painted as a bug fix > but it seems very questionable --- and even if it is, is it worth > the risk of unexpected side-effects? I'd rather see something that > touches wire-protocol behavior go through a normal beta test cycle. Yeah, I was surprised too, even though the author was clear they wanted to backpatch. I couldn't figure out why it was being backpatched, so I figured I was missing something. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com When a patient asks the doctor, "Am I going to die?", he means "Am I going to die soon?"
On Thu, Nov 28, 2024 at 12:26 AM Michael Paquier <michael@paquier.xyz> wrote: > I don't mind being more careful here based on your concerns, so I'll > go remove that in the stable branches. Sorry about that. I didn't have a strong need for this to be backpatched and should have made this clearer.
On Thu, Nov 28, 2024 at 2:53 AM Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> wrote: > On Thu, Nov 28, 2024 at 12:26 AM Michael Paquier <michael@paquier.xyz> wrote: > > I don't mind being more careful here based on your concerns, so I'll > > go remove that in the stable branches. > > Sorry about that. I didn't have a strong need for this to be > backpatched and should have made this clearer. FWIW, I don't think you did anything wrong. To me, the thread reads like you just submitted this as a normal patch and Michael decided to back-patch. -- Robert Haas EDB: http://www.enterprisedb.com