Re: BUG #17779: "commit" causes error 2D000 when "set search_path" is added to the first example in section "43.8"

From: Bryn Llewellyn <bryn(at)yugabyte(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>, david(dot)g(dot)johnston(at)gmail(dot)com
Cc: pgsql-bugs(at)lists(dot)postgresql(dot)org, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>
Subject: Re: BUG #17779: "commit" causes error 2D000 when "set search_path" is added to the first example in section "43.8"
Date: 2023-02-07 18:57:49
Message-ID: B2E57E85-722B-40BE-BFA9-BECFA88CE115@yugabyte.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

> andres(at)anarazel(dot)de wrote:
>
>> PG Bug reporting form wrote:
>>
>> However, when the "set search_path" line is uncommented, and procedure "s.transaction_test1()" is recompiled, it causes the 2D000 runtime error:
>>
>> invalid transaction termination
>>
>> Of course, now no rows are inserted into the target table. The outcome is the same if this is used:
>>
>> set search_path = pg_catalog, s, pg_temp
>>
>> for those who prefer less cluttered code. If this is a known bug, then please tell me the number.
>
> It's documented, although not that easy to find:
>
> https://www.postgresql.org/docs/15/sql-createprocedure.html
>
> «
> If a SET clause is attached to a procedure… However, an ordinary SET command (without LOCAL) overrides the SET clause, much as it would do for a previous SET LOCAL command: the effects of such a command will persist after procedure exit, unless the current transaction is rolled back. If a SET clause is attached to a procedure, then that procedure cannot execute transaction control statements (for example, COMMIT and ROLLBACK, depending on the language).
> »
>
> Perhaps this should be a <warning>?
>
> The relevant piece of code has an explanation as to why the restriction exists:
>
> /*
> * …That restriction could be lifted by redesigning the GUC nesting mechanism a bit.
> */
>
> ...this is a general restriction for procedures, not plpgsql specific.
>
>
> Seems like this should be mentioned in the plpgsql docs as well [here]:
>
> https://www.postgresql.org/docs/current/plpgsql-transactions.html

Thank you very much Andreas. And thank you, David, for your separate reply to the email that my posting of not-a-bug #17779 generated. I don't know the long history of PostgreSQL and I don't know its C implementation. From this P.o.V. There seem to be some arbitrary rules like these:

— A SECURITY DEFINER procedure cannot execute transaction control statements (quoted from the "create procedure" doc)

— A language SQL procedure cannot commit (the attempt causes "0A000: COMMIT is not allowed in a SQL function"). I can't (yet) find where this is documented. But it does make sense to prevent "select" causing a "commit"—so I'm sure that it's mentioned somewhere.

— A language plpgsql procedure that does "commit" and that works when invoked as a top-level call fails when it's invoked like this: "start transaction; call p(); commit;". (It causes "2D000: invalid transaction termination".) Documented with the "call" statement, as David pointed out. (There's a hint to this effect in "43.8. Transaction Management": "Transaction control is only possible in CALL or DO invocations from the top level…" but this wording doesn't call out the point as clearly as it might.)

However, it seems to me (from a mixture of anecdotal reports and intuition) that the overwhelmingly common (and therefore best understood) approach is always to set "autocommit" on and never to call a procedure within a top-level "start transaction; ... commit;" parenthesis. Then, close on the heels of this, it seems that having a procedure do "commit" (when this doesn't cause an immediate error) is pushing the boundaries and might end in tears. (For example, you might realize, having implemented working "security invoker" procedures with "commit" that they ought to be "security definer". Then you'd be stuck with a challenging major redesign.

If I go with this common approach, then the not-a-bug that I reported won't trouble me. (I had discovered that "commit" in a procedure provided a workaround for a short-term limitation in YugabyteDB—and added the "set search_path" attribute only when my scheme seemed to be working.)

Thanks, David, for your proposed workaround (to set the search_path in the procedure's executable section and to use the "local" flavor). If I did choose that, then I'd lose what I now enjoy: the ability to query the metadata for a set of functions and procedures of interest and to be able to see at a glance that every one sets the search_path *declaratively* to something sensible.

Of course, I'm guilty of not having remembered the relevant doc when I needed it. So, yes, it would help to add a x-ref from "43.8. Transaction Management" to "create procedure". It would also help to have a x-ref from from "43.8. Transaction Management" to the "call" doc.

I was seduced by the presently false notion that the declarative scheme for defining a run-time parameter with call-duration (i.e. the save and restore mechanism) simply worked by the same "local" magic that I could implement myself. I assume that the comment in the C code ("That restriction could be lifted by redesigning the GUC nesting mechanism a bit") means that magic like I'd imagined that I'd assume was already implemented could be done in a later PG Version.

In response to

Browse pgsql-bugs by date

  From Date Subject
Next Message Tom Lane 2023-02-07 23:26:57 Re: BUG #17781: Assert in setrefs.c
Previous Message David G. Johnston 2023-02-07 13:43:42 Re: BUG #17782: ERROR: variable not found in subplan target lists