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

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.

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