Re: Add header support to text format and matching feature

Thanks for your comments, Please find my thoughts inline.

> In my tests it works fine except for one crash that I can reproduce
> on a fresh build and default configuration with:
>
> $ cat >file.txt
> i
> 1
>
> $ psql
> postgres=# create table x(i int);
> CREATE TABLE
> postgres=# \copy x(i) from file.txt with (header match)
> COPY 1
> postgres=# \copy x(i) from file.txt with (header match)
> COPY 1
> postgres=# \copy x(i) from file.txt with (header match)
> COPY 1
> postgres=# \copy x(i) from file.txt with (header match)
> COPY 1
> postgres=# \copy x(i) from file.txt with (header match)
> COPY 1
> postgres=# \copy x(i) from file.txt with (header match)
> PANIC: ERRORDATA_STACK_SIZE exceeded
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
>

Fixed, replaced PG_TRY/PG_CATCH with strcmp logic to get the header option.

>
> Code comments:
>
>
> +/*
> + * Represents whether the header must be absent, present or present and
> match.
> + */
> +typedef enum CopyHeader
> +{
> + COPY_HEADER_ABSENT,
> + COPY_HEADER_PRESENT,
> + COPY_HEADER_MATCH
> +} CopyHeader;
> +
> /*
> * This struct contains all the state variables used throughout a COPY
> * operation. For simplicity, we use the same struct for all variants of
> COPY,
> @@ -136,7 +146,7 @@ typedef struct CopyStateData
> bool binary; /* binary format? */
> bool freeze; /* freeze rows on loading? */
> bool csv_mode; /* Comma Separated Value
> format? */
> - bool header_line; /* CSV or text header line? */
> + CopyHeader header_line; /* CSV or text header line? */
>
>
> After the redefinition into this enum type, there are still a
> bunch of references to header_line that treat it like a boolean:
>
> 1190: if (cstate->header_line)
> 1398: if (cstate->binary && cstate->header_line)
> 2119: if (cstate->header_line)
> 3635: if (cstate->cur_lineno == 0 && cstate->header_line)
>
> It works fine since COPY_HEADER_ABSENT is 0 as the first value of the enum,
> but maybe it's not good style to count on that.

Fixed. Changed it to cstate->header_line != COPY_HEADER_ABSENT.

>
>
>
> + PG_TRY();
> + {
> + if (defGetBoolean(defel))
> + cstate->header_line =
> COPY_HEADER_PRESENT;
> + else
> + cstate->header_line =
> COPY_HEADER_ABSENT;
> + }
> + PG_CATCH();
> + {
> + char *sval = defGetString(defel);
> +
> + if (!cstate->is_copy_from)
> + PG_RE_THROW();
> +
> + if (pg_strcasecmp(sval, "match") == 0)
> + cstate->header_line =
> COPY_HEADER_MATCH;
> + else
> + ereport(ERROR,
> +
> (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("header requires a
> boolean or \"match\"")));
> + }
> + PG_END_TRY();
>
> It seems wrong to use a PG_CATCH block for this. I understand that
> it's because defGetBoolean() calls ereport() on non-booleans, but then
> it should be split into an error-throwing function and a
> non-error-throwing lexical analysis of the boolean, the above code
> calling the latter.
> Besides the comments in elog.h above PG_TRY say that
> "the error recovery code
> can either do PG_RE_THROW to propagate the error outwards, or do a
> (sub)transaction abort. Failure to do so may leave the system in an
> inconsistent state for further processing."
> Maybe this is what happens with the repeated uses of "match"
> eventually failing with ERRORDATA_STACK_SIZE exceeded.
>

Fixed, replaced PG_TRY/PG_CATCH with strcmp logic to get the header option.

>
> - HEADER [ <replaceable class="parameter">boolean</replaceable> ]
> + HEADER { <literal>match</literal> | <literal>true</literal> |
> <literal>false</literal> }
>
> This should be enclosed in square brackets because HEADER
> with no argument is still accepted.
>

Fixed.

>
>
>
> + names from the table. On input, the first line is discarded when set
> + to <literal>true</literal> or required to match the column names if
> set
>
> The elision of "header" as the subject might be misinterpreted as if
> it's the first line that is true. I'd suggest
> "when <literal>header>/literal> is set to ..." to avoid any confusion.
>

Fixed.

Attached v5 patch with the fixes of above comments.
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com