public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Rémi Lapeyre <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Daniel Verite <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Add header support to text format and matching feature
Date: Tue, 29 Sep 2020 15:36:24 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<CALDaNm07bcwPY2W68Qghvw2QAV9X8NkV6pihGicko5om=txF+Q@mail.gmail.com>
<[email protected]>
On Thu, Aug 27, 2020 at 04:53:11PM +0200, Rémi 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?
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (41+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add header support to text format and matching feature
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox