Skip to content

Commit 0f7d133

Browse files
tglsfdctglsfdc
committed
Fix behavior of stable functions called from a CALL's argument list.
If the CALL is within an atomic context (e.g. there's an outer transaction block), _SPI_execute_plan should acquire a fresh snapshot to execute any such functions with. We failed to do that and instead passed them the Portal snapshot, which had been acquired at the start of the current SQL command. This'd lead to seeing stale values of rows modified since the start of the command. This is arguably a bug in 84f5c29: I failed to see that "are we in non-atomic mode" needs to be defined the same way as it is further down in _SPI_execute_plan, i.e. check !_SPI_current->atomic not just options->allow_nonatomic. Alternatively the blame could be laid on plpgsql, which is unconditionally passing allow_nonatomic = true for CALL/DO even when it knows it's in an atomic context. However, fixing it in spi.c seems like a better idea since that will also fix the problem for any extensions that may have copied plpgsql's coding pattern. While here, update an obsolete comment about _SPI_execute_plan's snapshot management. Per report from Victor Yegorov. Back-patch to all supported versions. Discussion: https://postgr.es/m/CAGnEboiRe+fG2QxuBO2390F7P8e2MQ6UyBjZSL_w1Cej+E4=Vw@mail.gmail.com
1 parent 269e2c3 commit 0f7d133

File tree

4 files changed

+130
-14
lines changed

4 files changed

+130
-14
lines changed

‎doc/src/sgml/spi.sgml

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,9 @@ int SPI_execute_extended(const char *<parameter>command</parameter>,
733733
<listitem>
734734
<para>
735735
<literal>true</literal> allows non-atomic execution of CALL and DO
736-
statements
736+
statements (but this field is ignored unless
737+
the <symbol>SPI_OPT_NONATOMIC</symbol> flag was passed
738+
to <function>SPI_connect_ext</function>)
737739
</para>
738740
</listitem>
739741
</varlistentry>
@@ -1874,7 +1876,9 @@ int SPI_execute_plan_extended(SPIPlanPtr <parameter>plan</parameter>,
18741876
<listitem>
18751877
<para>
18761878
<literal>true</literal> allows non-atomic execution of CALL and DO
1877-
statements
1879+
statements (but this field is ignored unless
1880+
the <symbol>SPI_OPT_NONATOMIC</symbol> flag was passed
1881+
to <function>SPI_connect_ext</function>)
18781882
</para>
18791883
</listitem>
18801884
</varlistentry>

‎src/backend/executor/spi.c

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2406,13 +2406,20 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
24062406
uint64 my_processed = 0;
24072407
SPITupleTable *my_tuptable = NULL;
24082408
int res = 0;
2409+
bool allow_nonatomic;
24092410
bool pushed_active_snap = false;
24102411
ResourceOwner plan_owner = options->owner;
24112412
SPICallbackArg spicallbackarg;
24122413
ErrorContextCallback spierrcontext;
24132414
CachedPlan *cplan = NULL;
24142415
ListCell *lc1;
24152416

2417+
/*
2418+
* We allow nonatomic behavior only if options->allow_nonatomic is set
2419+
* *and* the SPI_OPT_NONATOMIC flag was given when connecting.
2420+
*/
2421+
allow_nonatomic = options->allow_nonatomic && !_SPI_current->atomic;
2422+
24162423
/*
24172424
* Setup error traceback support for ereport()
24182425
*/
@@ -2432,12 +2439,17 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
24322439
* snapshot != InvalidSnapshot, read_only = false: use the given snapshot,
24332440
* modified by advancing its command ID before each querytree.
24342441
*
2435-
* snapshot == InvalidSnapshot, read_only = true: use the entry-time
2436-
* ActiveSnapshot, if any (if there isn't one, we run with no snapshot).
2442+
* snapshot == InvalidSnapshot, read_only = true: do nothing for queries
2443+
* that require no snapshot. For those that do, ensure that a Portal
2444+
* snapshot exists; then use that, or use the entry-time ActiveSnapshot if
2445+
* that exists and is different.
24372446
*
2438-
* snapshot == InvalidSnapshot, read_only = false: take a full new
2439-
* snapshot for each user command, and advance its command ID before each
2440-
* querytree within the command.
2447+
* snapshot == InvalidSnapshot, read_only = false: do nothing for queries
2448+
* that require no snapshot. For those that do, ensure that a Portal
2449+
* snapshot exists; then, in atomic execution (!allow_nonatomic) take a
2450+
* full new snapshot for each user command, and advance its command ID
2451+
* before each querytree within the command. In allow_nonatomic mode we
2452+
* just use the Portal snapshot unmodified.
24412453
*
24422454
* In the first two cases, we can just push the snap onto the stack once
24432455
* for the whole plan list.
@@ -2447,6 +2459,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
24472459
*/
24482460
if (snapshot != InvalidSnapshot)
24492461
{
2462+
/* this intentionally tests the options field not the derived value */
24502463
Assert(!options->allow_nonatomic);
24512464
if (options->read_only)
24522465
{
@@ -2592,7 +2605,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
25922605
* Skip it when doing non-atomic execution, though (we rely
25932606
* entirely on the Portal snapshot in that case).
25942607
*/
2595-
if (!options->read_only && !options->allow_nonatomic)
2608+
if (!options->read_only && !allow_nonatomic)
25962609
{
25972610
if (pushed_active_snap)
25982611
PopActiveSnapshot();
@@ -2692,14 +2705,13 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
26922705
QueryCompletion qc;
26932706

26942707
/*
2695-
* If the SPI context is atomic, or we were not told to allow
2696-
* nonatomic operations, tell ProcessUtility this is an atomic
2697-
* execution context.
2708+
* If we're not allowing nonatomic operations, tell
2709+
* ProcessUtility this is an atomic execution context.
26982710
*/
2699-
if (_SPI_current->atomic || !options->allow_nonatomic)
2700-
context = PROCESS_UTILITY_QUERY;
2701-
else
2711+
if (allow_nonatomic)
27022712
context = PROCESS_UTILITY_QUERY_NONATOMIC;
2713+
else
2714+
context = PROCESS_UTILITY_QUERY;
27032715

27042716
InitializeQueryCompletion(&qc);
27052717
ProcessUtility(stmt,

‎src/pl/plpgsql/src/expected/plpgsql_call.out

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -552,3 +552,53 @@ NOTICE: inner_p(44)
552552

553553
(1 row)
554554

555+
-- Check that stable functions in CALL see the correct snapshot
556+
CREATE TABLE t_test (x int);
557+
INSERT INTO t_test VALUES (0);
558+
CREATE FUNCTION f_get_x () RETURNS int
559+
AS $$
560+
DECLARE l_result int;
561+
BEGIN
562+
SELECT x INTO l_result FROM t_test;
563+
RETURN l_result;
564+
END
565+
$$ LANGUAGE plpgsql STABLE;
566+
CREATE PROCEDURE f_print_x (x int)
567+
AS $$
568+
BEGIN
569+
RAISE NOTICE 'f_print_x(%)', x;
570+
END
571+
$$ LANGUAGE plpgsql;
572+
-- test in non-atomic context
573+
DO $$
574+
BEGIN
575+
UPDATE t_test SET x = x + 1;
576+
RAISE NOTICE 'f_get_x(%)', f_get_x();
577+
CALL f_print_x(f_get_x());
578+
UPDATE t_test SET x = x + 1;
579+
RAISE NOTICE 'f_get_x(%)', f_get_x();
580+
CALL f_print_x(f_get_x());
581+
ROLLBACK;
582+
END
583+
$$;
584+
NOTICE: f_get_x(1)
585+
NOTICE: f_print_x(1)
586+
NOTICE: f_get_x(2)
587+
NOTICE: f_print_x(2)
588+
-- test in atomic context
589+
BEGIN;
590+
DO $$
591+
BEGIN
592+
UPDATE t_test SET x = x + 1;
593+
RAISE NOTICE 'f_get_x(%)', f_get_x();
594+
CALL f_print_x(f_get_x());
595+
UPDATE t_test SET x = x + 1;
596+
RAISE NOTICE 'f_get_x(%)', f_get_x();
597+
CALL f_print_x(f_get_x());
598+
END
599+
$$;
600+
NOTICE: f_get_x(1)
601+
NOTICE: f_print_x(1)
602+
NOTICE: f_get_x(2)
603+
NOTICE: f_print_x(2)
604+
ROLLBACK;

‎src/pl/plpgsql/src/sql/plpgsql_call.sql

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,3 +510,53 @@ CREATE FUNCTION f(int) RETURNS int AS $$ SELECT $1 + 2 $$ LANGUAGE sql;
510510

511511
CALL outer_p(42);
512512
SELECT outer_f(42);
513+
514+
-- Check that stable functions in CALL see the correct snapshot
515+
516+
CREATE TABLE t_test (x int);
517+
INSERT INTO t_test VALUES (0);
518+
519+
CREATE FUNCTION f_get_x () RETURNS int
520+
AS $$
521+
DECLARE l_result int;
522+
BEGIN
523+
SELECT x INTO l_result FROM t_test;
524+
RETURN l_result;
525+
END
526+
$$ LANGUAGE plpgsql STABLE;
527+
528+
CREATE PROCEDURE f_print_x (x int)
529+
AS $$
530+
BEGIN
531+
RAISE NOTICE 'f_print_x(%)', x;
532+
END
533+
$$ LANGUAGE plpgsql;
534+
535+
-- test in non-atomic context
536+
DO $$
537+
BEGIN
538+
UPDATE t_test SET x = x + 1;
539+
RAISE NOTICE 'f_get_x(%)', f_get_x();
540+
CALL f_print_x(f_get_x());
541+
UPDATE t_test SET x = x + 1;
542+
RAISE NOTICE 'f_get_x(%)', f_get_x();
543+
CALL f_print_x(f_get_x());
544+
ROLLBACK;
545+
END
546+
$$;
547+
548+
-- test in atomic context
549+
BEGIN;
550+
551+
DO $$
552+
BEGIN
553+
UPDATE t_test SET x = x + 1;
554+
RAISE NOTICE 'f_get_x(%)', f_get_x();
555+
CALL f_print_x(f_get_x());
556+
UPDATE t_test SET x = x + 1;
557+
RAISE NOTICE 'f_get_x(%)', f_get_x();
558+
CALL f_print_x(f_get_x());
559+
END
560+
$$;
561+
562+
ROLLBACK;

0 commit comments

Comments
 (0)