Horton

Thread

Commits

  1. Renumber GUC flags for a bit more sanity.

  2. Introduce GUC_NO_RESET flag.

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

    The Post Office <noreply@postgresql.org> — 2022-01-28T09:07:40Z

    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!

  2. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Dmitry Koval <d.koval@postgrespro.ru> — 2022-01-28T15:26:08Z

    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.

  3. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-02-04T08:48:41Z

    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/

  4. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Dilip Kumar <dilipbalaut@gmail.com> — 2022-02-04T10:14:46Z

    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

  5. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Dmitry Koval <d.koval@postgrespro.ru> — 2022-02-06T07:37:47Z

    > 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.

  6. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Dilip Kumar <dilipbalaut@gmail.com> — 2022-02-06T08:29:24Z

    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

  7. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Dmitry Koval <d.koval@postgrespro.ru> — 2022-02-06T21:21:39Z

    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.

  8. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-02-07T02:41:26Z

    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/

  9. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-13T21:50:29Z

    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

  10. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-13T22:09:35Z

    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

  11. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-02-14T05:13:27Z

    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/

  12. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-02-14T05:17:58Z

    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/

  13. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-14T05:39:28Z

    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

  14. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-02-15T16:47:56Z

    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/

  15. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-15T16:55:22Z

    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

  16. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-03-09T02:26:42Z

    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/

  17. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-09T15:11:47Z

    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

  18. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-03-09T23:36:35Z

    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/

  19. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-03-11T15:39:53Z

    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/

  20. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-11T19:53:12Z

    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

  21. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-03-14T08:12:18Z

    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/

  22. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-14T08:30:00Z

    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

  23. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-14T13:45:17Z

    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

  24. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-14T18:59:37Z

    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

  25. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-15T02:33:33Z

    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

  26. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-03-23T08:48:43Z

    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/

  27. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-03-24T14:23:57Z

    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/

  28. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Justin Pryzby <pryzby@telsasoft.com> — 2022-06-28T00:37:20Z

    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

  29. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-06-28T06:58:41Z

    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/

  30. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Michael Paquier <michael@paquier.xyz> — 2022-06-28T07:58:49Z

    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

  31. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Michael Paquier <michael@paquier.xyz> — 2022-06-29T06:47:57Z

    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

  32. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-07-01T05:11:47Z

    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/

  33. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-09-22T02:03:24Z

    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

  34. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-24T20:57:51Z

    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

  35. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Michael Paquier <michael@paquier.xyz> — 2022-09-26T03:40:55Z

    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

  36. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-26T04:07:50Z

    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

  37. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-26T04:16:24Z

    [ 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

  38. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Michael Paquier <michael@paquier.xyz> — 2022-09-26T09:54:59Z

    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

  39. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-26T10:59:47Z

    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

  40. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-09-27T01:26:11Z

    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

  41. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Michael Paquier <michael@paquier.xyz> — 2022-09-27T03:39:28Z

    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

  42. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-27T13:56:57Z

    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

  43. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-27T15:51:45Z

    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

  44. Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-09-28T02:23:16Z

    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