Thread: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

The following bug has been logged on the website:

Bug reference:      17385
Logged by:          Andrew Bille
Email address:      andrewbille@gmail.com
PostgreSQL version: 14.1
Operating system:   Ubuntu 20.04
Description:

Hi!

"RESET transaction_isolation" inside serializable transaction causes Assert
at the transaction end.
For example (on REL_10_STABLE) the following query:
CREATE TABLE abc (a int);
BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
        SELECT * FROM abc;
        RESET transaction_isolation;
END;

causes an assertion failure with the following stack:

[New LWP 839017]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `postgres: andrew regression [local] COMMIT
                        '.
Program terminated with signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f14b7f59859 in __GI_abort () at abort.c:79
#2  0x00005586da82c4d2 in ExceptionalCondition (conditionName=0x5586daa01e42
"IsolationIsSerializable()", errorType=0x5586daa0195c "FailedAssertion",
fileName=0x5586daa01950 "predicate.c", lineNumber=4838) at assert.c:69
#3  0x00005586da659b2f in PreCommit_CheckForSerializationFailure () at
predicate.c:4838
#4  0x00005586da20f32e in CommitTransaction () at xact.c:2168
#5  0x00005586da21020c in CommitTransactionCommand () at xact.c:3015
#6  0x00005586da66afcb in finish_xact_command () at postgres.c:2721
#7  0x00005586da668643 in exec_simple_query (query_string=0x5586dada1020
"END;") at postgres.c:1239
#8  0x00005586da66d4db in PostgresMain (argc=1, argv=0x7fffefd1c050,
dbname=0x5586dadcc168 "regression", username=0x5586dadcc148 "andrew") at
postgres.c:4486
#9  0x00005586da591be3 in BackendRun (port=0x5586dadc2010) at
postmaster.c:4530
#10 0x00005586da59143e in BackendStartup (port=0x5586dadc2010) at
postmaster.c:4252
#11 0x00005586da58d233 in ServerLoop () at postmaster.c:1745
#12 0x00005586da58c990 in PostmasterMain (argc=3, argv=0x5586dad9b240) at
postmaster.c:1417
#13 0x00005586da47be9d in main (argc=3, argv=0x5586dad9b240) at main.c:209

Reproduced on REL_10_STABLE..master.
However SET transaction_isolation='read committed' inside transaction just
emits
"ERROR:  SET TRANSACTION ISOLATION LEVEL must be called before any query".

Regards, Andrew!

Hi,

Additional information. Query
"RESET transaction_isolation;"
in previous message can be replaced to synonym
"SET transaction_isolation=DEFAULT;"
(error will be the same).

I attached file with simple fix for branch "master".

I not sure that need to use a separate block of code for the
"transaction_isolation" GUC-variable. But this is a special variable
and there are several places in the code with handling of this variable.

With best regards,
Dmitry Koval.

On Sat, Jan 29, 2022 at 12:26 AM Dmitry Koval <d.koval@postgrespro.ru> wrote:
>
> Hi,
>
> Additional information. Query
> "RESET transaction_isolation;"
> in previous message can be replaced to synonym
> "SET transaction_isolation=DEFAULT;"
> (error will be the same).
>
> I attached file with simple fix for branch "master".
>
> I not sure that need to use a separate block of code for the
> "transaction_isolation" GUC-variable. But this is a special variable
> and there are several places in the code with handling of this variable.

IIUC this problem comes from the fact that RESET command doesn't call
check_hook of GUC parameters. Therefore, all GUC parameters that don’t
expect to be changed after something operations are affected. For
example, in addition to transaction_isolation, we don’t support
changing transaction_read_only and default_transaction_deferrable
after the first snapshot is taken. Also, we don’t support changing
temp_buffers after accessing any temp tables in the session. But
RESET’ing these parameters bypasses the check. Considering these
facts, I think it’s better to call check_hook even in resetting cases.
I've attached a patch (with regression tests) for discussion.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Fri, Feb 4, 2022 at 2:19 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sat, Jan 29, 2022 at 12:26 AM Dmitry Koval <d.koval@postgrespro.ru> wrote:
> >
> > Hi,
> >
> > Additional information. Query
> > "RESET transaction_isolation;"
> > in previous message can be replaced to synonym
> > "SET transaction_isolation=DEFAULT;"
> > (error will be the same).
> >
> > I attached file with simple fix for branch "master".
> >
> > I not sure that need to use a separate block of code for the
> > "transaction_isolation" GUC-variable. But this is a special variable
> > and there are several places in the code with handling of this variable.
>
> IIUC this problem comes from the fact that RESET command doesn't call
> check_hook of GUC parameters. Therefore, all GUC parameters that don’t
> expect to be changed after something operations are affected. For
> example, in addition to transaction_isolation, we don’t support
> changing transaction_read_only and default_transaction_deferrable
> after the first snapshot is taken. Also, we don’t support changing
> temp_buffers after accessing any temp tables in the session. But
> RESET’ing these parameters bypasses the check. Considering these
> facts, I think it’s better to call check_hook even in resetting cases.
> I've attached a patch (with regression tests) for discussion.

+1 for the fix.  I have some comments on the patch

1.
+
+                    /* non-NULL value must always get strdup'd */
+                    if (newval)
                     {
-                        newval = guc_strdup(elevel, conf->boot_val);
+                        newval = guc_strdup(elevel, newval);
                         if (newval == NULL)
                             return 0;
                     }

+                    if (source != PGC_S_DEFAULT)
+                    {
+                        /* Release newextra as we use reset_extra */
+                        if (newextra)
+                            free(newextra);
+
+                        /*
+                         * strdup not needed, since reset_val is already under
+                         * guc.c's control
+                         */
+                        newval = conf->reset_val;
+                        newextra = conf->reset_extra;

In  if (source != PGC_S_DEFAULT) we are overwriting newval with
conf->reset_val so I think we should free the newval or can we even
avoid guc_strdup in case of (source != PGC_S_DEFAULT)?

2. There is one compilation warning

guc.c: In function ‘set_config_option’:
guc.c:7918:14: warning: assignment discards ‘const’ qualifier from
pointer target type [enabled by default]
       newval = conf->boot_val;



--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

> In  if (source != PGC_S_DEFAULT) we are overwriting newval with
> conf->reset_val so I think we should free the newval or can we even
> avoid guc_strdup in case of (source != PGC_S_DEFAULT)?

I think we can not avoid guc_strdup in case of (source != PGC_S_DEFAULT) 
because function call_string_check_hook() contains "free(*newval);" in 
PG_CATCH-section.
Probably we should free the newval in block

+                    if (source != PGC_S_DEFAULT)
+                    {
+                        /* Release newextra as we use reset_extra */
+                        if (newextra)
+                            free(newextra);

With best regards,
Dmitry Koval.

On Sun, Feb 6, 2022 at 1:07 PM Dmitry Koval <d.koval@postgrespro.ru> wrote:
>
> > In  if (source != PGC_S_DEFAULT) we are overwriting newval with
> > conf->reset_val so I think we should free the newval or can we even
> > avoid guc_strdup in case of (source != PGC_S_DEFAULT)?
>
> I think we can not avoid guc_strdup in case of (source != PGC_S_DEFAULT)
> because function call_string_check_hook() contains "free(*newval);" in
> PG_CATCH-section.
> Probably we should free the newval in block

+1


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Hi,

I a bit changed Masahiko Sawada's patch with using Dilip Kumar's 
recommendations (see it in attachment).

About testing.
The most simple path to testing of command "RESET <GUC_string_variable>" 
(with error) that I found:
----
1) need to modify "postgresql.conf". Add string:

default_table_access_method = 'heapXXX'

2) start PostgreSQL;

3) start "psql" and execute command:

RESET default_table_access_method;

After that we get an error:

ERROR:  invalid value for parameter "default_table_access_method": "heapXXX"
DETAIL:  Table access method "heapXXX" does not exist.

Without attached patch command "RESET default_table_access_method;" 
returns no error.
----
Probably no reason to create new tap-test for this case...

With best regards,
Dmitry Koval.

On Mon, Feb 7, 2022 at 6:21 AM Dmitry Koval <d.koval@postgrespro.ru> wrote:
>
> Hi,
>
> I a bit changed Masahiko Sawada's patch with using Dilip Kumar's
> recommendations (see it in attachment).

Thank you for updating the patch!

 +
 +                   if (source != PGC_S_DEFAULT)
 +                   {
 +                       /* Release newextra as we use reset_extra */
 +                       if (newextra)
 +                           free(newextra);
 +
 +                       newextra = conf->reset_extra;
 +                       source = conf->gen.reset_source;
 +                       context = conf->gen.reset_scontext;
 +                   }

I think it's better to check if "!extra_field_used(&conf->gen,
newextra)" before freeing newextra because otherwise, it's possible
that we free reset_extra. I've updated an updated patch, please check
it.

>
> About testing.
> The most simple path to testing of command "RESET <GUC_string_variable>"
> (with error) that I found:
> ----
> 1) need to modify "postgresql.conf". Add string:
>
> default_table_access_method = 'heapXXX'
>
> 2) start PostgreSQL;
>
> 3) start "psql" and execute command:
>
> RESET default_table_access_method;
>
> After that we get an error:
>
> ERROR:  invalid value for parameter "default_table_access_method": "heapXXX"
> DETAIL:  Table access method "heapXXX" does not exist.
>
> Without attached patch command "RESET default_table_access_method;"
> returns no error.
> ----
> Probably no reason to create new tap-test for this case...

Yes, I think we need regression tests at least for
transaction_isolation since it leads to an assertion failure. And this
new test covers the change that the patch made.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Masahiko Sawada <sawada.mshk@gmail.com> writes:
> On Mon, Feb 7, 2022 at 6:21 AM Dmitry Koval <d.koval@postgrespro.ru> wrote:
>  +                   if (source != PGC_S_DEFAULT)
>  +                   {
>  +                       /* Release newextra as we use reset_extra */
>  +                       if (newextra)
>  +                           free(newextra);
>  +
>  +                       newextra = conf->reset_extra;
>  +                       source = conf->gen.reset_source;
>  +                       context = conf->gen.reset_scontext;
>  +                   }

> I think it's better to check if "!extra_field_used(&conf->gen,
> newextra)" before freeing newextra because otherwise, it's possible
> that we free reset_extra. I've updated an updated patch, please check
> it.

I feel this is still pretty confused about what to do with reset_extra.
If we go this way, there's very little reason to have reset_extra at all,
so maybe we should get rid of it and save the associated bookkeeping
logic.  AFAICS the only remaining place that would be using reset_extra
is ResetAllOptions(), which could also be made to compute the new extra
value using the check_hook instead of relying on having it already.

But the mention of ResetAllOptions brings up a bigger intellectual
inconsistency: why hasn't the patch touched that already?  Surely,
resetting a variable via RESET ALL is just as much of a risk as
resetting it explicitly.

Now, if you try to break it by doing "BEGIN set-some-xact-property;
SELECT 1; RESET ALL", there's no crash.  That's because the transaction
state GUCs are marked GUC_NO_RESET_ALL, so that ResetAllOptions doesn't
actually do anything to them.  But that makes me wonder if we need to
reconsider the relationship of that flag to what we're doing here.
It seems like a GUC might have an old value that we couldn't necessarily
RESET to, without also wanting to exempt it from RESET ALL.  However,
if it isn't exempt, yet the check_hook fails, what shall we do then?
Is throwing an error the best thing, or should we silently leave the
variable alone?  I think a lot of people would be unhappy if RESET ALL
/ DISCARD ALL had failure conditions of this sort.  Should we document
that GUCs having state-dependent restrictions on their values had
better be marked GUC_NO_RESET_ALL?  If so, can we enforce that?

So it seems like we still have some definitional issues to sort out.

            regards, tom lane

Hmm, here's a related anomaly:

regression=# BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
BEGIN
regression=*# savepoint foo;
SAVEPOINT
regression=*# RESET transaction_isolation;
RESET
regression=*# select 1;
 ?column? 
----------
        1
(1 row)

regression=*# show transaction_isolation;
 transaction_isolation 
-----------------------
 read committed
(1 row)

regression=*# rollback to foo;
ROLLBACK
regression=*# show transaction_isolation;
 transaction_isolation 
-----------------------
 serializable
(1 row)

regression=*# commit;
COMMIT

I'm not sure why that didn't fail, but it seems like it should've:
the commit-time isolation level is different from what we were
using when we took the first snapshot.  (Maybe we discard the
snapshot state when rolling back?  Not sure.)

If we need to be sure that it's always okay to roll back a
(sub)transaction, then that's an additional constraint on what's
valid for GUCs to do.  Yet it'd be a really bad idea to run
check_hooks during transaction rollback, so maybe there's little
choice.

            regards, tom lane

On Mon, Feb 14, 2022 at 6:50 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > On Mon, Feb 7, 2022 at 6:21 AM Dmitry Koval <d.koval@postgrespro.ru> wrote:
> >  +                   if (source != PGC_S_DEFAULT)
> >  +                   {
> >  +                       /* Release newextra as we use reset_extra */
> >  +                       if (newextra)
> >  +                           free(newextra);
> >  +
> >  +                       newextra = conf->reset_extra;
> >  +                       source = conf->gen.reset_source;
> >  +                       context = conf->gen.reset_scontext;
> >  +                   }
>
> > I think it's better to check if "!extra_field_used(&conf->gen,
> > newextra)" before freeing newextra because otherwise, it's possible
> > that we free reset_extra. I've updated an updated patch, please check
> > it.
>
> I feel this is still pretty confused about what to do with reset_extra.
> If we go this way, there's very little reason to have reset_extra at all,
> so maybe we should get rid of it and save the associated bookkeeping
> logic.  AFAICS the only remaining place that would be using reset_extra
> is ResetAllOptions(), which could also be made to compute the new extra
> value using the check_hook instead of relying on having it already.
>
> But the mention of ResetAllOptions brings up a bigger intellectual
> inconsistency: why hasn't the patch touched that already?  Surely,
> resetting a variable via RESET ALL is just as much of a risk as
> resetting it explicitly.

Good point.

>
> Now, if you try to break it by doing "BEGIN set-some-xact-property;
> SELECT 1; RESET ALL", there's no crash.  That's because the transaction
> state GUCs are marked GUC_NO_RESET_ALL, so that ResetAllOptions doesn't
> actually do anything to them.  But that makes me wonder if we need to
> reconsider the relationship of that flag to what we're doing here.
> It seems like a GUC might have an old value that we couldn't necessarily
> RESET to, without also wanting to exempt it from RESET ALL.  However,
> if it isn't exempt, yet the check_hook fails, what shall we do then?
> Is throwing an error the best thing, or should we silently leave the
> variable alone?  I think a lot of people would be unhappy if RESET ALL
> / DISCARD ALL had failure conditions of this sort.  Should we document
> that GUCs having state-dependent restrictions on their values had
> better be marked GUC_NO_RESET_ALL?  If so, can we enforce that?

Why do RESET ALL and RESET work differently with respect to resetting
one GUC in the first place? IOW why GUC_NO_RESET_ALL works only in
RESET ALL? It seems to me that RESET ALL is a command to do RESET
every single GUC, so if a GUC is exempt from RESET ALL it should be
exempt also from RESET.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Mon, Feb 14, 2022 at 7:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Hmm, here's a related anomaly:
>
> regression=# BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> BEGIN
> regression=*# savepoint foo;
> SAVEPOINT
> regression=*# RESET transaction_isolation;
> RESET
> regression=*# select 1;
>  ?column?
> ----------
>         1
> (1 row)
>
> regression=*# show transaction_isolation;
>  transaction_isolation
> -----------------------
>  read committed
> (1 row)
>
> regression=*# rollback to foo;
> ROLLBACK
> regression=*# show transaction_isolation;
>  transaction_isolation
> -----------------------
>  serializable
> (1 row)
>
> regression=*# commit;
> COMMIT
>
> I'm not sure why that didn't fail, but it seems like it should've:
> the commit-time isolation level is different from what we were
> using when we took the first snapshot.  (Maybe we discard the
> snapshot state when rolling back?  Not sure.)
>
> If we need to be sure that it's always okay to roll back a
> (sub)transaction, then that's an additional constraint on what's
> valid for GUCs to do.  Yet it'd be a really bad idea to run
> check_hooks during transaction rollback, so maybe there's little
> choice.

In this case, I think that "RESET transaction_isolation" should not be
allowed since we're already in a subtransaction. This is a result of
the fact that we bypass check_XactIsoLevel() in RESET.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Masahiko Sawada <sawada.mshk@gmail.com> writes:
> On Mon, Feb 14, 2022 at 6:50 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It seems like a GUC might have an old value that we couldn't necessarily
>> RESET to, without also wanting to exempt it from RESET ALL.  However,
>> if it isn't exempt, yet the check_hook fails, what shall we do then?
>> Is throwing an error the best thing, or should we silently leave the
>> variable alone?  I think a lot of people would be unhappy if RESET ALL
>> / DISCARD ALL had failure conditions of this sort.  Should we document
>> that GUCs having state-dependent restrictions on their values had
>> better be marked GUC_NO_RESET_ALL?  If so, can we enforce that?

> Why do RESET ALL and RESET work differently with respect to resetting
> one GUC in the first place? IOW why GUC_NO_RESET_ALL works only in
> RESET ALL? It seems to me that RESET ALL is a command to do RESET
> every single GUC, so if a GUC is exempt from RESET ALL it should be
> exempt also from RESET.

I toyed with that idea for awhile too, but looking through the current
set of GUC_NO_RESET_ALL variables dissuaded me from it.  The
transaction-property GUCs need this behavior or something like it,
but the rest don't.  In particular, I fear that turning RESET ROLE
into a no-op would create security bugs for applications that
expect the current behavior.

Having said that, it does seem like GUC_NO_RESET_ALL is pretty
intellectually-inconsistent by definition.  I'm just not sure that
we can redefine our way out of that at this late date.

            regards, tom lane

On Mon, Feb 14, 2022 at 2:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > On Mon, Feb 14, 2022 at 6:50 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> It seems like a GUC might have an old value that we couldn't necessarily
> >> RESET to, without also wanting to exempt it from RESET ALL.  However,
> >> if it isn't exempt, yet the check_hook fails, what shall we do then?
> >> Is throwing an error the best thing, or should we silently leave the
> >> variable alone?  I think a lot of people would be unhappy if RESET ALL
> >> / DISCARD ALL had failure conditions of this sort.  Should we document
> >> that GUCs having state-dependent restrictions on their values had
> >> better be marked GUC_NO_RESET_ALL?  If so, can we enforce that?
>
> > Why do RESET ALL and RESET work differently with respect to resetting
> > one GUC in the first place? IOW why GUC_NO_RESET_ALL works only in
> > RESET ALL? It seems to me that RESET ALL is a command to do RESET
> > every single GUC, so if a GUC is exempt from RESET ALL it should be
> > exempt also from RESET.
>
> I toyed with that idea for awhile too, but looking through the current
> set of GUC_NO_RESET_ALL variables dissuaded me from it.  The
> transaction-property GUCs need this behavior or something like it,
> but the rest don't.  In particular, I fear that turning RESET ROLE
> into a no-op would create security bugs for applications that
> expect the current behavior.

Right.

It seems that we need another flag or a dedicated treatment for
transaction property GUCs. It effectively cannot change them within
the transaction regardless of SET, RESET, and RESET ALL, so I think we
can make it no-op (possibly with a notice).

> Having said that, it does seem like GUC_NO_RESET_ALL is pretty
> intellectually-inconsistent by definition.  I'm just not sure that
> we can redefine our way out of that at this late date.

Yeah, it would get into areas too deep to tackle for v15. And given
many application relies on this behavior for a long time (it seems
GUC_NO_RESET_ALL has introduced about 20 years ago, see f0811a74b), we
might not have a big win by redefining it.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Masahiko Sawada <sawada.mshk@gmail.com> writes:
> It seems that we need another flag or a dedicated treatment for
> transaction property GUCs. It effectively cannot change them within
> the transaction regardless of SET, RESET, and RESET ALL, so I think we
> can make it no-op (possibly with a notice).

Yeah, I was considering that too.  A new GUC_NO_RESET flag would be
cheaper than running the check hooks during RESET, and probably
safer too.  On the other hand, we would lose the property that
you can reset these settings as long as you've not yet taken a
snapshot.  I wonder whether there is any code out there that
depends on that ...

            regards, tom lane

On Wed, Feb 16, 2022 at 1:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > It seems that we need another flag or a dedicated treatment for
> > transaction property GUCs. It effectively cannot change them within
> > the transaction regardless of SET, RESET, and RESET ALL, so I think we
> > can make it no-op (possibly with a notice).
>
> Yeah, I was considering that too.  A new GUC_NO_RESET flag would be
> cheaper than running the check hooks during RESET, and probably
> safer too.  On the other hand, we would lose the property that
> you can reset these settings as long as you've not yet taken a
> snapshot.  I wonder whether there is any code out there that
> depends on that ...

Indeed. I guess that it's relatively common that the transaction
isolation level is set after BEGIN TRANSACTION but I've not heard that
it's reset after BEGIN TRANSACTION with setting non-default
transaction isolation level.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Masahiko Sawada <sawada.mshk@gmail.com> writes:
> On Wed, Feb 16, 2022 at 1:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah, I was considering that too.  A new GUC_NO_RESET flag would be
>> cheaper than running the check hooks during RESET, and probably
>> safer too.  On the other hand, we would lose the property that
>> you can reset these settings as long as you've not yet taken a
>> snapshot.  I wonder whether there is any code out there that
>> depends on that ...

> Indeed. I guess that it's relatively common that the transaction
> isolation level is set after BEGIN TRANSACTION but I've not heard that
> it's reset after BEGIN TRANSACTION with setting non-default
> transaction isolation level.

Yes, we certainly have to preserve the SET case, but using RESET
in that way seems like it'd be pretty weird coding.  I'd have no
hesitation about banning it in a HEAD-only change.  I'm slightly
more nervous about doing so in a back-patched bug fix.  On the
other hand, starting to call check hooks in a context that did
not use them before is also scary to back-patch.

Do you want to draft up a patch that fixes this with a new
GUC flag?

            regards, tom lane

On Thu, Mar 10, 2022 at 12:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > On Wed, Feb 16, 2022 at 1:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Yeah, I was considering that too.  A new GUC_NO_RESET flag would be
> >> cheaper than running the check hooks during RESET, and probably
> >> safer too.  On the other hand, we would lose the property that
> >> you can reset these settings as long as you've not yet taken a
> >> snapshot.  I wonder whether there is any code out there that
> >> depends on that ...
>
> > Indeed. I guess that it's relatively common that the transaction
> > isolation level is set after BEGIN TRANSACTION but I've not heard that
> > it's reset after BEGIN TRANSACTION with setting non-default
> > transaction isolation level.
>
> Yes, we certainly have to preserve the SET case, but using RESET
> in that way seems like it'd be pretty weird coding.  I'd have no
> hesitation about banning it in a HEAD-only change.  I'm slightly
> more nervous about doing so in a back-patched bug fix.  On the
> other hand, starting to call check hooks in a context that did
> not use them before is also scary to back-patch.

Agreed.

For back branches, it might be less scary to call check hooks for only
limited GUCs such as transaction_isolation.

> Do you want to draft up a patch that fixes this with a new
> GUC flag?

Yes, I'll update the patch accordingly.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Thu, Mar 10, 2022 at 8:36 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Mar 10, 2022 at 12:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > > On Wed, Feb 16, 2022 at 1:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> Yeah, I was considering that too.  A new GUC_NO_RESET flag would be
> > >> cheaper than running the check hooks during RESET, and probably
> > >> safer too.  On the other hand, we would lose the property that
> > >> you can reset these settings as long as you've not yet taken a
> > >> snapshot.  I wonder whether there is any code out there that
> > >> depends on that ...
> >
> > > Indeed. I guess that it's relatively common that the transaction
> > > isolation level is set after BEGIN TRANSACTION but I've not heard that
> > > it's reset after BEGIN TRANSACTION with setting non-default
> > > transaction isolation level.
> >
> > Yes, we certainly have to preserve the SET case, but using RESET
> > in that way seems like it'd be pretty weird coding.  I'd have no
> > hesitation about banning it in a HEAD-only change.  I'm slightly
> > more nervous about doing so in a back-patched bug fix.  On the
> > other hand, starting to call check hooks in a context that did
> > not use them before is also scary to back-patch.
>
> Agreed.
>
> For back branches, it might be less scary to call check hooks for only
> limited GUCs such as transaction_isolation.
>
> > Do you want to draft up a patch that fixes this with a new
> > GUC flag?
>
> Yes, I'll update the patch accordingly.

I've attached a draft patch for discussion.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Masahiko Sawada <sawada.mshk@gmail.com> writes:
> I've attached a draft patch for discussion.

Hm, I think that trying to RESET a GUC_NO_RESET variable ought to
actively throw an error.  Silently doing nothing will look like
a bug.

(This does imply that it's not sensible to mark a variable
GUC_NO_RESET without also saying GUC_NO_RESET_ALL.  That
seems fine to me, because I'm not sure what the combination
GUC_NO_RESET & !GUC_NO_RESET_ALL ought to mean.)

            regards, tom lane

On Sat, Mar 12, 2022 at 4:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > I've attached a draft patch for discussion.
>
> Hm, I think that trying to RESET a GUC_NO_RESET variable ought to
> actively throw an error.  Silently doing nothing will look like
> a bug.

Agreed. I've attached an updated patch.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

At Mon, 14 Mar 2022 17:12:18 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in 
> On Sat, Mar 12, 2022 at 4:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > > I've attached a draft patch for discussion.
> >
> > Hm, I think that trying to RESET a GUC_NO_RESET variable ought to
> > actively throw an error.  Silently doing nothing will look like
> > a bug.
> 
> Agreed. I've attached an updated patch.

FWIW, 

+    /* Check if the parameter supports RESET */
+    if (record->flags & GUC_NO_RESET && value == NULL)
+    {
+        ereport(elevel,
+                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                 errmsg("parameter \"%s\" cannot be reset",
+                        name)));

ERRCODE_FEATURE_NOT_SUPPORTED doesn't look proper for the case. Isn't
it ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE or something like?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> ERRCODE_FEATURE_NOT_SUPPORTED doesn't look proper for the case. Isn't
> it ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE or something like?

Mmm ... I guess you could think of it that way, but it seems a
little weird, because you have to suppose that the *transaction*
not the GUC itself is the object that is in the wrong state.
We could use ERRCODE_ACTIVE_SQL_TRANSACTION as is done in
check_XactIsoLevel et al.  But this code is supposed to be generic,
and if there are ever any other GUCs marked NO_RESET, who's to say
if that would be appropriate at all for them?

I'm OK with FEATURE_NOT_SUPPORTED.

            regards, tom lane

After thinking about this some more, it seems like there is a related
problem with GUC save/restore actions.  Consider

regression=# create function foo() returns int language sql as 'select 1'
regression-# set transaction_read_only = 1;
CREATE FUNCTION
regression=# begin;
BEGIN
regression=*# select foo();
 foo 
-----
   1
(1 row)

regression=*# show transaction_read_only;
 transaction_read_only 
-----------------------
 off
(1 row)

transaction_read_only was set while we executed foo(), but now it's
off again.  I've not tried to weaponize this behavior, but if we
have any optimizations that depend on transaction_read_only, this
would probably break them.  (SERIALIZABLE mode looks like a likely
candidate for problems.)

So it seems like we also need to forbid save/restore for these
settings, which probably means rejecting action==GUC_ACTION_SAVE
as well as value==NULL.  That makes NO_RESET something of a misnomer,
but I don't have an idea for a better name.

            regards, tom lane

At Mon, 14 Mar 2022 09:45:17 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > ERRCODE_FEATURE_NOT_SUPPORTED doesn't look proper for the case. Isn't
> > it ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE or something like?
> 
> Mmm ... I guess you could think of it that way, but it seems a
> little weird, because you have to suppose that the *transaction*
> not the GUC itself is the object that is in the wrong state.
> We could use ERRCODE_ACTIVE_SQL_TRANSACTION as is done in
> check_XactIsoLevel et al.  But this code is supposed to be generic,
> and if there are ever any other GUCs marked NO_RESET, who's to say
> if that would be appropriate at all for them?
> 
> I'm OK with FEATURE_NOT_SUPPORTED.

Hmm. Actually ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE is somewhat
strange since the error is not caused by some state of the object.  I
thought at the time that some error code like
ERRCODE_DOESNT_FOR_FOR_THIS_OBJECT but, yeah, finally
FEATURE_NOT_SUPPORTED looks fine after some additional thought.

Thanks.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Mar 15, 2022 at 3:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> After thinking about this some more, it seems like there is a related
> problem with GUC save/restore actions.  Consider
>
> regression=# create function foo() returns int language sql as 'select 1'
> regression-# set transaction_read_only = 1;
> CREATE FUNCTION
> regression=# begin;
> BEGIN
> regression=*# select foo();
>  foo
> -----
>    1
> (1 row)
>
> regression=*# show transaction_read_only;
>  transaction_read_only
> -----------------------
>  off
> (1 row)

Good catch.

>
> transaction_read_only was set while we executed foo(), but now it's
> off again.  I've not tried to weaponize this behavior, but if we
> have any optimizations that depend on transaction_read_only, this
> would probably break them.  (SERIALIZABLE mode looks like a likely
> candidate for problems.)
>
> So it seems like we also need to forbid save/restore for these
> settings, which probably means rejecting action==GUC_ACTION_SAVE
> as well as value==NULL.  That makes NO_RESET something of a misnomer,
> but I don't have an idea for a better name.

Yes, it seems we need that change too. I'll update the patch.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Wed, Mar 23, 2022 at 5:48 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Mar 15, 2022 at 3:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > After thinking about this some more, it seems like there is a related
> > problem with GUC save/restore actions.  Consider
> >
> > regression=# create function foo() returns int language sql as 'select 1'
> > regression-# set transaction_read_only = 1;
> > CREATE FUNCTION
> > regression=# begin;
> > BEGIN
> > regression=*# select foo();
> >  foo
> > -----
> >    1
> > (1 row)
> >
> > regression=*# show transaction_read_only;
> >  transaction_read_only
> > -----------------------
> >  off
> > (1 row)
>
> Good catch.
>
> >
> > transaction_read_only was set while we executed foo(), but now it's
> > off again.  I've not tried to weaponize this behavior, but if we
> > have any optimizations that depend on transaction_read_only, this
> > would probably break them.  (SERIALIZABLE mode looks like a likely
> > candidate for problems.)
> >
> > So it seems like we also need to forbid save/restore for these
> > settings, which probably means rejecting action==GUC_ACTION_SAVE
> > as well as value==NULL.  That makes NO_RESET something of a misnomer,
> > but I don't have an idea for a better name.
>
> Yes, it seems we need that change too. I'll update the patch.

Attached an updated patch. I kept the name GUC_NO_RESET but I'll
change it if we find a better name for it.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Fri, Mar 11, 2022 at 02:53:12PM -0500, Tom Lane wrote:
> (This does imply that it's not sensible to mark a variable
> GUC_NO_RESET without also saying GUC_NO_RESET_ALL.  That
> seems fine to me, because I'm not sure what the combination
> GUC_NO_RESET & !GUC_NO_RESET_ALL ought to mean.)

On Thu, Mar 24, 2022 at 11:23:57PM +0900, Masahiko Sawada wrote:
> Attached an updated patch. I kept the name GUC_NO_RESET but I'll
> change it if we find a better name for it.

I think guc.sql should check that NO_RESET implies NO_RESET_ALL, or otherwise
guc.c could incorporate that logic by checking (NO_RESET | NO_RESET_ALL)

-- 
Justin

On Tue, Jun 28, 2022 at 9:37 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Fri, Mar 11, 2022 at 02:53:12PM -0500, Tom Lane wrote:
> > (This does imply that it's not sensible to mark a variable
> > GUC_NO_RESET without also saying GUC_NO_RESET_ALL.  That
> > seems fine to me, because I'm not sure what the combination
> > GUC_NO_RESET & !GUC_NO_RESET_ALL ought to mean.)
>
> On Thu, Mar 24, 2022 at 11:23:57PM +0900, Masahiko Sawada wrote:
> > Attached an updated patch. I kept the name GUC_NO_RESET but I'll
> > change it if we find a better name for it.
>
> I think guc.sql should check that NO_RESET implies NO_RESET_ALL, or otherwise
> guc.c could incorporate that logic by checking (NO_RESET | NO_RESET_ALL)

Agreed. I've attached an updated patch.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Tue, Jun 28, 2022 at 03:58:41PM +0900, Masahiko Sawada wrote:
> Agreed. I've attached an updated patch.

I can see that this has been registered in the CF app.  I'll try to
glimpse through that.
--
Michael

On Tue, Jun 28, 2022 at 03:58:41PM +0900, Masahiko Sawada wrote:
> Agreed. I've attached an updated patch.

+#define GUC_NO_RESET         0x400000  /* not support RESET and save */

It is a bit sad to see this new flag with this number, separated from
its cousin properties.  Could it be better to reorganize the flag
values and give more room to the properties?  The units for memory and
time could go first, for example.

+CREATE FUNCTION errfunc() RETURNS int LANGUAGE SQL AS 'SELECT 1'
+SET transaction_read_only = on; -- error
+ERROR:  parameter "transaction_read_only" cannot be reset
Well, this is confusing when setting a GUC_NO_RESET in the context of
GUC_ACTION_SAVE.

By the way, what about "seed"?
--
Michael

On Wed, Jun 29, 2022 at 3:48 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jun 28, 2022 at 03:58:41PM +0900, Masahiko Sawada wrote:
> > Agreed. I've attached an updated patch.
>
> +#define GUC_NO_RESET         0x400000  /* not support RESET and save */
>
> It is a bit sad to see this new flag with this number, separated from
> its cousin properties.  Could it be better to reorganize the flag
> values and give more room to the properties?  The units for memory and
> time could go first, for example.

It seems a good idea, I'll update it in the next version patch.

>
> +CREATE FUNCTION errfunc() RETURNS int LANGUAGE SQL AS 'SELECT 1'
> +SET transaction_read_only = on; -- error
> +ERROR:  parameter "transaction_read_only" cannot be reset
> Well, this is confusing when setting a GUC_NO_RESET in the context of
> GUC_ACTION_SAVE.

Right, how about "parameter %s cannot be set"?

> By the way, what about "seed"?

Resetting seed is no-op so it might be better to throw an error when
resetting the seed rather than doing nothing silently.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Fri, Jul 1, 2022 at 2:11 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Jun 29, 2022 at 3:48 PM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Tue, Jun 28, 2022 at 03:58:41PM +0900, Masahiko Sawada wrote:
> > > Agreed. I've attached an updated patch.
> >
> > +#define GUC_NO_RESET         0x400000  /* not support RESET and save */
> >
> > It is a bit sad to see this new flag with this number, separated from
> > its cousin properties.  Could it be better to reorganize the flag
> > values and give more room to the properties?  The units for memory and
> > time could go first, for example.
>
> It seems a good idea, I'll update it in the next version patch.
>
> >
> > +CREATE FUNCTION errfunc() RETURNS int LANGUAGE SQL AS 'SELECT 1'
> > +SET transaction_read_only = on; -- error
> > +ERROR:  parameter "transaction_read_only" cannot be reset
> > Well, this is confusing when setting a GUC_NO_RESET in the context of
> > GUC_ACTION_SAVE.
>
> Right, how about "parameter %s cannot be set"?
>
> > By the way, what about "seed"?
>
> Resetting seed is no-op so it might be better to throw an error when
> resetting the seed rather than doing nothing silently.

I've attached patches.

I did the reorganization of GUC flags in a separate patch (0002 patch)
since it's not relevant with the new flag GUC_NO_RESET.

Regards,

--
Masahiko Sawada
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Masahiko Sawada <sawada.mshk@gmail.com> writes:
> I've attached patches.

Per the cfbot, this incorrectly updates the plpgsql_transaction
test.  Here's an updated version that fixes that.

The proposed error message for the GUC_ACTION_SAVE case,

    errmsg("parameter \"%s\" cannot be set temporarily or in functions locally",

does not seem like very good English to me.  In the attached I changed
it to

    errmsg("parameter \"%s\" cannot be set temporarily or locally in functions",

but that isn't great either: it seems redundant and confusing.
I think we could just make it

    errmsg("parameter \"%s\" cannot be set locally in functions",

because that's really the only case users should ever see.

Other than the message-wording issue, I think v6-0001 is OK.

> I did the reorganization of GUC flags in a separate patch (0002 patch)
> since it's not relevant with the new flag GUC_NO_RESET.

0002 seems quite invasive and hard to review compared to what it
accomplishes.  I think we should just keep the flags in the same
order, stick in GUC_NO_RESET in front of GUC_NO_RESET_ALL, and
renumber the flag values as needed.  I did not change 0002 below,
though.

            regards, tom lane

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e1fe4fec57..546213fa93 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24353,6 +24353,12 @@ SELECT collation for ('foo' COLLATE "de_DE");
        <command>SHOW ALL</command> commands.
       </entry>
      </row>
+     <row>
+      <entry><literal>NO_RESET</literal></entry>
+      <entry>Parameters with this flag do not support
+      <command>RESET</command> commands.
+      </entry>
+     </row>
      <row>
       <entry><literal>NO_RESET_ALL</literal></entry>
       <entry>Parameters with this flag are excluded from
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8c2e08fad6..29ec6d886c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3243,6 +3243,26 @@ set_config_option_ext(const char *name, const char *value,
         }
     }

+    /* Disallow resetting and saving GUC_NO_RESET values */
+    if (record->flags & GUC_NO_RESET)
+    {
+        if (value == NULL)
+        {
+            ereport(elevel,
+                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                     errmsg("parameter \"%s\" cannot be reset", name)));
+            return 0;
+        }
+        if (action == GUC_ACTION_SAVE)
+        {
+            ereport(elevel,
+                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                     errmsg("parameter \"%s\" cannot be set temporarily or locally in functions",
+                            name)));
+            return 0;
+        }
+    }
+
     /*
      * Should we set reset/stacked values?    (If so, the behavior is not
      * transactional.)    This is done either when we get a default value from
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index 3d2df18659..ffc71726f9 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -141,9 +141,6 @@ ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
                 WarnNoTransactionBlock(isTopLevel, "SET LOCAL");
             /* fall through */
         case VAR_RESET:
-            if (strcmp(stmt->name, "transaction_isolation") == 0)
-                WarnNoTransactionBlock(isTopLevel, "RESET TRANSACTION");
-
             (void) set_config_option(stmt->name,
                                      NULL,
                                      (superuser() ? PGC_SUSET : PGC_USERSET),
@@ -539,7 +536,7 @@ ShowAllGUCConfig(DestReceiver *dest)
 Datum
 pg_settings_get_flags(PG_FUNCTION_ARGS)
 {
-#define MAX_GUC_FLAGS    5
+#define MAX_GUC_FLAGS    6
     char       *varname = TextDatumGetCString(PG_GETARG_DATUM(0));
     struct config_generic *record;
     int            cnt = 0;
@@ -554,6 +551,8 @@ pg_settings_get_flags(PG_FUNCTION_ARGS)

     if (record->flags & GUC_EXPLAIN)
         flags[cnt++] = CStringGetTextDatum("EXPLAIN");
+    if (record->flags & GUC_NO_RESET)
+        flags[cnt++] = CStringGetTextDatum("NO_RESET");
     if (record->flags & GUC_NO_RESET_ALL)
         flags[cnt++] = CStringGetTextDatum("NO_RESET_ALL");
     if (record->flags & GUC_NO_SHOW_ALL)
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index ab3140ff3a..fda3f9befb 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -1505,7 +1505,7 @@ struct config_bool ConfigureNamesBool[] =
         {"transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT,
             gettext_noop("Sets the current transaction's read-only status."),
             NULL,
-            GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+            GUC_NO_RESET | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
         },
         &XactReadOnly,
         false,
@@ -1524,7 +1524,7 @@ struct config_bool ConfigureNamesBool[] =
         {"transaction_deferrable", PGC_USERSET, CLIENT_CONN_STATEMENT,
             gettext_noop("Whether to defer a read-only serializable transaction until it can be executed with no
possibleserialization failures."), 
             NULL,
-            GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+            GUC_NO_RESET | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
         },
         &XactDeferrable,
         false,
@@ -3606,7 +3606,7 @@ struct config_real ConfigureNamesReal[] =
         {"seed", PGC_USERSET, UNGROUPED,
             gettext_noop("Sets the seed for random-number generation."),
             NULL,
-            GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+            GUC_NO_SHOW_ALL | GUC_NO_RESET | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
         },
         &phony_random_seed,
         0.0, -1.0, 1.0,
@@ -4557,7 +4557,7 @@ struct config_enum ConfigureNamesEnum[] =
         {"transaction_isolation", PGC_USERSET, CLIENT_CONN_STATEMENT,
             gettext_noop("Sets the current transaction's isolation level."),
             NULL,
-            GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+            GUC_NO_RESET | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
         },
         &XactIsoLevel,
         XACT_READ_COMMITTED, isolation_level_options,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e426dd757d..a846500bc5 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -238,6 +238,8 @@ typedef enum
  */
 #define GUC_RUNTIME_COMPUTED  0x200000

+#define GUC_NO_RESET          0x400000    /* not support RESET and save */
+
 #define GUC_UNIT                (GUC_UNIT_MEMORY | GUC_UNIT_TIME)


diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
index 254e5b7a70..adff10fa6d 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -576,8 +576,7 @@ BEGIN
     PERFORM 1;
     RAISE INFO '%', current_setting('transaction_isolation');
     COMMIT;
-    SET TRANSACTION ISOLATION LEVEL REPEATABLE READ;
-    RESET TRANSACTION ISOLATION LEVEL;
+    SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
     PERFORM 1;
     RAISE INFO '%', current_setting('transaction_isolation');
     COMMIT;
@@ -585,7 +584,7 @@ END;
 $$;
 INFO:  read committed
 INFO:  repeatable read
-INFO:  read committed
+INFO:  serializable
 -- error cases
 DO LANGUAGE plpgsql $$
 BEGIN
diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
index 8d76d00daa..c73fca7e03 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
@@ -481,8 +481,7 @@ BEGIN
     PERFORM 1;
     RAISE INFO '%', current_setting('transaction_isolation');
     COMMIT;
-    SET TRANSACTION ISOLATION LEVEL REPEATABLE READ;
-    RESET TRANSACTION ISOLATION LEVEL;
+    SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
     PERFORM 1;
     RAISE INFO '%', current_setting('transaction_isolation');
     COMMIT;
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 3de6404ba5..2914b950b4 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -839,6 +839,7 @@ SELECT pg_settings_get_flags('does_not_exist');

 CREATE TABLE tab_settings_flags AS SELECT name, category,
     'EXPLAIN'          = ANY(flags) AS explain,
+    'NO_RESET'         = ANY(flags) AS no_reset,
     'NO_RESET_ALL'     = ANY(flags) AS no_reset_all,
     'NO_SHOW_ALL'      = ANY(flags) AS no_show_all,
     'NOT_IN_SAMPLE'    = ANY(flags) AS not_in_sample,
@@ -906,4 +907,12 @@ SELECT name FROM tab_settings_flags
 ------
 (0 rows)

+-- NO_RESET implies NO_RESET_ALL.
+SELECT name FROM tab_settings_flags
+  WHERE no_reset AND NOT no_reset_all
+  ORDER BY 1;
+ name
+------
+(0 rows)
+
 DROP TABLE tab_settings_flags;
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index a46fa5d48a..9351d1d134 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -44,6 +44,40 @@ SELECT * FROM xacttest;
  777 | 777.777
 (5 rows)

+-- Test that transaction characteristics cannot be reset.
+BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+SELECT COUNT(*) FROM xacttest;
+ count
+-------
+     5
+(1 row)
+
+RESET transaction_isolation; -- error
+ERROR:  parameter "transaction_isolation" cannot be reset
+END;
+BEGIN TRANSACTION READ ONLY;
+SELECT COUNT(*) FROM xacttest;
+ count
+-------
+     5
+(1 row)
+
+RESET transaction_read_only; -- error
+ERROR:  parameter "transaction_read_only" cannot be reset
+END;
+BEGIN TRANSACTION DEFERRABLE;
+SELECT COUNT(*) FROM xacttest;
+ count
+-------
+     5
+(1 row)
+
+RESET transaction_deferrable; -- error
+ERROR:  parameter "transaction_deferrable" cannot be reset
+END;
+CREATE FUNCTION errfunc() RETURNS int LANGUAGE SQL AS 'SELECT 1'
+SET transaction_read_only = on; -- error
+ERROR:  parameter "transaction_read_only" cannot be set temporarily or locally in functions
 -- Read-only tests
 CREATE TABLE writetest (a int);
 CREATE TEMPORARY TABLE temptest (a int);
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index d5db101e48..d40d0c178f 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -324,6 +324,7 @@ SELECT pg_settings_get_flags(NULL);
 SELECT pg_settings_get_flags('does_not_exist');
 CREATE TABLE tab_settings_flags AS SELECT name, category,
     'EXPLAIN'          = ANY(flags) AS explain,
+    'NO_RESET'         = ANY(flags) AS no_reset,
     'NO_RESET_ALL'     = ANY(flags) AS no_reset_all,
     'NO_SHOW_ALL'      = ANY(flags) AS no_show_all,
     'NOT_IN_SAMPLE'    = ANY(flags) AS not_in_sample,
@@ -360,4 +361,8 @@ SELECT name FROM tab_settings_flags
 SELECT name FROM tab_settings_flags
   WHERE no_show_all AND NOT not_in_sample
   ORDER BY 1;
+-- NO_RESET implies NO_RESET_ALL.
+SELECT name FROM tab_settings_flags
+  WHERE no_reset AND NOT no_reset_all
+  ORDER BY 1;
 DROP TABLE tab_settings_flags;
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index d71c3ce93e..7ee5f6aaa5 100644
--- a/src/test/regress/sql/transactions.sql
+++ b/src/test/regress/sql/transactions.sql
@@ -35,6 +35,24 @@ SELECT oid FROM pg_class WHERE relname = 'disappear';
 -- should have members again
 SELECT * FROM xacttest;

+-- Test that transaction characteristics cannot be reset.
+BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+SELECT COUNT(*) FROM xacttest;
+RESET transaction_isolation; -- error
+END;
+
+BEGIN TRANSACTION READ ONLY;
+SELECT COUNT(*) FROM xacttest;
+RESET transaction_read_only; -- error
+END;
+
+BEGIN TRANSACTION DEFERRABLE;
+SELECT COUNT(*) FROM xacttest;
+RESET transaction_deferrable; -- error
+END;
+
+CREATE FUNCTION errfunc() RETURNS int LANGUAGE SQL AS 'SELECT 1'
+SET transaction_read_only = on; -- error

 -- Read-only tests

From b207f4a2560a19435598d442a0ca3f5e43aba532 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Wed, 21 Sep 2022 21:07:20 +0900
Subject: [PATCH v5 2/2] Reorganize GUC_XXX flag values.

Previously, we defined GUC flags for units for memory and time
followed by flags for properties. This change makes more room for
properties by inverting the order.
---
 src/include/utils/guc.h | 56 ++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 29 deletions(-)

diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index a846500bc5..8daeb877ad 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -204,41 +204,39 @@ typedef enum
 /*
  * bit values in "flags" of a GUC variable
  */
-#define GUC_LIST_INPUT            0x0001    /* input can be list format */
-#define GUC_LIST_QUOTE            0x0002    /* double-quote list elements */
-#define GUC_NO_SHOW_ALL            0x0004    /* exclude from SHOW ALL */
-#define GUC_NO_RESET_ALL        0x0008    /* exclude from RESET ALL */
-#define GUC_REPORT                0x0010    /* auto-report changes to client */
-#define GUC_NOT_IN_SAMPLE        0x0020    /* not in postgresql.conf.sample */
-#define GUC_DISALLOW_IN_FILE    0x0040    /* can't set in postgresql.conf */
-#define GUC_CUSTOM_PLACEHOLDER    0x0080    /* placeholder for custom variable */
-#define GUC_SUPERUSER_ONLY        0x0100    /* show only to superusers */
-#define GUC_IS_NAME                0x0200    /* limit string to NAMEDATALEN-1 */
-#define GUC_NOT_WHILE_SEC_REST    0x0400    /* can't set if security restricted */
-#define GUC_DISALLOW_IN_AUTO_FILE 0x0800    /* can't set in
+#define GUC_UNIT_KB                0x000001    /* value is in kilobytes */
+#define GUC_UNIT_BLOCKS            0x000002    /* value is in blocks */
+#define GUC_UNIT_XBLOCKS        0x000003    /* value is in xlog blocks */
+#define GUC_UNIT_MB                0x000004    /* value is in megabytes */
+#define GUC_UNIT_BYTE            0x000005    /* value is in bytes */
+#define GUC_UNIT_MEMORY            0x00000F    /* mask for size-related units */
+
+#define GUC_UNIT_MS                0x000010    /* value is in milliseconds */
+#define GUC_UNIT_S                0x000020    /* value is in seconds */
+#define GUC_UNIT_MIN            0x000030    /* value is in minutes */
+#define GUC_UNIT_TIME            0x0000F0    /* mask for time-related units */
+
+#define GUC_LIST_INPUT            0x000100    /* input can be list format */
+#define GUC_LIST_QUOTE            0x000200    /* double-quote list elements */
+#define GUC_NO_SHOW_ALL            0x000400    /* exclude from SHOW ALL */
+#define GUC_NO_RESET            0x000800    /* not support RESET and save */
+#define GUC_NO_RESET_ALL        0x001000    /* exclude from RESET ALL */
+#define GUC_REPORT                0x002000    /* auto-report changes to client */
+#define GUC_NOT_IN_SAMPLE        0x004000    /* not in postgresql.conf.sample */
+#define GUC_DISALLOW_IN_FILE    0x008000    /* can't set in postgresql.conf */
+#define GUC_CUSTOM_PLACEHOLDER    0x010000    /* placeholder for custom variable */
+#define GUC_SUPERUSER_ONLY        0x020000    /* show only to superusers */
+#define GUC_IS_NAME                0x040000    /* limit string to NAMEDATALEN-1 */
+#define GUC_NOT_WHILE_SEC_REST    0x080000    /* can't set if security restricted */
+#define GUC_DISALLOW_IN_AUTO_FILE 0x100000    /* can't set in
                                              * PG_AUTOCONF_FILENAME */
-
-#define GUC_UNIT_KB                0x1000    /* value is in kilobytes */
-#define GUC_UNIT_BLOCKS            0x2000    /* value is in blocks */
-#define GUC_UNIT_XBLOCKS        0x3000    /* value is in xlog blocks */
-#define GUC_UNIT_MB                0x4000    /* value is in megabytes */
-#define GUC_UNIT_BYTE            0x5000    /* value is in bytes */
-#define GUC_UNIT_MEMORY            0xF000    /* mask for size-related units */
-
-#define GUC_UNIT_MS               0x10000    /* value is in milliseconds */
-#define GUC_UNIT_S               0x20000    /* value is in seconds */
-#define GUC_UNIT_MIN           0x30000    /* value is in minutes */
-#define GUC_UNIT_TIME           0xF0000    /* mask for time-related units */
-
-#define GUC_EXPLAIN              0x100000    /* include in explain */
+#define GUC_EXPLAIN                0x200000    /* include in explain */

 /*
  * GUC_RUNTIME_COMPUTED is intended for runtime-computed GUCs that are only
  * available via 'postgres -C' if the server is not running.
  */
-#define GUC_RUNTIME_COMPUTED  0x200000
-
-#define GUC_NO_RESET          0x400000    /* not support RESET and save */
+#define GUC_RUNTIME_COMPUTED    0x400000

 #define GUC_UNIT                (GUC_UNIT_MEMORY | GUC_UNIT_TIME)

--
2.31.1

On Sat, Sep 24, 2022 at 04:57:51PM -0400, Tom Lane wrote:
> but that isn't great either: it seems redundant and confusing.
> I think we could just make it
>
>     errmsg("parameter \"%s\" cannot be set locally in functions",
>
> because that's really the only case users should ever see.

Yes, agreed that "cannot be set locally in functions" should be
enough.

> Other than the message-wording issue, I think v6-0001 is OK.

Just to be sure about something here.  The change of behavior with SET
in the context of a PL makes this patch unsuitable for a backpatch,
hence the plan is to apply this stuff only on HEAD, right?

+      <entry><literal>NO_RESET</literal></entry>
+      <entry>Parameters with this flag do not support
+      <command>RESET</command> commands.
+      </entry>

As per the issue with SET commands used with functions, this
description does not completely reflect the reality.

> 0002 seems quite invasive and hard to review compared to what it
> accomplishes.  I think we should just keep the flags in the same
> order, stick in GUC_NO_RESET in front of GUC_NO_RESET_ALL, and
> renumber the flag values as needed.  I did not change 0002 below,
> though.

0002 is not that confusing to me: the units are moved to be first and
to use the first flag bytes, while the more conceptual flags are moved
to be always after.  I would have reorganized a bit more the
description flags, TBH.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> Just to be sure about something here.  The change of behavior with SET
> in the context of a PL makes this patch unsuitable for a backpatch,
> hence the plan is to apply this stuff only on HEAD, right? 

Yeah, I think we concluded that back-patching might cause more
trouble than it's worth.

> +      <entry><literal>NO_RESET</literal></entry>
> +      <entry>Parameters with this flag do not support
> +      <command>RESET</command> commands.
> +      </entry>

> As per the issue with SET commands used with functions, this
> description does not completely reflect the reality.

It seems adequate enough to me ... do you have a suggestion?

            regards, tom lane

[ hit send too soon ... ]

Michael Paquier <michael@paquier.xyz> writes:
> On Sat, Sep 24, 2022 at 04:57:51PM -0400, Tom Lane wrote:
>> 0002 seems quite invasive and hard to review compared to what it
>> accomplishes.

> 0002 is not that confusing to me: the units are moved to be first and
> to use the first flag bytes, while the more conceptual flags are moved
> to be always after.

Yeah, but why?  I see no good reason why those fields need to be first.

> I would have reorganized a bit more the
> description flags, TBH.

Looking at it closer, I agree that GUC_EXPLAIN and GUC_RUNTIME_COMPUTED
should be moved to be with the other non-units flags.  But I don't
see why we need to re-order the entries more than that.  I'm concerned
for one thing that the order of the entries in this list stay comparable
to the order in which the flags are dealt with in other code, such as
pg_settings_get_flags or the guc_tables.c entries.

            regards, tom lane

On Mon, Sep 26, 2022 at 12:16:24AM -0400, Tom Lane wrote:
> Yeah, but why?  I see no good reason why those fields need to be first.

My reasoning on these ones is that we are most likely going to add
more description flags in the future than new unit types.  Perhaps I
am wrong.

> Looking at it closer, I agree that GUC_EXPLAIN and GUC_RUNTIME_COMPUTED
> should be moved to be with the other non-units flags.  But I don't
> see why we need to re-order the entries more than that.  I'm concerned
> for one thing that the order of the entries in this list stay comparable
> to the order in which the flags are dealt with in other code, such as
> pg_settings_get_flags or the guc_tables.c entries.

Yes, that's a minimal move.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Sep 26, 2022 at 12:16:24AM -0400, Tom Lane wrote:
>> Yeah, but why?  I see no good reason why those fields need to be first.

> My reasoning on these ones is that we are most likely going to add
> more description flags in the future than new unit types.  Perhaps I
> am wrong.

Sure, but we could easily leave unused bits there.  Aligning the
units subfields on byte boundaries might result in slightly better
machine code, anyway.

            regards, tom lane

On Sun, Sep 25, 2022 at 5:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > I've attached patches.
>
> Per the cfbot, this incorrectly updates the plpgsql_transaction
> test.  Here's an updated version that fixes that.
>
> The proposed error message for the GUC_ACTION_SAVE case,
>
>     errmsg("parameter \"%s\" cannot be set temporarily or in functions locally",
>
> does not seem like very good English to me.  In the attached I changed
> it to
>
>     errmsg("parameter \"%s\" cannot be set temporarily or locally in functions",
>
> but that isn't great either: it seems redundant and confusing.
> I think we could just make it
>
>     errmsg("parameter \"%s\" cannot be set locally in functions",
>
> because that's really the only case users should ever see.
>
> Other than the message-wording issue, I think v6-0001 is OK.

Thank you for updating the patch. I've attached an updated 0001 patch.

Regards,

-- 
Masahiko Sawada
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, Sep 26, 2022 at 12:07:50AM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> +      <entry><literal>NO_RESET</literal></entry>
>> +      <entry>Parameters with this flag do not support
>> +      <command>RESET</command> commands.
>> +      </entry>
>
> > As per the issue with SET commands used with functions, this
> > description does not completely reflect the reality.
>
> It seems adequate enough to me ... do you have a suggestion?

As we are talking about a description with GUC_ACTION_SAVE, something
like "Parameters with this flag do not support RESET, or SET in the
context of a function call"?  NO_RESET sounds a bit confusing as a
name if you consider this second part (it can be understood as
resetting the value as well), but keeping it as-is does not look like
a big deal to me with this description, or an equivalent, in place.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> As we are talking about a description with GUC_ACTION_SAVE, something
> like "Parameters with this flag do not support RESET, or SET in the
> context of a function call"?  NO_RESET sounds a bit confusing as a
> name if you consider this second part (it can be understood as
> resetting the value as well), but keeping it as-is does not look like
> a big deal to me with this description, or an equivalent, in place.

Yeah, we already talked upthread about how the SAVE restriction makes
"NO_RESET" a bit of a misnomer.  Nobody proposed a better name though.

            regards, tom lane

Masahiko Sawada <sawada.mshk@gmail.com> writes:
> Thank you for updating the patch. I've attached an updated 0001 patch.

Pushed, thanks.

            regards, tom lane

On Wed, Sep 28, 2022 at 12:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > Thank you for updating the patch. I've attached an updated 0001 patch.
>
> Pushed, thanks.

Thanks!

Regards,

-- 
Masahiko Sawada
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com