On Thu, 10 Oct 2019 at 12:05, Andres Freund <and...@anarazel.de> wrote:
> 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 <davecra...@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.
>
So off the top of my head providing a system function seems like the way to
go here unless you were contemplating adding something to the protocol ?
>
> > @@ -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()?
>
Fair enough
>
>
> > + 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.
>
> Upon review I'm left with "what was I thinking?"
>
>
>
>
> > +/*
> > + * 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.
>
> I'm open to suggestions here, although I'm not sure what your concern is?
Dave Cramer
da...@postgresintl.com
www.postgresintl.com
>
>
