Re: Improvements and additions to COPY progress reporting

On Fri, 12 Feb 2021 at 13:40, Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Fri, Feb 12, 2021 at 5:40 PM Matthias van de Meent
> <boekewurm+postgres(at)gmail(dot)com> wrote:
> >
> > On Fri, 12 Feb 2021 at 12:23, Matthias van de Meent
> > <boekewurm+postgres(at)gmail(dot)com> wrote:
> > >
> > > On Thu, 11 Feb 2021 at 15:44, Bharath Rupireddy
> > > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > > >
> > > >
> > > > On Thu, Feb 11, 2021, 8:08 PM Josef Šimánek <josef(dot)simanek(at)gmail(dot)com> wrote:
> > > >> I have split it since it should be the start of progress reporting
> > > >> testing at all. If you better consider this as part of COPY testing,
> > > >> feel free to move it to already existing copy testing related files.
> > > >> There's no real reason to keep it separated if not needed.
> > > >
> > > >
> > > > +1 to move those test cases to existing copy test files.
> > >
> > > Thanks for your reviews. PFA v4 of the patchset, in which the tests
> > > are put into copy.sql (well, input/copy.source). This also adds tests
> > > for correctly reporting COPY ... FROM 'file'.
> >
> > PFA v5, which fixes a failure in the pg_upgrade regression tests due
> > to incorrect usage of @abs_builddir(at)(dot) I had the changes staged, but
> > forgot to add them to the patches.
> >
> > Sorry for the noise.
>
> Looks like the patch 0001 that was adding tuples_excluded was missing
> and cfbot is also not happy with the v5 patch set.
>
> Maybe, we may not need 6 patches as they are relatively very small
> patches. IMO, the following are enough:
>
> 0001 - tuples_excluded, lines to tuples change, COPY FROM/COPY TO
> addition, io_target -- basically all the code related patches can go
> into 0001
> 0002 - documentation
> 0003 - tests - I think we can only have a simple test(in copy2.sql)
> showing stdin/stdout and not have file related tests. Because these
> patches work as expected, please find my testing below:

I agree with that split, the current split was mainly for the reason
that some of the patches (except 1, 3 and 6, which are quite
substantially different from the rest) each have had their seperate
concerns voiced about the changes contained in that patch (be it
direct or indirect); e.g. the renaming of lines_* to tuples_* is in my
opionion a good thing, and Josef disagrees.

Anyway, please find attached patchset v6 applying that split.

Regarding only a simple test: I believe it is useful to have at least
a test that distinguishes between two different import types. I've
made a mistake before, so I think it is useful to add a regression
tests to prevent someone else from making this same mistake (trivial
as it may be). Additionally, testing in copy.sql also allows for
validating the bytes_total column, which cannot be tested in copy2.sql
due to the lack of COPY FROM FILE -support over there. I'm +0.5 on
keeping it as-is in copy.sql, so unless someone has some strong
feelings about this, I'd like to keep it in copy.sql.

With regards,

Matthias van de Meent.