Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Hi,

On 2019-10-09 16:29:07 -0400, Dave Cramer wrote:
> I've added functionality into libpq to be able to set this STARTUP
> parameter as well as changed it to _pq_.report.
> Still need to document this and figure out how to test it.

> From 85de9f48f80a3bfd9e8bdd4f1ba6b177b1ff9749 Mon Sep 17 00:00:00 2001
> From: Dave Cramer <davecramer(at)gmail(dot)com>
> Date: Thu, 11 Jul 2019 08:20:14 -0400
> Subject: [PATCH] Add a STARTUP packet option to set GUC_REPORT on GUC's that
> currently do not have that option set. There is a facility to add protocol
> options using _pq_.<newoption> The new option name is report and takes a
> comma delmited string of GUC names which will have GUC_REPORT set. Add
> functionality into libpq to accept this new option key

I don't think it's good to only be able to change this at connection
time. Especially for poolers this ought to be configurable at any
time. I do think startup message support makes sense (especially to
avoid race conditions around to-be-reported gucs changing soon after
connecting), don't get me wrong, I just don't think it's sufficient.

> @@ -2094,6 +2094,7 @@ retry1:
> * zeroing extra byte above.
> */
> port->guc_options = NIL;
> + port->guc_report = NIL;
>
> while (offset < len)
> {
> @@ -2138,13 +2139,34 @@ retry1:
> }
> else if (strncmp(nameptr, "_pq_.", 5) == 0)
> {
> - /*
> - * Any option beginning with _pq_. is reserved for use as a
> - * protocol-level option, but at present no such options are
> - * defined.
> - */
> - unrecognized_protocol_options =
> - lappend(unrecognized_protocol_options, pstrdup(nameptr));
> + if (strncasecmp(nameptr + 5, "report", 6) == 0)
> + {
> + char sep[3] = " ,";
> +
> + /* avoid scribbling on valptr */
> + char *temp_val = pstrdup(valptr);
> +
> + /* strtok is going to scribble on temp_val */
> + char *freeptr = temp_val;
> + char *guc_report = strtok(temp_val, sep);
> + while (guc_report)
> + {
> + port->guc_report = lappend(port->guc_report,
> + pstrdup(guc_report));
> + guc_report = strtok(NULL, sep);
> + }
> + pfree(freeptr);
> + }

I don't think it's good to open-code this inside
ProcessStartupPacket(). Should be moved into its own function. I'd
probably even move all of the option handling out of
ProcessStartupPacket() before expanding it further.

I don't like the use of strtok, nor the type of parsing done
here. Perhaps we could just use SplitGUCList()?

> + else
> + {
> + /*
> + * Any option beginning with _pq_. is reserved for use as a
> + * protocol-level option, but at present no such options are
> + * defined.
> + */
> + unrecognized_protocol_options =
> + lappend(unrecognized_protocol_options, pstrdup(nameptr));
> + }
> }

You can't just move a comment explaining what _pq_ is into the else,
especially not without adjusting the contents.

> +/*
> + * Set the option to be GUC_REPORT
> + */
> +
> +bool
> +SetConfigReport(const char *name, bool missing_ok)
> +{
> + struct config_generic *record;
>
> + record = find_option(name, false, WARNING);
> + if (record == NULL)
> + {
> + if (missing_ok)
> + return 0;
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_OBJECT),
> + errmsg("unrecognized configuration parameter \"%s\"",
> + name)));
> + }
> + record->flags |= GUC_REPORT;
> +
> + return 0;
> +}

This way we loose track which gucs originally were marked as REPORT,
that strikes me as bad. We'd imo want to be able to debug this by
querying pg_settings.

Greetings,

Andres Freund