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

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