From: | Simon Muller <samullers(at)gmail(dot)com> |
---|---|
To: | Cynthia Shang <cynthia(dot)shang(at)crunchydata(dot)com> |
Cc: | Daniel Verite <daniel(at)manitou-mail(dot)org>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Allow COPY's 'text' format to output a header |
Date: | 2018-08-01 18:36:47 |
Message-ID: | CAF1-J-0p4wnexMzaV5gJHbtHuoX9vcn45X272ASXgu_o=XFt9w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 1 August 2018 at 17:18, Cynthia Shang <cynthia(dot)shang(at)crunchydata(dot)com>
wrote:
>
> > On Aug 1, 2018, at 10:20 AM, Daniel Verite <daniel(at)manitou-mail(dot)org>
> wrote:
> >
> > /* Check header */
> > - if (!cstate->csv_mode && cstate->header_line)
> > + if (cstate->binary && cstate->header_line)
> > ereport(ERROR,
> > - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > - errmsg("COPY HEADER available only in CSV mode")));
> > + (errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("cannot specify HEADER in BINARY mode")));
> >
> > Why should ERRCODE_FEATURE_NOT_SUPPORTED become ERRCODE_SYNTAX_ERROR?
> >
>
> I agree; it should remain ERRCODE_FEATURE_NOT_SUPPORTED and I might also
> suggest the message read "COPY HEADER not available in BINARY mode",
> although I'm pretty agnostic on the latter.
>
> Regards,
> -Cynthia Shang
I changed the error type and message for consistency with other similar
errors in that file. Whenever options are combined that are incompatible,
it looks like the convention is for a ERRCODE_SYNTAX_ERROR to be thrown.
For instance, in case you both specify a specific DELIMITER but also
declare the format as BINARY, then there is this code in that same file:
if (cstate->binary && cstate->delim)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("cannot specify DELIMITER in BINARY mode")));
HEADER seems very similar to me since, like DELIMITER, it makes sense for
the textual formats such as CSV and TEXT, but doesn't make sense with the
BINARY format.
ERRCODE_FEATURE_NOT_SUPPORTED previously made sense since the only reason
TEXT and HEADER weren't compatible options was because the feature was not
yet implemented, but now ERRCODE_SYNTAX_ERROR seems to make sense to me
since I can't foresee a use case where BINARY and HEADER would ever be
compatible options.
--
Simon Muller
From | Date | Subject | |
---|---|---|---|
Next Message | Sergei Kornilov | 2018-08-01 18:42:27 | Re: Online enabling of checksums |
Previous Message | Tom Lane | 2018-08-01 18:22:12 | Re: Internal error XX000 with enable_partition_pruning=on, pg 11 beta1 on Debian |