From: Tom Lane <tgl@sss.pgh.pa.us>
To: panso8@gmail.com
cc: Peter Eisentraut <peter@eisentraut.org>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18656: "STABLE" function sometimes does not see changes
In-reply-to: <18656-cade1780866ef66c@postgresql.org>
References: <18656-cade1780866ef66c@postgresql.org>
Comments: In-reply-to PG Bug reporting form <noreply@postgresql.org>
	message dated "Tue, 15 Oct 2024 09:58:35 -0000"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <4082731.1729014635.0@sss.pgh.pa.us>
Date: Tue, 15 Oct 2024 13:51:15 -0400
Message-ID: <4082805.1729014675@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/4082805.1729014675%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4082731.1729014635.1@sss.pgh.pa.us>

PG Bug reporting form <noreply@postgresql.org> writes:
> CREATE or replace PROCEDURE stbl_update_state()
>   LANGUAGE plpgsql AS
> $$
> declare
>   s text;
> begin
>   update stbl_states set state='on' where id=1;
>   s := stbl_get_state(1);
>   call stbl_log(s);
>   call stbl_log(stbl_get_state(1));
>   insert into stbl_logs(log) values (stbl_get_state(1));
> exception when others then
>   s := SQLERRM;
>   call stbl_log(s);
> end;
> $$;

> [ the second stbl_get_state call produces the wrong result ]

This seems closely related to 2dc1deaea, but that didn't fix it.
The key difference between this example and the tests added by
2dc1deaea is that the problematic CALL is inside an exception block.

After some study I propose that the correct fix is that
_SPI_execute_plan should act as though we're inside an atomic
context if IsSubTransaction().  We are inside an atomic context
so far as _SPI_commit and _SPI_rollback are concerned: they
will not allow you to COMMIT/ROLLBACK.  So it's not very clear
why _SPI_execute_plan should think differently.

The attached draft patch also makes SPI_inside_nonatomic_context
say we're in an atomic context if IsSubTransaction().  That's a
no-op so far as the one extant caller StartTransaction is concerned,
because we won't get there when inside a subtransaction.  But it
seems like all the places that consult _SPI_current->atomic ought
to be doing this the same way.

Thoughts?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="bug-18656-draft-fix.patch";
	charset="us-ascii"
Content-ID: <4082731.1729014635.2@sss.pgh.pa.us>
Content-Description: bug-18656-draft-fix.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 90d9834576..a268c04097 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -584,6 +584,8 @@ SPI_inside_nonatomic_context(void)
 		return false;			/* not in any SPI context at all */
 	if (_SPI_current->atomic)
 		return false;			/* it's atomic (ie function not procedure) */
+	if (IsSubTransaction())
+		return false;			/* if within subtransaction, it's atomic */
 	return true;
 }
 =

@@ -2411,9 +2413,12 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecute=
Options *options,
 =

 	/*
 	 * We allow nonatomic behavior only if options->allow_nonatomic is set
-	 * *and* the SPI_OPT_NONATOMIC flag was given when connecting.
+	 * *and* the SPI_OPT_NONATOMIC flag was given when connecting and we are
+	 * not inside a subtransaction.  The latter two tests match whether
+	 * _SPI_commit() would allow a commit.
 	 */
-	allow_nonatomic =3D options->allow_nonatomic && !_SPI_current->atomic;
+	allow_nonatomic =3D options->allow_nonatomic &&
+		!_SPI_current->atomic && !IsSubTransaction();
 =

 	/*
 	 * Setup error traceback support for ereport()
diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql=
/src/expected/plpgsql_call.out
index 0a63b1d44e..ea7107dca0 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_call.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_call.out
@@ -597,6 +597,26 @@ NOTICE:  f_get_x(1)
 NOTICE:  f_print_x(1)
 NOTICE:  f_get_x(2)
 NOTICE:  f_print_x(2)
+-- test in non-atomic context, except exception block is locally atomic
+DO $$
+BEGIN
+ 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());
+ EXCEPTION WHEN division_by_zero THEN
+   RAISE NOTICE '%', SQLERRM;
+ END;
+  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 $$
diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/=
sql/plpgsql_call.sql
index 4cbda0382e..08c1659ef1 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_call.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql
@@ -557,6 +557,23 @@ BEGIN
 END
 $$;
 =

+-- test in non-atomic context, except exception block is locally atomic
+DO $$
+BEGIN
+ 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());
+ EXCEPTION WHEN division_by_zero THEN
+   RAISE NOTICE '%', SQLERRM;
+ END;
+  ROLLBACK;
+END
+$$;
+
 -- test in atomic context
 BEGIN;
 =


------- =_aaaaaaaaaa0--