Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix behavior of stable functions called from a CALL's argument list.

  1. Unexpected results from CALL and AUTOCOMMIT=off

    Victor Yegorov <vyegorov@gmail.com> — 2024-06-03T14:41:38Z

    Greetings.

I am observing the following results on PostgreSQL 15.7
First, setup:

create table t_test(x bigint);
insert into t_test values(0);

create or replace function f_get_x()
returns bigint
language plpgsql
stable
as $function$
declare
    l_result bigint;
begin
    select x into l_result from t_test;
    --raise notice 'f_get_x() >> x=%', l_result;
    --raise notice 'f_get_x() >> xact=%', txid_current_if_assigned();
    return l_result;
end;
$function$;

create or replace procedure f_print_x(x bigint)
language plpgsql
as $procedure$
begin
    raise notice 'f_print_x() >> x=%', x;
    --raise notice 'f_print_x() >> xact=%', txid_current_if_assigned();
end;
$procedure$;


Now, the case:
\set AUTOCOMMIT off
do
$$ begin
    --raise notice 'do >> xact=%', txid_current_if_assigned();
    update t_test set x = 1;
    --raise notice 'do >> xact=%', txid_current_if_assigned();
    raise notice 'do >> x=%', f_get_x();
    --raise notice 'do >> xact=%', txid_current_if_assigned();
    call f_print_x(f_get_x());
end; $$;
NOTICE:  do >> x=1
NOTICE:  f_print_x() >> x=0
DO

I don't understand why CALL statement is not seeing an updated record.
With AUTOCOMMIT=on, all goes as expected.

I tried to examine snapshots and xids (commented lines), but they're always
the same.

Can you explain this behavior, please? Is it expected?

-- 
Victor Yegorov

  2. Re: Unexpected results from CALL and AUTOCOMMIT=off

    pierre.forstmann@gmail.com — 2024-06-03T17:40:24Z

    You declared function f_get_x as stable which means:

https://www.postgresql.org/docs/15/sql-createfunction.html

STABLE indicates that the function cannot modify the database, and that
within a single table scan it will consistently return the same result for
the same argument values, but that its result could change across SQL
statements. This is the appropriate selection for functions whose results
depend on database lookups, parameter variables (such as the current time
zone), etc. (It is inappropriate for AFTER triggers that wish to query rows
modified by the current command.) Also note that the current_timestamp
family of functions qualify as stable, since their values do not change
within a transaction.

If you remove stable from function declaration, it works as expected:

drop table t_test;
DROP TABLE
create table t_test(x bigint);
CREATE TABLE
insert into t_test values(0);
INSERT 0 1
create or replace function f_get_x()
returns bigint
language plpgsql
-- stable
as $function$
declare
    l_result bigint;
begin
    select x into l_result from t_test;
    --raise notice 'f_get_x() >> x=%', l_result;
    --raise notice 'f_get_x() >> xact=%', txid_current_if_assigned();
    return l_result;
end;
$function$;
CREATE FUNCTION
create or replace procedure f_print_x(x bigint)
language plpgsql
as $procedure$
begin
    raise notice 'f_print_x() >> x=%', x;
    --raise notice 'f_print_x() >> xact=%', txid_current_if_assigned();
end;
$procedure$;
CREATE PROCEDURE
do
$$ begin
    --raise notice 'do >> xact=%', txid_current_if_assigned();
    update t_test set x = 1;
    --raise notice 'do >> xact=%', txid_current_if_assigned();
    raise notice 'do >> x=%', f_get_x();
    --raise notice 'do >> xact=%', txid_current_if_assigned();
    call f_print_x(f_get_x());
end; $$;
psql:test.sql:38: NOTICE:  do >> x=1
psql:test.sql:38: NOTICE:  f_print_x() >> x=1
DO

Le lun. 3 juin 2024 à 16:42, Victor Yegorov <vyegorov@gmail.com> a écrit :

> Greetings.
>
> I am observing the following results on PostgreSQL 15.7
> First, setup:
>
> create table t_test(x bigint);
> insert into t_test values(0);
>
> create or replace function f_get_x()
> returns bigint
> language plpgsql
> stable
> as $function$
> declare
>     l_result bigint;
> begin
>     select x into l_result from t_test;
>     --raise notice 'f_get_x() >> x=%', l_result;
>     --raise notice 'f_get_x() >> xact=%', txid_current_if_assigned();
>     return l_result;
> end;
> $function$;
>
> create or replace procedure f_print_x(x bigint)
> language plpgsql
> as $procedure$
> begin
>     raise notice 'f_print_x() >> x=%', x;
>     --raise notice 'f_print_x() >> xact=%', txid_current_if_assigned();
> end;
> $procedure$;
>
>
> Now, the case:
> \set AUTOCOMMIT off
> do
> $$ begin
>     --raise notice 'do >> xact=%', txid_current_if_assigned();
>     update t_test set x = 1;
>     --raise notice 'do >> xact=%', txid_current_if_assigned();
>     raise notice 'do >> x=%', f_get_x();
>     --raise notice 'do >> xact=%', txid_current_if_assigned();
>     call f_print_x(f_get_x());
> end; $$;
> NOTICE:  do >> x=1
> NOTICE:  f_print_x() >> x=0
> DO
>
> I don't understand why CALL statement is not seeing an updated record.
> With AUTOCOMMIT=on, all goes as expected.
>
> I tried to examine snapshots and xids (commented lines), but they're
> always the same.
>
> Can you explain this behavior, please? Is it expected?
>
> --
> Victor Yegorov
>

  3. Re: Unexpected results from CALL and AUTOCOMMIT=off

    Victor Yegorov <vyegorov@gmail.com> — 2024-06-03T18:15:07Z

    пн, 3 июн. 2024 г. в 20:40, Pierre Forstmann <pierre.forstmann@gmail.com>:

> You declared function f_get_x as stable which means:
>
> …
>
> If you remove stable from function declaration, it works as expected:
>

Well, I checked
https://www.postgresql.org/docs/current/xfunc-volatility.html
There's a paragraph describing why STABLE (and IMMUTABLE) use different
snapshots:

> For functions written in SQL or in any of the standard procedural
languages, there is a second important property determined by the
volatility category, namely the visibility of any data changes that have
been made by the SQL command that is calling the function. A > VOLATILE
function will see such changes, a STABLE or IMMUTABLE function will not.
This behavior is implemented using the snapshotting behavior of MVCC (see
Chapter 13): STABLE and IMMUTABLE functions use a snapshot established as
of the start of the
> calling query, whereas VOLATILE functions obtain a fresh snapshot at the
start of each query they execute.

But later, docs state, that

> Because of this snapshotting behavior, a function containing only SELECT
commands can safely be marked STABLE, even if it selects from tables that
might be undergoing modifications by concurrent queries. PostgreSQL will
execute all commands of a STABLE function using the snapshot established
for the calling query, and so it will see a fixed view of the database
throughout that query.

And therefore I assume STABLE should work in this case. Well, it seems not
to.

I assume there's smth to do with implicit BEGIN issued in non-AUTOCOMMIT
mode and non-atomic DO block behaviour.


-- 
Victor Yegorov

  4. Re: Unexpected results from CALL and AUTOCOMMIT=off

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-03T19:28:10Z

    Victor Yegorov <vyegorov@gmail.com> writes:
> пн, 3 июн. 2024 г. в 20:40, Pierre Forstmann <pierre.forstmann@gmail.com>:
>> If you remove stable from function declaration, it works as expected:

> ... therefore I assume STABLE should work in this case. Well, it seems not
> to.

I agree that this looks like a bug, since your example shows that the
same function works as-expected in an ordinary expression but not in
a CALL.  The dependency on AUTOCOMMIT (that is, being within an outer
transaction block) seems even odder.  I've not dug into it yet, but
I suppose we're passing the wrong snapshot to the CALL arguments.
A volatile function wouldn't use that snapshot, explaining Pierre's
result.

			regards, tom lane

  5. Re: Unexpected results from CALL and AUTOCOMMIT=off

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-04T01:32:28Z

    [ redirecting to pgsql-hackers ]

I wrote:
> I agree that this looks like a bug, since your example shows that the
> same function works as-expected in an ordinary expression but not in
> a CALL.  The dependency on AUTOCOMMIT (that is, being within an outer
> transaction block) seems even odder.  I've not dug into it yet, but
> I suppose we're passing the wrong snapshot to the CALL arguments.

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.
I've not attempted to make those doc updates in the attached draft
patch though, nor have I added a test case yet.

Before realizing that this was the issue, I spent a fair amount of
time on the idea that _SPI_execute_plan() was doing things wrong,
and that led me to notice that its comment about having four modes
of snapshot operation has been falsified in multiple ways.  So this
draft does include fixes for that comment.

Thoughts?

			regards, tom lane

  6. Re: Unexpected results from CALL and AUTOCOMMIT=off

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-04T18:28:43Z

    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

  7. Re: Unexpected results from CALL and AUTOCOMMIT=off

    Nathan Bossart <nathandbossart@gmail.com> — 2024-06-04T20:13:12Z

    On Tue, Jun 04, 2024 at 02:28:43PM -0400, Tom Lane wrote:
> 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.

+1

> Hence, new patch attached, now with docs and tests.  Barring
> objections I'll push this one.

Should we expand the documentation for SPI_connect_ext() to note that
SPI_execute_extended()/SPI_execute_plan_extended() depend on the flag?

-- 
nathan

  8. Re: Unexpected results from CALL and AUTOCOMMIT=off

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-04T20:31:34Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> On Tue, Jun 04, 2024 at 02:28:43PM -0400, Tom Lane wrote:
>> Hence, new patch attached, now with docs and tests.  Barring
>> objections I'll push this one.

> Should we expand the documentation for SPI_connect_ext() to note that
> SPI_execute_extended()/SPI_execute_plan_extended() depend on the flag?

Perhaps.  They already did, in that the atomic flag was taken into
account while deciding how to handle a nested CALL; basically what this
fix does is to make sure that the snapshot handling is done the same
way.  I think that what I added to the docs is probably sufficient,
but I'll yield to majority opinion if people think not.

			regards, tom lane