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

On Mon, Feb 14, 2022 at 6:50 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> writes:
> > On Mon, Feb 7, 2022 at 6:21 AM Dmitry Koval <d(dot)koval(at)postgrespro(dot)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/