From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Dave Cramer <pg(at)fastcrypt(dot)com> |
Cc: | Craig Ringer <craig(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp>, Ian Barwick <ian(dot)barwick(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status) |
Date: | 2019-10-10 16:05:42 |
Message-ID: | 20191010160542.2gszdxhd56abzgvr@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2019-10-10 16:33:50 | Re: BUG #16045: vacuum_db crash and illegal memory alloc after pg_upgrade from PG11 to PG12 |
Previous Message | Tom Lane | 2019-10-10 15:51:21 | Re: configure fails for perl check on CentOS8 |