Re: Add header support to text format and matching feature

Peter Eisentraut wrote:

> - The DefGetCopyHeader() function seems very bulky and might not be
> necessary. I think you can just check for the string "match" first and
> then use defGetBoolean() as before if it didn't match.

The problem is that defGetBoolean() ends like this in the non-matching
case:

ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a Boolean value",
def->defname)));

We don't want this error message when the string does not match
since it's really a tri-state option, not a boolean.

To avoid duplicating the code that recognizes true/false/on/off/0/1,
probably defGetBoolean()'s guts should be moved into another function
that does the matching and report to the caller instead of throwing an
error. Then DefGetCopyHeader() could call that non-throwing function.

> - If you define COPY_HEADER_ABSENT = 0 in the enum, then most of the
> existing truth checks don't need to be changed.

It's already 0 by default. Not changing some truth checks does work, but
then we get some code that treat CopyFromState.header_line like
a boolean and some other code like an enum, which I found not
ideal wrt clarity in an earlier round of review [1]

It's not a major issue though, as it concerns only 3 lines of code in the
v12
patch:
- if (opts_out->binary && opts_out->header_line)
+ if (opts_out->binary && opts_out->header_line != COPY_HEADER_ABSENT)

+ /* on input check that the header line is correct if needed */
+ if (cstate->cur_lineno == 0 && cstate->opts.header_line !=
COPY_HEADER_ABSENT)

- if (cstate->opts.header_line)
+ if (cstate->opts.header_line != COPY_HEADER_ABSENT)

> - In NextCopyFromRawFields(), it looks like you have code that replaces
> the null_print value if the supplied column name is null. I don't think
> we should allow null column values. Someone, this should be an error.

+1

[1]
https://www.postgresql.org/message-id/80a9b594-01d6-4ee4-a612-9ae9fd3c1194%40manitou-mail.org

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite