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.


Greetings,

Andres Freund