Thread: Stale references to guc.c in comments/tests

I happened to notice that there were a few references to guc.c regarding
variables, which with the recent refactoring in 0a20ff54f have become stale.
Attached is a trivial patch to instead point to guc_tables.c.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:
> I happened to notice that there were a few references to guc.c regarding
> variables, which with the recent refactoring in 0a20ff54f have become stale.
> Attached is a trivial patch to instead point to guc_tables.c.

Hmm, I think you may have done an overenthusiastic replacement here.
I agree with changes like this:

-extern char *role_string;        /* in guc.c */
+extern char *role_string;        /* in guc_tables.c */

because clearly that variable is now declared in guc_tables.c
and guc.c knows nothing of it explicitly.  However, a lot of
these places are really talking about the behavior of the GUC
mechanisms as a whole, and so a pointer to guc_tables.c doesn't
seem very on-point to me --- I find it hard to attribute behavior
to a static table.  Here for instance:

@@ -3041,7 +3041,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
                  *
                  * Variables that are not so marked should just be emitted as
                  * simple string literals.  If the variable is not known to
-                 * guc.c, we'll do that; this makes it unsafe to use
+                 * guc_tables.c, we'll do that; this makes it unsafe to use
                  * GUC_LIST_QUOTE for extension variables.
                  */
                 if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE)

An extension's GUC is by definition not known in guc_tables.c, so I think
this change is losing the point of the text.  What it's really describing
is a variable that hasn't been entered into the dynamic tables maintained
by guc.c.

Perhaps you could use "the GUC mechanisms" in these places, but it's a bit
longer than "guc.c".  Leaving such references alone seems OK too.

            regards, tom lane

> On 24 Feb 2023, at 16:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> I happened to notice that there were a few references to guc.c regarding
>> variables, which with the recent refactoring in 0a20ff54f have become stale.
>> Attached is a trivial patch to instead point to guc_tables.c.
>
> Hmm, I think you may have done an overenthusiastic replacement here.

Fair enough, I only changed those places I felt referenced variables, or their
definition, in guc_tables.c but I agree that there is a lot of greyzone in the
interpretation.

> Perhaps you could use "the GUC mechanisms" in these places, but it's a bit
> longer than "guc.c".  Leaving such references alone seems OK too.

I've opted for mostly leaving them in the attached v2.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:
> On 24 Feb 2023, at 16:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Perhaps you could use "the GUC mechanisms" in these places, but it's a bit
>> longer than "guc.c".  Leaving such references alone seems OK too.

> I've opted for mostly leaving them in the attached v2.

This version seems OK to me except for this bit:

  * This is a straightforward one-to-one mapping, but doing it this way makes
- * guc.c independent of OpenSSL availability and version.
+ * GUC definition independent of OpenSSL availability and version.

The grammar is a bit off ("the GUC definition" would read better),
but really I think the wording was vague already and we should tighten
it up.  Can we specify exactly which GUC variable(s) we're talking about?

            regards, tom lane

> On 27 Feb 2023, at 17:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> The grammar is a bit off ("the GUC definition" would read better),
> but really I think the wording was vague already and we should tighten
> it up.  Can we specify exactly which GUC variable(s) we're talking about?

Specifying the GUCs in question is a good idea, done in the attached.  I'm not
sure the phrasing is spot-on though, but I can't think of a better one.  If you
can think of a better one I'm all ears.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:
> Specifying the GUCs in question is a good idea, done in the attached.  I'm not
> sure the phrasing is spot-on though, but I can't think of a better one.  If you
> can think of a better one I'm all ears.

I'd just change "the definition of" to "the definitions of".
LGTM otherwise.

            regards, tom lane