Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 2dc1dea

Browse files
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 d92573a commit 2dc1dea

File tree

4 files changed

+130
-14
lines changed

4 files changed

+130
-14
lines changed

doc/src/sgml/spi.sgml

+6-2
Original file line numberDiff line numberDiff line change
@@ -752,7 +752,9 @@ int SPI_execute_extended(const char *<parameter>command</parameter>,
752752
<listitem>
753753
<para>
754754
<literal>true</literal> allows non-atomic execution of CALL and DO
755-
statements
755+
statements (but this field is ignored unless
756+
the <symbol>SPI_OPT_NONATOMIC</symbol> flag was passed
757+
to <function>SPI_connect_ext</function>)
756758
</para>
757759
</listitem>
758760
</varlistentry>
@@ -1893,7 +1895,9 @@ int SPI_execute_plan_extended(SPIPlanPtr <parameter>plan</parameter>,
18931895
<listitem>
18941896
<para>
18951897
<literal>true</literal> allows non-atomic execution of CALL and DO
1896-
statements
1898+
statements (but this field is ignored unless
1899+
the <symbol>SPI_OPT_NONATOMIC</symbol> flag was passed
1900+
to <function>SPI_connect_ext</function>)
18971901
</para>
18981902
</listitem>
18991903
</varlistentry>

src/backend/executor/spi.c

+24-12
Original file line numberDiff line numberDiff line change
@@ -2399,13 +2399,20 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
23992399
uint64 my_processed = 0;
24002400
SPITupleTable *my_tuptable = NULL;
24012401
int res = 0;
2402+
bool allow_nonatomic;
24022403
bool pushed_active_snap = false;
24032404
ResourceOwner plan_owner = options->owner;
24042405
SPICallbackArg spicallbackarg;
24052406
ErrorContextCallback spierrcontext;
24062407
CachedPlan *cplan = NULL;
24072408
ListCell *lc1;
24082409

2410+
/*
2411+
* We allow nonatomic behavior only if options->allow_nonatomic is set
2412+
* *and* the SPI_OPT_NONATOMIC flag was given when connecting.
2413+
*/
2414+
allow_nonatomic = options->allow_nonatomic && !_SPI_current->atomic;
2415+
24092416
/*
24102417
* Setup error traceback support for ereport()
24112418
*/
@@ -2425,12 +2432,17 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
24252432
* snapshot != InvalidSnapshot, read_only = false: use the given snapshot,
24262433
* modified by advancing its command ID before each querytree.
24272434
*
2428-
* snapshot == InvalidSnapshot, read_only = true: use the entry-time
2429-
* ActiveSnapshot, if any (if there isn't one, we run with no snapshot).
2435+
* snapshot == InvalidSnapshot, read_only = true: do nothing for queries
2436+
* that require no snapshot. For those that do, ensure that a Portal
2437+
* snapshot exists; then use that, or use the entry-time ActiveSnapshot if
2438+
* that exists and is different.
24302439
*
2431-
* snapshot == InvalidSnapshot, read_only = false: take a full new
2432-
* snapshot for each user command, and advance its command ID before each
2433-
* querytree within the command.
2440+
* snapshot == InvalidSnapshot, read_only = false: do nothing for queries
2441+
* that require no snapshot. For those that do, ensure that a Portal
2442+
* snapshot exists; then, in atomic execution (!allow_nonatomic) take a
2443+
* full new snapshot for each user command, and advance its command ID
2444+
* before each querytree within the command. In allow_nonatomic mode we
2445+
* just use the Portal snapshot unmodified.
24342446
*
24352447
* In the first two cases, we can just push the snap onto the stack once
24362448
* for the whole plan list.
@@ -2440,6 +2452,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
24402452
*/
24412453
if (snapshot != InvalidSnapshot)
24422454
{
2455+
/* this intentionally tests the options field not the derived value */
24432456
Assert(!options->allow_nonatomic);
24442457
if (options->read_only)
24452458
{
@@ -2585,7 +2598,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
25852598
* Skip it when doing non-atomic execution, though (we rely
25862599
* entirely on the Portal snapshot in that case).
25872600
*/
2588-
if (!options->read_only && !options->allow_nonatomic)
2601+
if (!options->read_only && !allow_nonatomic)
25892602
{
25902603
if (pushed_active_snap)
25912604
PopActiveSnapshot();
@@ -2685,14 +2698,13 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
26852698
QueryCompletion qc;
26862699

26872700
/*
2688-
* If the SPI context is atomic, or we were not told to allow
2689-
* nonatomic operations, tell ProcessUtility this is an atomic
2690-
* execution context.
2701+
* If we're not allowing nonatomic operations, tell
2702+
* ProcessUtility this is an atomic execution context.
26912703
*/
2692-
if (_SPI_current->atomic || !options->allow_nonatomic)
2693-
context = PROCESS_UTILITY_QUERY;
2694-
else
2704+
if (allow_nonatomic)
26952705
context = PROCESS_UTILITY_QUERY_NONATOMIC;
2706+
else
2707+
context = PROCESS_UTILITY_QUERY;
26962708

26972709
InitializeQueryCompletion(&qc);
26982710
ProcessUtility(stmt,

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

+50
Original file line numberDiff line numberDiff line change
@@ -564,3 +564,53 @@ NOTICE: inner_p(44)
564564

565565
(1 row)
566566

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

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

+50
Original file line numberDiff line numberDiff line change
@@ -522,3 +522,53 @@ CREATE FUNCTION f(int) RETURNS int AS $$ SELECT $1 + 2 $$ LANGUAGE sql;
522522

523523
CALL outer_p(42);
524524
SELECT outer_f(42);
525+
526+
-- Check that stable functions in CALL see the correct snapshot
527+
528+
CREATE TABLE t_test (x int);
529+
INSERT INTO t_test VALUES (0);
530+
531+
CREATE FUNCTION f_get_x () RETURNS int
532+
AS $$
533+
DECLARE l_result int;
534+
BEGIN
535+
SELECT x INTO l_result FROM t_test;
536+
RETURN l_result;
537+
END
538+
$$ LANGUAGE plpgsql STABLE;
539+
540+
CREATE PROCEDURE f_print_x (x int)
541+
AS $$
542+
BEGIN
543+
RAISE NOTICE 'f_print_x(%)', x;
544+
END
545+
$$ LANGUAGE plpgsql;
546+
547+
-- test in non-atomic context
548+
DO $$
549+
BEGIN
550+
UPDATE t_test SET x = x + 1;
551+
RAISE NOTICE 'f_get_x(%)', f_get_x();
552+
CALL f_print_x(f_get_x());
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+
ROLLBACK;
557+
END
558+
$$;
559+
560+
-- test in atomic context
561+
BEGIN;
562+
563+
DO $$
564+
BEGIN
565+
UPDATE t_test SET x = x + 1;
566+
RAISE NOTICE 'f_get_x(%)', f_get_x();
567+
CALL f_print_x(f_get_x());
568+
UPDATE t_test SET x = x + 1;
569+
RAISE NOTICE 'f_get_x(%)', f_get_x();
570+
CALL f_print_x(f_get_x());
571+
END
572+
$$;
573+
574+
ROLLBACK;

0 commit comments

Comments
 (0)