Re: Allow COPY's 'text' format to output a header
On 14 May 2018 at 08:35, Simon Muller wrote: > Okay, I've added this to the next commitfest at > https://commitfest.postgresql.org/18/1629/. > > Thanks both Michael and David for the feedback so far. > I noticed through the patch tester link at http://commitfest.cputube.org/ that my patch caused a file_fdw test to fail (since I previously tested only with "make check" and not with "make check-world"). This v2 patch should fix that. text_header_v2.patch Description: Binary data
Re: Allow COPY's 'text' format to output a header
On 4 July 2018 at 22:44, Simon Muller wrote: > I noticed through the patch tester link at http://commitfest.cputube.org/ > that my patch caused a file_fdw test to fail (since I previously tested > only with "make check" and not with "make check-world"). > > This v2 patch should fix that. > This patch just fixes a newline issue introduced in my previous patch. text_header_v3.patch Description: Binary data
Re: Allow COPY's 'text' format to output a header
On 25 July 2018 at 19:24, Cynthia Shang wrote: > > I've reviewed this patch and feel this patch addresses the original ask. I > tested it manually trying to break it and, as mentioned previously, it's > behavior is the same as the CSV copy with regards to it's shortcomings. > However, I feel > 1) a "copy from" test is needed and > 2) the current "copy to" test is (along with a few others) in the wrong > file. > > With regards to #2, the copy.source tests are for things requiring > replacement when running the tests. Given that these copy tests do not, I > have moved the current last set of copy tests to the copy2.sql file and > have provided an attached patch. > > Thanks for reviewing the patch. I agree that moving those previous and these new tests out of the .source files seems to make more sense as they don't make use of the preprocessing/replacement feature. With regards to #1, the patch I have provided can then be used and the > following added as the COPY TO/FROM tests (perhaps after line 426 of the > attached copy2.sql file). Note that I moved the FROM test before the TO > test and omitted the "(format text, header true)" in the FROM test since it > is another way the command can be invoked. > > copy copytest3 from stdin header; > this is just a line full of junk that would error out if parsed > 11 a 1 > 22 b 2 > \. > > copy copytest3 to stdout with (format text, header true); > > > I've incorporated both your suggestions and included the patch you provided in the attached patch. Hope it's as expected. > As for the matching check of the header in the discussion of this patch, I > feel that is a separate patch that can be added later since it would affect > the general functionality of the copy command, not just the ability to have > a text header. > > Best, > - Cynthia Shang > P.S. I did receive the first attached patch, but on my Ubuntu I had to apply it using "git apply --ignore-space-change --ignore-whitespace", probably due to line ending differences. -- Simon Muller text_header_v4.patch Description: Binary data
Re: Allow COPY's 'text' format to output a header
On 1 August 2018 at 17:18, Cynthia Shang
wrote:
>
> > On Aug 1, 2018, at 10:20 AM, Daniel Verite
> 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
Re: Allow COPY's 'text' format to output a header
On 2 August 2018 at 17:07, Cynthia Shang wrote: > > > On Aug 2, 2018, at 8:11 AM, Daniel Verite > wrote: > > > > That makes sense, thanks for elaborating, although there are also > > a fair number of ERRCODE_FEATURE_NOT_SUPPORTED in copy.c > > that are raised on forbidden/nonsensical combination of features, > > so the consistency argument could work both ways. > > > > If there is not a strong reason to change the error code, then I believe > we should not. The error is the same as it was before, just narrower in > scope. > > Best, > -Cynthia Sure, thanks both for the feedback. Attached is a patch with the error kept as ERRCODE_FEATURE_NOT_SUPPORTED. -- Simon Muller text_header_v5.patch Description: Binary data
Re: Allow COPY's 'text' format to output a header
On 6 August 2018 at 16:34, Stephen Frost wrote: > Greetings, > > * Cynthia Shang (cynthia.sh...@crunchydata.com) wrote: > > I was able to apply the patch (after resolving a merge conflict which > was expected given an update in master). All looks good. > > If there's a merge conflict against master, then it'd be good for an > updated patch to be posted. > > Thanks! > > Stephen > Attached is an updated patch that should directly apply against current master. -- Simon Muller text_header_v6.patch Description: Binary data
Allow COPY's 'text' format to output a header
This patch adds the capability to use the HEADER feature with the "text" format of the COPY command. The patch includes the related update to documentation and an additional regression test for this feature. Currently you can only add a header line (which lists the column names) when exporting with COPY to the CSV format, but I much prefer using the default "text" format. This feature is also currently listed on the to-do list (https://wiki.postgresql.org/wiki/Todo#COPY) where it seems to have been requested some years ago. Hopefully I've done everything correctly and the patch is acceptable enough to be considered for application. Simon Muller 0001-Allow-COPY-s-text-format-to-output-a-header.patch Description: Binary data
Re: Allow COPY's 'text' format to output a header
Okay, I've added this to the next commitfest at https://commitfest.postgresql.org/18/1629/. Thanks both Michael and David for the feedback so far. On 14 May 2018 at 02:37, Michael Paquier wrote: > On Sun, May 13, 2018 at 07:01:00PM -0400, David Steele wrote: > > This patch makes sense to me and looks reasonable. > > One "potential" problem is if a relation has a full set of column which > allows the input of text-like data: if the header has been added with > COPY TO, and that the user forgets to add again the header option with > COPY FROM, then an extra row will be generated but there is the same > problem with CSV format :) > > One comment I have about the patch is that there is no test for > COPY FROM with an output file which has a header. In this case if > HEADER is true then the file can be loaded. If HEADER is wrong, an > error should normally be raised because of the format (well, let's > discard the case of the relation with text-only columns). So the tests > could be extended a bit even for CSV. > > > We're in the middle of a feature freeze that will last most of the > > summer, so be sure to enter your patch into the next commitfest so it > > can be considered when the freeze is over. > > > > https://commitfest.postgresql.org/18/ > > Yes, you will need to be patient a couple of months here. > -- > Michael >
