Stale references to guc.c in comments/tests

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