Date: Wed, 21 Mar 2018 14:01:03 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>,
	pavel.stehule@gmail.com, a.zakirov@postgrespro.ru,
	pgsql-hackers@postgresql.org
Subject: Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs
Message-ID: <20180321050103.GD2288@paquier.xyz>
References: <2637.1521501336@sss.pgh.pa.us>
 <20180320003951.GA4963@paquier.xyz>
 <10037.1521515233@sss.pgh.pa.us>
 <20180320.123747.43512160.horiguchi.kyotaro@lab.ntt.co.jp>
 <32403.1521566855@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="yudcn1FV7Hsu/q59"
Content-Disposition: inline
In-Reply-To: <32403.1521566855@sss.pgh.pa.us>
Precedence: bulk


--yudcn1FV7Hsu/q59
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--yudcn1FV7Hsu/q59
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlqx5w8ACgkQnvQgOdby
QH0UVQ//fGo1UsIUAEb6wyXJd13ATHtb4nBE6qHEtjYAvFQe9SvsU7xmPtIiKMIE
p8DNV1zUu6GP/MCFLm99LBQ5dOCDRRZvWpZAphTLTx+saBoTJLes2stt3jtnGr4N
3m5SMoM5iGb5V5B8SFS0mshLbUYxopO1BNHrXegjbLaiXCRXrfghXYK68XMwU+JM
XgwPtnh6CO20huepQzthf2FuxaMGhlVuoAr51O0r5dqUMufU3gmaH9AYiMmA7ok8
8wq/TlSax7O4yhqWF/b1PNPduB7iQWKlg38Qy0ezEQBvf1EN5QSicWCkrqyWMHvi
MwFYkqwT5RKZKkwF0arnXEwIL7ODTtFc3Kj450DyWQPqFrz/Q0Hoz4HvwPqjQnvp
l90+vR1llVTaBdmS+Oh5cK5w8+eGgTjZD/zr9aDxYyzb8+IbdE08xoNSaL7yqEWm
ruMQXNMONV2RBroyZ2bvBwl8+ePS5k/u16ndjY4CQYrufo5vXqh15xkeYB6owzDy
CL5G1g4L+DawJZN9tCYF8sgdST2d4TQRX4RVpXXTKVtev1eZP+J5K1Dj/C8drh4o
j2fOzU5nsPKYTMUO+diNEDxPX7I8TjllBbiPoQvaFVWI88wO+EJMGXWI1LJ3sDZ0
AfSGfjNFm54gxvAXSAzodPVjY5khIR+FHQYXI9aO27+buAjNx90=
=PCLS
-----END PGP SIGNATURE-----

--yudcn1FV7Hsu/q59--