Thread: BUG #18656: "STABLE" function sometimes does not see changes

The following bug has been logged on the website:

Bug reference:      18656
Logged by:          Alexander Alehin
Email address:      panso8@gmail.com
PostgreSQL version: 16.4
Operating system:   Debian 11
Description:

CREATE TABLE stbl_states(
  id    int primary key,
  state text
);

CREATE TABLE stbl_logs(
  ts timestamp default clock_timestamp(),
  log text
);

CREATE or replace FUNCTION stbl_get_state(p_id int) RETURNS text
  LANGUAGE sql STABLE AS
$$
select coalesce(k.state, 'off') from stbl_states k where k.id = p_id;
$$;

CREATE or replace PROCEDURE stbl_log(p_message text)
  LANGUAGE plpgsql AS
$$
begin
  insert into stbl_logs(log) values (p_message);
end;
$$;

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;
$$;

insert into stbl_states(id) values(1);
call stbl_update_state();
select log from stbl_logs order by ts;

---------------------------------------------------------------

 log
-----
 on
 off
 on
(3 rows)

But expected:
on
on
on

If "STABLE" or "exception ..." is removed, it will work as expected.

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

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 SPIExecuteOptions *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 = options->allow_nonatomic && !_SPI_current->atomic;
+    allow_nonatomic = 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 = x + 1;
+  RAISE NOTICE 'f_get_x(%)', f_get_x();
+  CALL f_print_x(f_get_x());
+  UPDATE t_test SET x = 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 = x + 1;
+  RAISE NOTICE 'f_get_x(%)', f_get_x();
+  CALL f_print_x(f_get_x());
+  UPDATE t_test SET x = 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;

I wrote:
> 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.

I dug into this further and identified exactly where it's going off
the rails.  As things stand, _SPI_execute_plan thinks it can operate
non-atomically, so it doesn't push a snapshot and it passes
PROCESS_UTILITY_QUERY_NONATOMIC to ProcessUtility.  But in
standard_ProcessUtility we find

    bool        isAtomicContext = (!(context == PROCESS_UTILITY_TOPLEVEL || context == PROCESS_UTILITY_QUERY_NONATOMIC)
||IsTransactionBlock()); 

IsTransactionBlock() is always true in a subtransaction, so we pass
down atomic = true to ExecuteCallStmt, which then thinks it doesn't
need to push a snapshot either.  End result is that the stable
function runs with the Portal snapshot and sees stale data.

So my patch fixes it by ensuring that _SPI_execute_plan will think
it's atomic in the same cases that standard_ProcessUtility will force
that.  It's a little concerning though that standard_ProcessUtility is
arriving at its result through a completely different bit of
reasoning.  There's an argument to be made that the above-quoted bit
of logic needs revision.

Nonetheless, it seems correct to me that the different parts of spi.c
share identical rules for what is a nonatomic execution environment.
I'm also fairly leery of touching standard_ProcessUtility's logic in a
back-patch --- that seems way too likely to have unforeseen side
effects on unrelated parts of the system.  So I went ahead and pushed
what I had (after a bit more work on the comments).  We can think
about whether to revise the standard_ProcessUtility logic at leisure.

            regards, tom lane