Re: ISN extension - wrong volatility level for isn_weak() function

Viktor Holmberg <v@viktorh.net> writes:
> Oh, sorry! Don’t know what I was doing there - not used to this patch based workflow. Here comes the real patch.
> This now uses the GUC - that was a lot easier than I thought.

I reviewed this and pushed it with some corrections.  Thanks for
the contribution!

Some notes:

* To make g_weak actually act like a GUC, accept_weak_input() has
to set it via set_config_option() not just overwrite it.  Otherwise
it won't roll back on transaction abort, for example.  There was also
a missing MarkGUCPrefixReserved() call.

* I concluded that the isn_weak() functions ought to be marked like
set_config() (VOLATILE and PARALLEL UNSAFE) and current_setting()
(STABLE and PARALLEL SAFE).  It's debatable whether a function that
reacts to a GUC should really be STABLE, since you can surely make its
output change intra-query if you try.  However, we pretty consistently
do that elsewhere because of the negative performance implications of
marking all such functions VOLATILE.  The previous PARALLEL RESTRICTED
markings were probably okay when g_weak's value couldn't propagate into
parallel workers, but they're wrong now.

* You missed updating the module's meson.build file, which is hardly
your fault since I pointed you at an example commit that predated
our Meson support.  But nowadays pretty much anything done to a
Makefile has to be reflected in meson.build and vice versa.

* I editorialized on the docs a bit, mostly to be more like existing
practice in other contrib modules.  One point there is that "GUC"
is internal jargon that we prefer to avoid using in user-facing docs.

All in all though, pretty close for a first contribution.
Thanks again!

            regards, tom lane