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@gmail.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.