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(dot)mshk(at)gmail(dot)com> wrote:
>
> On Sat, Jan 29, 2022 at 12:26 AM Dmitry Koval <d(dot)koval(at)postgrespro(dot)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