Re: Add header support to text format and matching feature

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