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
signature.asc
Description: PGP signature