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

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