public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: [email protected]
Cc: Peter Eisentraut <[email protected]>
Cc: Victor Yegorov <[email protected]>
Cc: Pierre Forstmann <[email protected]>
Subject: Re: Unexpected results from CALL and AUTOCOMMIT=off
Date: Mon, 03 Jun 2024 21:32:28 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CAGnEboiRe+fG2QxuBO2390F7P8e2MQ6UyBjZSL_w1Cej+E4=Vw@mail.gmail.com>
<CAM-sOH80Z=OqYBWgYP=BDGLbxQ72wkC9=tZ-vRKxkmKkhTD7MQ@mail.gmail.com>
<CAGnEbojf5Awm862ghvooLku6mm0m4yF60PyHQxCinM8pn01sbw@mail.gmail.com>
<[email protected]>
[ redirecting to pgsql-hackers ]
I wrote:
> I agree that this looks like a bug, since your example shows that the
> same function works as-expected in an ordinary expression but not in
> a CALL. The dependency on AUTOCOMMIT (that is, being within an outer
> transaction block) seems even odder. I've not dug into it yet, but
> I suppose we're passing the wrong snapshot to the CALL arguments.
I poked into this and found that the source of the problem is that
plpgsql's exec_stmt_call passes allow_nonatomic = true even when
it's running in an atomic context. So we can fix it with basically
a one-line change:
- options.allow_nonatomic = true;
+ options.allow_nonatomic = !estate->atomic;
I'm worried about whether external callers might've made a comparable
mistake, but I think all we can do is document it a little better.
AFAICS there isn't any good way for spi.c to realize that this mistake
has been made, else we could have it patch up the mistake centrally.
I've not attempted to make those doc updates in the attached draft
patch though, nor have I added a test case yet.
Before realizing that this was the issue, I spent a fair amount of
time on the idea that _SPI_execute_plan() was doing things wrong,
and that led me to notice that its comment about having four modes
of snapshot operation has been falsified in multiple ways. So this
draft does include fixes for that comment.
Thoughts?
regards, tom lane
Attachments:
[text/x-diff] fix-call-in-atomic-context-draft.patch (2.3K, 2-fix-call-in-atomic-context-draft.patch)
download | inline diff:
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index a97a7e3bd4..c93b6c192f 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2425,12 +2425,17 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
* snapshot != InvalidSnapshot, read_only = false: use the given snapshot,
* modified by advancing its command ID before each querytree.
*
- * snapshot == InvalidSnapshot, read_only = true: use the entry-time
- * ActiveSnapshot, if any (if there isn't one, we run with no snapshot).
+ * snapshot == InvalidSnapshot, read_only = true: do nothing for queries
+ * that require no snapshot. For those that do, ensure that a Portal
+ * snapshot exists; then use that, or use the entry-time ActiveSnapshot if
+ * that exists and is different.
*
- * snapshot == InvalidSnapshot, read_only = false: take a full new
- * snapshot for each user command, and advance its command ID before each
- * querytree within the command.
+ * snapshot == InvalidSnapshot, read_only = false: do nothing for queries
+ * that require no snapshot. For those that do, ensure that a Portal
+ * snapshot exists; then, in atomic execution (!allow_nonatomic) take a
+ * full new snapshot for each user command, and advance its command ID
+ * before each querytree within the command. In allow_nonatomic mode we
+ * just use the Portal snapshot unmodified.
*
* In the first two cases, we can just push the snap onto the stack once
* for the whole plan list.
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 6947575b94..54ede9d85e 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2218,12 +2218,12 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
* for the plan. This avoids refcount leakage complaints if the called
* procedure ends the current transaction.
*
- * Also, tell SPI to allow non-atomic execution.
+ * Also, tell SPI to allow non-atomic execution if appropriate.
*/
memset(&options, 0, sizeof(options));
options.params = paramLI;
options.read_only = estate->readonly_func;
- options.allow_nonatomic = true;
+ options.allow_nonatomic = !estate->atomic;
options.owner = estate->procedure_resowner;
rc = SPI_execute_plan_extended(expr->plan, &options);
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Unexpected results from CALL and AUTOCOMMIT=off
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox