Re: GUC values - recommended way to declare the C variables?

On Thu, Oct 27, 2022 at 1:33 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Oct 26, 2022 at 09:14:37PM -0500, Justin Pryzby wrote:
> > It seems like you're reviewing the previous version of the patch, rather
> > than the one attached to the message you responded to (which doesn't
> > have anything to do with GUC_DEFAULT_COMPILE).
>
> It does not seem so as things stand, I have been looking at
> v5-0001-GUC-C-variable-sanity-check.patch as posted by Peter here:
> https://www.postgresql.org/message-id/CAHut+PuCHjYXiTGdTOvHvDnjpbivLLr49gWVS+8VwnfoM4hJTw@mail.gmail.com
>
> In combination with a two-patch set as posted by you here:
> 0001-add-DYNAMIC_DEFAULT-for-settings-which-vary-by-.-con.patch
> 0002-WIP-test-guc-default-values.patch
> https://www.postgresql.org/message-id/20221024220544.GJ16921@telsasoft.com
>
> These are the latest patch versions posted on their respective thread
> I am aware of, and based on the latest updates of each thread it still
> looked like there was a dependency between both. So, is that the case
> or not? If not, sorry if I misunderstood things.

No. My v5 is no longer dependent on the other patch.

>
> > I don't know what you meant by "make the CF bot happy" (?)
>
> It is in my opinion confusing to see that the v5 posted on this
> thread, which was marked as ready for committer as of
> https://commitfest.postgresql.org/40/3934/, seem to rely on a facility
> that it makes no use of. Hence it looks to me that this patch has
> been posted as-is to allow the CF bot to pass (I would have posted
> that as an isolated two-patch set with the first patch introducing the
> flag if need be).

Yeah, my v4 was posted along with the other GUC flag patch as a
prerequisite to make the cfbot happy. This is no longer the case - v5
is a single independent patch. Sorry for the "ready for the committer"
status being confusing. At that time I thought it was.

>
> Anyway, per my previous comments in my last message of this thread as
> of https://www.postgresql.org/message-id/Y1nnwFTrnL3ItleP@paquier.xyz,
> I don't see a need for DYNAMIC_DEFAULT from the other thread, nor do I
> see a need to a style like that:
> +/* GUC variable */
> +bool update_process_title =
> +#ifdef WIN32
> + false;
> +#else
> + true;
> +#endif
>
> I think that it would be cleaner to use the same approach as
> checking_after_flush and similar GUCs with a centralized definition,
> rather than spreading such style in two places for each GUC that this
> patch touches (aka its declaration and its default value in
> guc_tables.c). In any case, the patch of this thread still needs some
> adjustments IMO.

OK, I can make that adjustment if it is preferred. I think it is the
same as what I already suggested a while ago [1] ("But probably it is
no problem to just add #defines...")

------
[1] https://www.postgresql.org/message-id/1113448.1665717297%40sss.pgh.pa.us

Kind Regards,
Peter Smith.
Fujitsu Australia.