Date: Tue, 29 Sep 2020 15:36:24 +0900
From: Michael Paquier <michael@paquier.xyz>
To: =?iso-8859-1?Q?R=E9mi?= Lapeyre <remi.lapeyre@lenstra.fr>
Cc: vignesh C <vignesh21@gmail.com>,
	Daniel Verite <daniel@manitou-mail.org>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Add header support to text format and matching feature
Message-ID: <20200929063624.GC29096@paquier.xyz>
References: <4009F36F-7D63-4E76-8CFE-776A5ED71BB9@lenstra.fr>
 <80a9b594-01d6-4ee4-a612-9ae9fd3c1194@manitou-mail.org>
 <CALDaNm07bcwPY2W68Qghvw2QAV9X8NkV6pihGicko5om=txF+Q@mail.gmail.com>
 <0B55BD07-83E4-439F-AACC-FA2D7CF50532@lenstra.fr>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="E/DnYTRukya0zdZ1"
Content-Disposition: inline
In-Reply-To: <0B55BD07-83E4-439F-AACC-FA2D7CF50532@lenstra.fr>
Precedence: bulk


--E/DnYTRukya0zdZ1
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Aug 27, 2020 at 04:53:11PM +0200, R=E9mi Lapeyre wrote:
> I have two remarks with the state of the current patches:
> - DefGetCopyHeader() duplicates a lot of code from defGetBoolean(),
> should we refactor this so that they can share more of their
> internals? In the current implementation any change to
> defGetBoolean() should be made to DefGetCopyHeader() too or their
> behaviour will subtly differ.

The difference comes from the use of "match", and my take would be
here that it is wrong to assume that header can be a boolean-like
parameter with only one exception.  It seems to me that we may
actually be looking at having this stuff as an option different than
"header" at the end to have clear semantics.

> - It is possible to set the header option multiple time:
>      \copy x(i) from file.txt with (format csv, header off, header on);
>   In which case the last one is the one kept. I think this is a bug
> and it should be fixed, but this is already the behaviour in the
> current implementation so fixing it would not be backward
> compatible. Do you think users should not do this and I can fix it
> or that keeping the current behaviour is better for backward
> compatibility?=20

I would agree that this is a bug because we are failing to detect
what's actually a redundant option here as the first option still
causes the flag to be set to false, but that's not something worth a
back-patch IMO.  What we are looking here is something similar
to what is done with "format", where we track if the option has been
specified with format_specified.  The same is actually true with the
"freeze" option here, and it is true that we tend to prefer error-ing
in such cases while there are exceptions like EXPLAIN.  I think that
it would be nicer to be at least consistent with the behavior that
each command has chosen, and COPY is now a mixed bag.

I have marked the patch as returned with feedback for now.
--
Michael

--E/DnYTRukya0zdZ1
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl9y1egACgkQnvQgOdby
QH2VBw//W61hbG+fjplaqBysOP2dwrmR0xGn+frTwfIF1Gj/UgwqTSVbdbt5IBI0
4r73jpCBmMCuPW5vJ4Va7xFthIBXwwz44+o1sbHZVDXZg8LxfD4LcQ6892jh794x
zZVbYpbikr5WMW7mt7CdWzZ4tjmgoBzImn25JPeQCUJnPvf0JQEuPYyC09Op4Rt+
wIJWw17yG83k0zM3gyjqTJD1/lBzl6ivVRGiTwTfIcZ1c2oUr3Sq3i/mztixU5rl
S0+ryHU1c1iLwNfOBuEuNypi6p02rK1MzeJkJdLppFAKPDT4lrVVg6vispEjPTxJ
OkcnkpdlX4U4zNPqsMo0q0AGr04YyeP0/Xb9Ky31Nt9s4kSwvJMt2WYq59q/pRAS
5gyaAHU7auRbm1bskkOxdSB6s5E0oxcDCEvek6GALDT0Na1MJn2TOzLoqcrSOjqL
tkel9ZvltJyAQL80YEM9TWd/dI0G5qciYv0xZ/ouIBJRXEHXwMa3QCm+b76w89fx
sjKIu9/3dS+bmz4OrlODRXq8mBEk8aRBT6FYOdr/3inDmSf6KLScyizNb3tUt4SP
Mb/Mug2ItNY15ZUwWyb6OJlCMn42FOFG4ivtmRCvyq06tfDVsUVfNGplMU5xyGgc
kqK62Ju5mu7eoeVRlUUgPgj42MwwcD7mrj+WCFrWrRJd9T6ymhA=
=Osb7
-----END PGP SIGNATURE-----

--E/DnYTRukya0zdZ1--