Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sEYty-0029v1-Vo for pgsql-hackers@arkaria.postgresql.org; Tue, 04 Jun 2024 18:28:56 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1sEYty-001nAd-UO for pgsql-hackers@arkaria.postgresql.org; Tue, 04 Jun 2024 18:28:54 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sEYty-001nAV-GT for pgsql-hackers@lists.postgresql.org; Tue, 04 Jun 2024 18:28:54 +0000 Received: from sss.pgh.pa.us ([68.162.161.243]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sEYtr-003RGf-KR for pgsql-hackers@lists.postgresql.org; Tue, 04 Jun 2024 18:28:53 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 454IShmT538152; Tue, 4 Jun 2024 14:28:43 -0400 From: Tom Lane To: pgsql-hackers@lists.postgresql.org cc: Peter Eisentraut , Victor Yegorov , Pierre Forstmann Subject: Re: Unexpected results from CALL and AUTOCOMMIT=off In-reply-to: <378291.1717464748@sss.pgh.pa.us> References: <90052.1717442890@sss.pgh.pa.us> <378291.1717464748@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Mon, 03 Jun 2024 21:32:28 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <537899.1717525675.0@sss.pgh.pa.us> Date: Tue, 04 Jun 2024 14:28:43 -0400 Message-ID: <538151.1717525723@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <537899.1717525675.1@sss.pgh.pa.us> I wrote: > 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. Actually, after poking around some more I found that there *is* a way to deal with this within spi.c: we can make _SPI_execute_plan ignore options->allow_nonatomic unless the SPI_OPT_NONATOMIC flag was given when connecting. I like this better than my first solution because (a) it seems to make the allow_nonatomic flag behave in a more intuitive way; (b) spi.c gates some other behaviors on SPI_OPT_NONATOMIC, so that gating this one too seems more consistent, and (c) this way, we fix not only plpgsql but anything that has copied its coding pattern. Hence, new patch attached, now with docs and tests. Barring objections I'll push this one. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-call-in-atomic-context-v1.patch"; charset="us-ascii" Content-ID: <537899.1717525675.2@sss.pgh.pa.us> Content-Description: fix-call-in-atomic-context-v1.patch Content-Transfer-Encoding: quoted-printable diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml index bb3778688b..7d154914b9 100644 --- a/doc/src/sgml/spi.sgml +++ b/doc/src/sgml/spi.sgml @@ -752,7 +752,9 @@ int SPI_execute_extended(const char *comman= d, true allows non-atomic execution of CALL and DO - statements + statements (but this field is ignored unless + the SPI_OPT_NONATOMIC flag was passed + to SPI_connect_ext) @@ -1893,7 +1895,9 @@ int SPI_execute_plan_extended(SPIPlanPtr = plan, true allows non-atomic execution of CALL and DO - statements + statements (but this field is ignored unless + the SPI_OPT_NONATOMIC flag was passed + to SPI_connect_ext) diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index a97a7e3bd4..e516c0a67c 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -2399,6 +2399,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteO= ptions *options, uint64 my_processed =3D 0; SPITupleTable *my_tuptable =3D NULL; int res =3D 0; + bool allow_nonatomic; bool pushed_active_snap =3D false; ResourceOwner plan_owner =3D options->owner; SPICallbackArg spicallbackarg; @@ -2406,6 +2407,12 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecute= Options *options, CachedPlan *cplan =3D NULL; ListCell *lc1; = + /* + * We allow nonatomic behavior only if options->allow_nonatomic is set + * *and* the SPI_OPT_NONATOMIC flag was given when connecting. + */ + allow_nonatomic =3D options->allow_nonatomic && !_SPI_current->atomic; + /* * Setup error traceback support for ereport() */ @@ -2425,12 +2432,17 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecut= eOptions *options, * snapshot !=3D InvalidSnapshot, read_only =3D false: use the given sna= pshot, * modified by advancing its command ID before each querytree. * - * snapshot =3D=3D InvalidSnapshot, read_only =3D true: use the entry-ti= me - * ActiveSnapshot, if any (if there isn't one, we run with no snapshot). + * snapshot =3D=3D InvalidSnapshot, read_only =3D true: do nothing for q= ueries + * 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 =3D=3D InvalidSnapshot, read_only =3D false: take a full new - * snapshot for each user command, and advance its command ID before eac= h - * querytree within the command. + * snapshot =3D=3D InvalidSnapshot, read_only =3D 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. @@ -2440,6 +2452,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteO= ptions *options, */ if (snapshot !=3D InvalidSnapshot) { + /* this intentionally tests the options field not the derived value */ Assert(!options->allow_nonatomic); if (options->read_only) { @@ -2585,7 +2598,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteO= ptions *options, * Skip it when doing non-atomic execution, though (we rely * entirely on the Portal snapshot in that case). */ - if (!options->read_only && !options->allow_nonatomic) + if (!options->read_only && !allow_nonatomic) { if (pushed_active_snap) PopActiveSnapshot(); @@ -2685,14 +2698,13 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecut= eOptions *options, QueryCompletion qc; = /* - * If the SPI context is atomic, or we were not told to allow - * nonatomic operations, tell ProcessUtility this is an atomic - * execution context. + * If we're not allowing nonatomic operations, tell + * ProcessUtility this is an atomic execution context. */ - if (_SPI_current->atomic || !options->allow_nonatomic) - context =3D PROCESS_UTILITY_QUERY; - else + if (allow_nonatomic) context =3D PROCESS_UTILITY_QUERY_NONATOMIC; + else + context =3D PROCESS_UTILITY_QUERY; = InitializeQueryCompletion(&qc); ProcessUtility(stmt, diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql= /src/expected/plpgsql_call.out index 17235fca91..0a63b1d44e 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_call.out +++ b/src/pl/plpgsql/src/expected/plpgsql_call.out @@ -564,3 +564,53 @@ NOTICE: inner_p(44) = (1 row) = +-- Check that stable functions in CALL see the correct snapshot +CREATE TABLE t_test (x int); +INSERT INTO t_test VALUES (0); +CREATE FUNCTION f_get_x () RETURNS int +AS $$ +DECLARE l_result int; +BEGIN + SELECT x INTO l_result FROM t_test; + RETURN l_result; +END +$$ LANGUAGE plpgsql STABLE; +CREATE PROCEDURE f_print_x (x int) +AS $$ +BEGIN + RAISE NOTICE 'f_print_x(%)', x; +END +$$ LANGUAGE plpgsql; +-- test in non-atomic context +DO $$ +BEGIN + UPDATE t_test SET x =3D x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + UPDATE t_test SET x =3D x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + ROLLBACK; +END +$$; +NOTICE: f_get_x(1) +NOTICE: f_print_x(1) +NOTICE: f_get_x(2) +NOTICE: f_print_x(2) +-- test in atomic context +BEGIN; +DO $$ +BEGIN + UPDATE t_test SET x =3D x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + UPDATE t_test SET x =3D x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); +END +$$; +NOTICE: f_get_x(1) +NOTICE: f_print_x(1) +NOTICE: f_get_x(2) +NOTICE: f_print_x(2) +ROLLBACK; diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/= sql/plpgsql_call.sql index 869d021a07..4cbda0382e 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_call.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql @@ -522,3 +522,53 @@ CREATE FUNCTION f(int) RETURNS int AS $$ SELECT $1 + = 2 $$ LANGUAGE sql; = CALL outer_p(42); SELECT outer_f(42); + +-- Check that stable functions in CALL see the correct snapshot + +CREATE TABLE t_test (x int); +INSERT INTO t_test VALUES (0); + +CREATE FUNCTION f_get_x () RETURNS int +AS $$ +DECLARE l_result int; +BEGIN + SELECT x INTO l_result FROM t_test; + RETURN l_result; +END +$$ LANGUAGE plpgsql STABLE; + +CREATE PROCEDURE f_print_x (x int) +AS $$ +BEGIN + RAISE NOTICE 'f_print_x(%)', x; +END +$$ LANGUAGE plpgsql; + +-- test in non-atomic context +DO $$ +BEGIN + UPDATE t_test SET x =3D x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + UPDATE t_test SET x =3D x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + ROLLBACK; +END +$$; + +-- test in atomic context +BEGIN; + +DO $$ +BEGIN + UPDATE t_test SET x =3D x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + UPDATE t_test SET x =3D x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); +END +$$; + +ROLLBACK; ------- =_aaaaaaaaaa0--