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@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/