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

On Mon, Feb 14, 2022 at 2:39 PM 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 14, 2022 at 6:50 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> 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.
>
> I toyed with that idea for awhile too, but looking through the current
> set of GUC_NO_RESET_ALL variables dissuaded me from it. The
> transaction-property GUCs need this behavior or something like it,
> but the rest don't. In particular, I fear that turning RESET ROLE
> into a no-op would create security bugs for applications that
> expect the current behavior.

Right.

It seems that we need another flag or a dedicated treatment for
transaction property GUCs. It effectively cannot change them within
the transaction regardless of SET, RESET, and RESET ALL, so I think we
can make it no-op (possibly with a notice).

> Having said that, it does seem like GUC_NO_RESET_ALL is pretty
> intellectually-inconsistent by definition. I'm just not sure that
> we can redefine our way out of that at this late date.

Yeah, it would get into areas too deep to tackle for v15. And given
many application relies on this behavior for a long time (it seems
GUC_NO_RESET_ALL has introduced about 20 years ago, see f0811a74b), we
might not have a big win by redefining it.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/