| From: | JoongHyuk Shin <sjh910805(at)gmail(dot)com> |
|---|---|
| To: | Álvaro Herrera <alvherre(at)kurilemu(dot)de> |
| Cc: | Scott Ray <scott(at)scottray(dot)io>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks |
| Date: | 2026-06-22 04:37:27 |
| Message-ID: | CACSdjfMt5obQc0QCX8E+v-pxROWFQxmeW6Ki9X16F60D3tSJgg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Thanks for the review.
v6 attached.
The set-parameter list in the errdetail now wraps the separator and quotes
in _()
so the punctuation is translatable, following 3692a622d3fd.
The five GetConfigOption() checks are now a local macro, as suggested.
--
JH Shin
On Sun, Jun 21, 2026 at 9:32 PM Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:
> Hello,
>
> On 2026-Jun-21, JoongHyuk Shin wrote:
>
> > The errdetail now lists which recovery_target_* parameters are actually
> set,
> > instead of the full candidate list.
> > This follows Scott's idea to surface the set targets;
> > following Álvaro, the values are dropped
> > and a new errhint points to pg_settings for them and their sources.
> >
> > I kept the "which ones are set" list in errdetail rather than errhint,
> > it states the current configuration, which reads as detail,
> > while the errhint carries the actionable pg_settings pointer.
>
> Please see https://postgr.es/c/3692a622d3fd for more on translatable
> message construction. You should end up with a translatable string in
> a _() call like
> _(", \"%s\"")
> and the GUC names in a separate string in each case.
>
> Maybe you can make this a local macro to avoid repetitive coding,
>
> #define considerAndComplainAboutGUC(gucname, buf) \
> do { \
> val = GetConfigOption(gucname, false, false); \
> if (val[0] != '\0') \
> { \
> ntargets++; \
> if (buf.len == 0) \
> appendStringInfoString(&buf, _("\"%s\""), gucname); \
> else \
> appendStringInfoString(&buf, _(", \"%s\""), gucname); \
> } \
> } while (0)
>
> considerAndComplainAboutGUC("recovery_target", buf);
> considerAndComplainAboutGUC("recovery_target_lsn", buf);
> and so on. (Of course, you should choose a less stupid macro name, but
> you get my meaning.)
>
> --
> Álvaro Herrera PostgreSQL Developer —
> https://www.EnterpriseDB.com/
> "... In accounting terms this makes perfect sense. To rational humans, it
> is insane. Welcome to IBM." (Robert X. Cringely)
>
> https://www.cringely.com/2015/06/03/autodesks-john-walker-explained-hp-and-ibm-in-1991/
>
| Attachment | Content-Type | Size |
|---|---|---|
| v6-0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC.patch | application/octet-stream | 22.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | David Rowley | 2026-06-22 04:38:39 | Re: Improve UNION's output rowcount estimate |
| Previous Message | Peter Smith | 2026-06-22 04:35:11 | Re: Support EXCEPT for ALL SEQUENCES publications |