From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, pavel(dot)stehule(at)gmail(dot)com, a(dot)zakirov(at)postgrespro(dot)ru, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs |
Date: | 2018-03-21 05:01:03 |
Message-ID: | 20180321050103.GD2288@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Mar 20, 2018 at 01:27:35PM -0400, Tom Lane wrote:
> 1. Only GUC_LIST_QUOTE variables need to be special-cased. It works
> fine to treat plain LIST variables (e.g. datestyle) with the regular
> code path. This is good because there are so few of them.
Check.
> 2. We should make an effort to minimize the number of places that know
> explicitly which variables are GUC_LIST_QUOTE. In the backend, the
> right thing to do is ask guc.c. In pg_dump, there's no convenient
> way to ask the backend (and certainly nothing that would work against
> old servers), but we should at least make sure there's only one place
> to fix not two.
The only way I can think about for that would be to provide this
information in pg_settings using a text[] array or such which lists all
the GUC flags of a parameter. That's a hell lot of infrastructure
though which can be easily replaced with the maintenance of a small
hardcoded parameter list.
> 3. We need to prevent extensions from trying to create GUC_LIST_QUOTE
> variables, because those are not going to work correctly if they're
> set before the extension is loaded, nor is there any hope of pg_dump
> dumping them correctly.
This really depends on the elements of the list involved here, so
pg_dump may be able to dump them correctly, though not reliably as that
would be fully application-dependent. At the end I think that I am on
board with what you are proposing here.
> The attached 0001 patch does the first two things, and could be
> back-patched. The 0002 patch, which I'd only propose for HEAD,
> is one way we could address point 3. A different way would be
> to throw a WARNING rather than ERROR and then just mask off the
> problematic bit. Or we could decide that either of these cures
> is worse than the disease, but I don't really agree with that.
Looks roughly sane to me.
+ if (flags & GUC_LIST_QUOTE)
+ elog(FATAL, "extensions cannot define GUC_LIST_QUOTE
variables");
This would be better as an ereport with ERRCODE_FEATURE_NOT_SUPPORTED I
think. An ERROR is better in my opinion.
- if (pg_strcasecmp(configitem, "DateStyle") == 0
- || pg_strcasecmp(configitem, "search_path") == 0)
+ if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE)
For boolean-based comparisons, I would recommend using a comparison with
zero.
+ record = find_option(name, false, WARNING);
+ if (record == NULL)
LOG instead of WARNING?
--
Michael
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2018-03-21 05:24:16 | Re: [HACKERS] proposal: schema variables |
Previous Message | Andres Freund | 2018-03-21 03:43:16 | Re: [HACKERS] taking stdbool.h into use |