Guess I shouldn’t have said it was easy considering all the edits needed hehe.
Thanks a lot for the help, fixes and push Tom! Wasn’t expecting such a lightning turnaround.
On 16 Mar 2025 at 18:02 +0000, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
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.
Makes sense. I actually thought about calling set_config option but decided not fixing it was more backwards compatible.
But can’t think of any reason why anyone wouldn’t want it working like a true GUC.
* 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.
Makes sense
* 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.
Agree you edits make it more clear.
All in all though, pretty close for a first contribution.
Thanks again!
regards, tom lane
