Horton

Thread

Commits

  1. Add some basic tests for progress reporting of COPY

  2. Add support for more progress reporting in COPY

  3. doc: Add backlinks to progress reporting documentation

  1. Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-02-08T18:35:45Z

    Hi,

With [0] we got COPY progress reporting. Before the column names of
this newly added view are effectively set in stone with the release of
pg14, I propose the following set of relatively small patches. These
are v2, because it is a patchset that is based on a set of patches
that I previously posted in [0].

0001 Adds a column to pg_stat_progress_copy which details the amount
of tuples that were excluded from insertion by the WHERE clause of the
COPY FROM command.

0002 alters pg_stat_progress_copy to use 'tuple'-terminology instead
of 'line'-terminology. 'Line' doesn't make sense in the binary copy
case, and only for the 'text' copy format there can be a guarantee
that the source / output file actually contains the reported amount of
lines, whereas the amount of data tuples (which is also what it's
called internally) is guaranteed to equal for all data types.

There was some discussion about this in [0] where the author thought
'line' is more consistent with the CSV documentation, and where I
argued that 'tuple' is both more consistent with the rest of the
progress reporting tables and more consistent with the actual counted
items: these are the tuples serialized / inserted (as noted in the CSV
docs; "Thus the files are not strictly one line per table row like
text-format files.").

Patch 0003 adds backlinks to the progress reporting docs from the docs
of the commands that have progress reporting (re/index, cluster,
vacuum, etc.) such that progress reporting is better discoverable from
the relevant commands, and removes the datname column from the
progress_copy view (that column was never committed). This too should
be fairly trivial and uncontroversial.

0004 adds the 'command' column to the progress_copy view; which
distinguishes between COPY FROM and COPY TO. The two commands are (in
my opinion) significantly different enough to warrant this column;
similar to the difference between CREATE INDEX/REINDEX [CONCURRENTLY]
which also report that information. I believe that this change is
appropriate; as the semantics of the columns change depending on the
command being executed.

Lastly, 0005 adds 'io_target' to the reported information, that is,
FILE, PROGRAM, STDIO or CALLBACK. Although this can relatively easily
be determined based on the commands in pg_stat_activity, it is
reasonably something that a user would want to query on, as the
origin/target of COPY has security and performance implications,
whereas other options (e.g. format) are less interesting for clients
that are not executing that specific COPY command.

Of special interest in 0005 is that it reports the io_target for the
logical replications' initial tablesyncs' internal COPY. This would
otherwise be measured, but no knowledge about the type of copy (or its
origin) would be available on the worker's side. I'm not married to
this patch 0005, but I believe it could be useful, and therefore
included it in the patchset.


With regards,

Matthias van de Meent.


[0] https://www.postgresql.org/message-id/flat/CAFp7Qwr6_FmRM6pCO0x_a0mymOfX_Gg%2BFEKet4XaTGSW%3DLitKQ%40mail.gmail.com

  2. Re: Improvements and additions to COPY progress reporting

    Josef Šimánek <josef.simanek@gmail.com> — 2021-02-09T07:02:55Z

    po 8. 2. 2021 v 19:35 odesílatel Matthias van de Meent
<boekewurm+postgres@gmail.com> napsal:
>
> Hi,
>
> With [0] we got COPY progress reporting. Before the column names of
> this newly added view are effectively set in stone with the release of
> pg14, I propose the following set of relatively small patches. These
> are v2, because it is a patchset that is based on a set of patches
> that I previously posted in [0].

Hello. I had this in my backlog to revisit this feature as well before
the release. Thanks for picking this up.

> 0001 Adds a column to pg_stat_progress_copy which details the amount
> of tuples that were excluded from insertion by the WHERE clause of the
> COPY FROM command.
>
> 0002 alters pg_stat_progress_copy to use 'tuple'-terminology instead
> of 'line'-terminology. 'Line' doesn't make sense in the binary copy
> case, and only for the 'text' copy format there can be a guarantee
> that the source / output file actually contains the reported amount of
> lines, whereas the amount of data tuples (which is also what it's
> called internally) is guaranteed to equal for all data types.
>
> There was some discussion about this in [0] where the author thought
> 'line' is more consistent with the CSV documentation, and where I
> argued that 'tuple' is both more consistent with the rest of the
> progress reporting tables and more consistent with the actual counted
> items: these are the tuples serialized / inserted (as noted in the CSV
> docs; "Thus the files are not strictly one line per table row like
> text-format files.").

As an mentioned author I have no preference over line or tuple
terminology here. For some cases "line" terminology fits better, for
some "tuple" one. Docs can be improved later if needed to make it
clear at some cases (for example in most common case probably - CSV
import/export) tuple equals one line in CSV.

> Patch 0003 adds backlinks to the progress reporting docs from the docs
> of the commands that have progress reporting (re/index, cluster,
> vacuum, etc.) such that progress reporting is better discoverable from
> the relevant commands, and removes the datname column from the
> progress_copy view (that column was never committed). This too should
> be fairly trivial and uncontroversial.
>
> 0004 adds the 'command' column to the progress_copy view; which
> distinguishes between COPY FROM and COPY TO. The two commands are (in
> my opinion) significantly different enough to warrant this column;
> similar to the difference between CREATE INDEX/REINDEX [CONCURRENTLY]
> which also report that information. I believe that this change is
> appropriate; as the semantics of the columns change depending on the
> command being executed.

This was part of my initial patch as well, but I decided to strip it
out to make the final patch as small as possible to make it quickly
mergeable without need of further discussion. From my side this is
useful to have directly in the progress report as well.

> Lastly, 0005 adds 'io_target' to the reported information, that is,
> FILE, PROGRAM, STDIO or CALLBACK. Although this can relatively easily
> be determined based on the commands in pg_stat_activity, it is
> reasonably something that a user would want to query on, as the
> origin/target of COPY has security and performance implications,
> whereas other options (e.g. format) are less interesting for clients
> that are not executing that specific COPY command.

Similar (simplified, not supporting CALLBACK) info was also part of
the initial patch and stripped out later. I'm also +1 on this info
being useful to have directly in the progress report.

> Of special interest in 0005 is that it reports the io_target for the
> logical replications' initial tablesyncs' internal COPY. This would
> otherwise be measured, but no knowledge about the type of copy (or its
> origin) would be available on the worker's side. I'm not married to
> this patch 0005, but I believe it could be useful, and therefore
> included it in the patchset.

All patches seem good to me. I was able to apply them to current clean
master and "make check" has succeeded without problems.

>
> With regards,
>
> Matthias van de Meent.
>
>
> [0] https://www.postgresql.org/message-id/flat/CAFp7Qwr6_FmRM6pCO0x_a0mymOfX_Gg%2BFEKet4XaTGSW%3DLitKQ%40mail.gmail.com

  3. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-09T07:12:32Z

    On Tue, Feb 9, 2021 at 12:06 AM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> With [0] we got COPY progress reporting. Before the column names of
> this newly added view are effectively set in stone with the release of
> pg14, I propose the following set of relatively small patches. These
> are v2, because it is a patchset that is based on a set of patches
> that I previously posted in [0].

Thanks for working on the patches. Here are some comments:

0001 - +1 to add tuples_excluded and the patch LGTM.

0002 - Yes, the tuples_processed or tuples_excluded makes more sense
to me than lines_processed and lines_excluded. The patch LGTM.

0003 - Instead of just adding the progress reporting to "See also"
sections in the footer of the respective pages analyze, cluster and
others, it would be nice if we have a mention of it in the description
as pg_basebackup has something like below:
 <para>
   Whenever <application>pg_basebackup</application> is taking a base
   backup, the server's <structname>pg_stat_progress_basebackup</structname>
   view will report the progress of the backup.
   See <xref linkend="basebackup-progress-reporting"/> for details.

0004 -
1) How about PROGRESS_COPY_COMMAND_TYPE instead of
PROGRESS_COPY_COMMAND? The names looks bit confusing with the existing
PROGRESS_COMMAND_COPY.

0005 -
1) How about
+       or <literal>CALLBACK</literal> (used in the table
synchronization background
+       worker).
instead of
+       or <literal>CALLBACK</literal> (used in the tablesync background
+       worker).
Because "table synchronization" is being used in logical-replication.sgml.

2) I think cstate->copy_src = COPY_CALLBACK is assigned after the
switch case added in copyfrom.c
    if (data_source_cb)
    {
        cstate->copy_src = COPY_CALLBACK;
        cstate->data_source_cb = data_source_cb;
    }

Also, you can add this to the current commitfest.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  4. Re: Improvements and additions to COPY progress reporting

    Josef Šimánek <josef.simanek@gmail.com> — 2021-02-09T08:32:48Z

    po 8. 2. 2021 v 19:35 odesílatel Matthias van de Meent
<boekewurm+postgres@gmail.com> napsal:
>
> Hi,
>
> With [0] we got COPY progress reporting. Before the column names of
> this newly added view are effectively set in stone with the release of
> pg14, I propose the following set of relatively small patches. These
> are v2, because it is a patchset that is based on a set of patches
> that I previously posted in [0].
>
> 0001 Adds a column to pg_stat_progress_copy which details the amount
> of tuples that were excluded from insertion by the WHERE clause of the
> COPY FROM command.
>
> 0002 alters pg_stat_progress_copy to use 'tuple'-terminology instead
> of 'line'-terminology. 'Line' doesn't make sense in the binary copy
> case, and only for the 'text' copy format there can be a guarantee
> that the source / output file actually contains the reported amount of
> lines, whereas the amount of data tuples (which is also what it's
> called internally) is guaranteed to equal for all data types.
>
> There was some discussion about this in [0] where the author thought
> 'line' is more consistent with the CSV documentation, and where I
> argued that 'tuple' is both more consistent with the rest of the
> progress reporting tables and more consistent with the actual counted
> items: these are the tuples serialized / inserted (as noted in the CSV
> docs; "Thus the files are not strictly one line per table row like
> text-format files.").
>
> Patch 0003 adds backlinks to the progress reporting docs from the docs
> of the commands that have progress reporting (re/index, cluster,
> vacuum, etc.) such that progress reporting is better discoverable from
> the relevant commands, and removes the datname column from the
> progress_copy view (that column was never committed). This too should
> be fairly trivial and uncontroversial.
>
> 0004 adds the 'command' column to the progress_copy view; which
> distinguishes between COPY FROM and COPY TO. The two commands are (in
> my opinion) significantly different enough to warrant this column;
> similar to the difference between CREATE INDEX/REINDEX [CONCURRENTLY]
> which also report that information. I believe that this change is
> appropriate; as the semantics of the columns change depending on the
> command being executed.
>
> Lastly, 0005 adds 'io_target' to the reported information, that is,
> FILE, PROGRAM, STDIO or CALLBACK. Although this can relatively easily
> be determined based on the commands in pg_stat_activity, it is
> reasonably something that a user would want to query on, as the
> origin/target of COPY has security and performance implications,
> whereas other options (e.g. format) are less interesting for clients
> that are not executing that specific COPY command.

I took a little deeper look and I'm not sure if I understand FILE and
STDIO. I have finally tried to finalize some initial regress testing
of COPY command progress using triggers. I have attached the initial
patch  applicable to your changes. As you can see COPY FROM STDIN is
reported as FILE. That's probably expected, but it is a little
confusing for me since STDIN and STDIO sound similar. What is the
purpose of STDIO? When is the COPY command reported with io_target of
STDIO?

> Of special interest in 0005 is that it reports the io_target for the
> logical replications' initial tablesyncs' internal COPY. This would
> otherwise be measured, but no knowledge about the type of copy (or its
> origin) would be available on the worker's side. I'm not married to
> this patch 0005, but I believe it could be useful, and therefore
> included it in the patchset.
>
>
> With regards,
>
> Matthias van de Meent.
>
>
> [0] https://www.postgresql.org/message-id/flat/CAFp7Qwr6_FmRM6pCO0x_a0mymOfX_Gg%2BFEKet4XaTGSW%3DLitKQ%40mail.gmail.com

  5. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm@gmail.com> — 2021-02-09T11:51:20Z

    On Tue, 9 Feb 2021 at 09:32, Josef Šimánek <josef.simanek@gmail.com> wrote:
>
> po 8. 2. 2021 v 19:35 odesílatel Matthias van de Meent
> <boekewurm+postgres@gmail.com> napsal:
> > Lastly, 0005 adds 'io_target' to the reported information, that is,
> > FILE, PROGRAM, STDIO or CALLBACK. Although this can relatively easily
> > be determined based on the commands in pg_stat_activity, it is
> > reasonably something that a user would want to query on, as the
> > origin/target of COPY has security and performance implications,
> > whereas other options (e.g. format) are less interesting for clients
> > that are not executing that specific COPY command.
>
> I took a little deeper look and I'm not sure if I understand FILE and
> STDIO. I have finally tried to finalize some initial regress testing
> of COPY command progress using triggers. I have attached the initial
> patch  applicable to your changes. As you can see COPY FROM STDIN is
> reported as FILE. That's probably expected, but it is a little
> confusing for me since STDIN and STDIO sound similar. What is the
> purpose of STDIO? When is the COPY command reported with io_target of
> STDIO?

I checked for the type of the copy_src before it was correctly set,
therefore only reporting FILE type, but this will be fixed shortly in
v3.

Matthias

  6. Re: Improvements and additions to COPY progress reporting

    Josef Šimánek <josef.simanek@gmail.com> — 2021-02-09T11:53:44Z

    út 9. 2. 2021 v 12:51 odesílatel 0010203112132233 <boekewurm@gmail.com> napsal:
>
> On Tue, 9 Feb 2021 at 09:32, Josef Šimánek <josef.simanek@gmail.com> wrote:
> >
> > po 8. 2. 2021 v 19:35 odesílatel Matthias van de Meent
> > <boekewurm+postgres@gmail.com> napsal:
> > > Lastly, 0005 adds 'io_target' to the reported information, that is,
> > > FILE, PROGRAM, STDIO or CALLBACK. Although this can relatively easily
> > > be determined based on the commands in pg_stat_activity, it is
> > > reasonably something that a user would want to query on, as the
> > > origin/target of COPY has security and performance implications,
> > > whereas other options (e.g. format) are less interesting for clients
> > > that are not executing that specific COPY command.
> >
> > I took a little deeper look and I'm not sure if I understand FILE and
> > STDIO. I have finally tried to finalize some initial regress testing
> > of COPY command progress using triggers. I have attached the initial
> > patch  applicable to your changes. As you can see COPY FROM STDIN is
> > reported as FILE. That's probably expected, but it is a little
> > confusing for me since STDIN and STDIO sound similar. What is the
> > purpose of STDIO? When is the COPY command reported with io_target of
> > STDIO?
>
> I checked for the type of the copy_src before it was correctly set,
> therefore only reporting FILE type, but this will be fixed shortly in
> v3.

OK, would you mind to integrate my regression test initial patch as
well in v3 or should I submit it later in a separate way?

> Matthias

  7. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-02-09T12:32:33Z

    On Tue, 9 Feb 2021 at 08:12, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Feb 9, 2021 at 12:06 AM Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> > With [0] we got COPY progress reporting. Before the column names of
> > this newly added view are effectively set in stone with the release of
> > pg14, I propose the following set of relatively small patches. These
> > are v2, because it is a patchset that is based on a set of patches
> > that I previously posted in [0].
>
> Thanks for working on the patches. Here are some comments:
>
> 0001 - +1 to add tuples_excluded and the patch LGTM.
>
> 0002 - Yes, the tuples_processed or tuples_excluded makes more sense
> to me than lines_processed and lines_excluded. The patch LGTM.
>
> 0003 - Instead of just adding the progress reporting to "See also"
> sections in the footer of the respective pages analyze, cluster and
> others, it would be nice if we have a mention of it in the description
> as pg_basebackup has something like below:
>  <para>
>    Whenever <application>pg_basebackup</application> is taking a base
>    backup, the server's <structname>pg_stat_progress_basebackup</structname>
>    view will report the progress of the backup.
>    See <xref linkend="basebackup-progress-reporting"/> for details.

Added

> 0004 -
> 1) How about PROGRESS_COPY_COMMAND_TYPE instead of
> PROGRESS_COPY_COMMAND? The names looks bit confusing with the existing
> PROGRESS_COMMAND_COPY.

The current name is consistent with the naming of the other
command-reporting progress views; CREATEIDX and CLUSTER both use the
*_COMMAND as this column indexes' internal name.

> 0005 -
> 1) How about
> +       or <literal>CALLBACK</literal> (used in the table
> synchronization background
> +       worker).
> instead of
> +       or <literal>CALLBACK</literal> (used in the tablesync background
> +       worker).
> Because "table synchronization" is being used in logical-replication.sgml.

Fixed

> 2) I think cstate->copy_src = COPY_CALLBACK is assigned after the
> switch case added in copyfrom.c
>     if (data_source_cb)
>     {
>         cstate->copy_src = COPY_CALLBACK;
>         cstate->data_source_cb = data_source_cb;
>     }

Yes, I noticed this too while working on the patchset, but apparently
didn't act on this... Fixed in attachted version.

> Also, you can add this to the current commitfest.

See https://commitfest.postgresql.org/32/2977/

On Tue, 9 Feb 2021 at 12:53, Josef Šimánek <josef.simanek@gmail.com> wrote:
>
> OK, would you mind to integrate my regression test initial patch as
> well in v3 or should I submit it later in a separate way?

Attached, with minor fixes


With regards,

Matthias van de Meent

  8. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-10T06:43:08Z

    On Tue, Feb 9, 2021 at 6:02 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> > Also, you can add this to the current commitfest.
>
> See https://commitfest.postgresql.org/32/2977/
>
> On Tue, 9 Feb 2021 at 12:53, Josef Šimánek <josef.simanek@gmail.com> wrote:
> >
> > OK, would you mind to integrate my regression test initial patch as
> > well in v3 or should I submit it later in a separate way?
>
> Attached, with minor fixes

Why do we need to have a new test file progress.sql for the test
cases? Can't we add them into existing copy.sql or copy2.sql? Or do
you have a plan to add test cases into progress.sql for other progress
reporting commands?

IMO, it's better not add any new test file but add the tests to existing files.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  9. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-02-11T14:27:15Z

    On Wed, 10 Feb 2021 at 07:43, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Feb 9, 2021 at 6:02 PM Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> > > Also, you can add this to the current commitfest.
> >
> > See https://commitfest.postgresql.org/32/2977/
> >
> > On Tue, 9 Feb 2021 at 12:53, Josef Šimánek <josef.simanek@gmail.com> wrote:
> > >
> > > OK, would you mind to integrate my regression test initial patch as
> > > well in v3 or should I submit it later in a separate way?
> >
> > Attached, with minor fixes
>
> Why do we need to have a new test file progress.sql for the test
> cases? Can't we add them into existing copy.sql or copy2.sql? Or do
> you have a plan to add test cases into progress.sql for other progress
> reporting commands?

I don't mind moving the test into copy or copy2, but the main reason
to put it in a seperate file is to test the 'copy' component of the
feature called 'progress reporting'. If the feature instead is 'copy'
and 'progress reporting' is part of that feature, then I'd put it in
the copy-tests, but because the documentation of this has it's own
docs page  'progress reporting', and because 'copy' is a subsection of
that, I do think that this feature warrants its own regression test
file.

There are no other tests for the progress reporting feature yet,
because COPY ... FROM is the only command that is progress reported
_and_ that can fire triggers while running the command, so checking
the progress view during the progress reported command is only
feasable in COPY progress reporting. To test the other progress
reporting views, we would need multiple sessions, which I believe is
impossible in this test format. Please correct me if I'm wrong; I'd
love to add tests for the other components. That will not be in this
patchset, though.

> IMO, it's better not add any new test file but add the tests to existing files.

In general I agree, but in some cases (e.g. new system component, new
full-fledged feature), new test files are needed. I think that this
could be one of those cases.


With regards,

Matthias van de Meent

  10. Re: Improvements and additions to COPY progress reporting

    Josef Šimánek <josef.simanek@gmail.com> — 2021-02-11T14:38:42Z

    čt 11. 2. 2021 v 15:27 odesílatel Matthias van de Meent
<boekewurm+postgres@gmail.com> napsal:
>
> On Wed, 10 Feb 2021 at 07:43, Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Tue, Feb 9, 2021 at 6:02 PM Matthias van de Meent
> > <boekewurm+postgres@gmail.com> wrote:
> > > > Also, you can add this to the current commitfest.
> > >
> > > See https://commitfest.postgresql.org/32/2977/
> > >
> > > On Tue, 9 Feb 2021 at 12:53, Josef Šimánek <josef.simanek@gmail.com> wrote:
> > > >
> > > > OK, would you mind to integrate my regression test initial patch as
> > > > well in v3 or should I submit it later in a separate way?
> > >
> > > Attached, with minor fixes
> >
> > Why do we need to have a new test file progress.sql for the test
> > cases? Can't we add them into existing copy.sql or copy2.sql? Or do
> > you have a plan to add test cases into progress.sql for other progress
> > reporting commands?
>
> I don't mind moving the test into copy or copy2, but the main reason
> to put it in a seperate file is to test the 'copy' component of the
> feature called 'progress reporting'. If the feature instead is 'copy'
> and 'progress reporting' is part of that feature, then I'd put it in
> the copy-tests, but because the documentation of this has it's own
> docs page  'progress reporting', and because 'copy' is a subsection of
> that, I do think that this feature warrants its own regression test
> file.
>
> There are no other tests for the progress reporting feature yet,
> because COPY ... FROM is the only command that is progress reported
> _and_ that can fire triggers while running the command, so checking
> the progress view during the progress reported command is only
> feasable in COPY progress reporting. To test the other progress
> reporting views, we would need multiple sessions, which I believe is
> impossible in this test format. Please correct me if I'm wrong; I'd
> love to add tests for the other components. That will not be in this
> patchset, though.
>
> > IMO, it's better not add any new test file but add the tests to existing files.
>
> In general I agree, but in some cases (e.g. new system component, new
> full-fledged feature), new test files are needed. I think that this
> could be one of those cases.

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.

>
> With regards,
>
> Matthias van de Meent

  11. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-11T14:44:37Z

    On Thu, Feb 11, 2021, 8:08 PM Josef Šimánek <josef.simanek@gmail.com> wrote:

> čt 11. 2. 2021 v 15:27 odesílatel Matthias van de Meent
> <boekewurm+postgres@gmail.com> napsal:
> >
> > On Wed, 10 Feb 2021 at 07:43, Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Tue, Feb 9, 2021 at 6:02 PM Matthias van de Meent
> > > <boekewurm+postgres@gmail.com> wrote:
> > > > > Also, you can add this to the current commitfest.
> > > >
> > > > See https://commitfest.postgresql.org/32/2977/
> > > >
> > > > On Tue, 9 Feb 2021 at 12:53, Josef Šimánek <josef.simanek@gmail.com>
> wrote:
> > > > >
> > > > > OK, would you mind to integrate my regression test initial patch as
> > > > > well in v3 or should I submit it later in a separate way?
> > > >
> > > > Attached, with minor fixes
> > >
> > > Why do we need to have a new test file progress.sql for the test
> > > cases? Can't we add them into existing copy.sql or copy2.sql? Or do
> > > you have a plan to add test cases into progress.sql for other progress
> > > reporting commands?
> >
> > I don't mind moving the test into copy or copy2, but the main reason
> > to put it in a seperate file is to test the 'copy' component of the
> > feature called 'progress reporting'. If the feature instead is 'copy'
> > and 'progress reporting' is part of that feature, then I'd put it in
> > the copy-tests, but because the documentation of this has it's own
> > docs page  'progress reporting', and because 'copy' is a subsection of
> > that, I do think that this feature warrants its own regression test
> > file.
> >
> > There are no other tests for the progress reporting feature yet,
> > because COPY ... FROM is the only command that is progress reported
> > _and_ that can fire triggers while running the command, so checking
> > the progress view during the progress reported command is only
> > feasable in COPY progress reporting. To test the other progress
> > reporting views, we would need multiple sessions, which I believe is
> > impossible in this test format. Please correct me if I'm wrong; I'd
> > love to add tests for the other components. That will not be in this
> > patchset, though.
> >
> > > IMO, it's better not add any new test file but add the tests to
> existing files.
> >
> > In general I agree, but in some cases (e.g. new system component, new
> > full-fledged feature), new test files are needed. I think that this
> > could be one of those cases.
>
> 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.

  12. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-02-12T11:23:34Z

    On Thu, 11 Feb 2021 at 15:44, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
>
> On Thu, Feb 11, 2021, 8:08 PM Josef Šimánek <josef.simanek@gmail.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'.

I've changed the notice-alerted format from manually naming each
column to calling to_jsonb and removing the unstable columns from the
reported value; this should therefore be stable and give direct notice
to changes in the view.

With regards,

Matthias van de Meent.

  13. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-02-12T12:10:46Z

    On Fri, 12 Feb 2021 at 12:23, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
>
> On Thu, 11 Feb 2021 at 15:44, Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> >
> > On Thu, Feb 11, 2021, 8:08 PM Josef Šimánek <josef.simanek@gmail.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@. I had the changes staged, but
forgot to add them to the patches.

Sorry for the noise.

-Matthias

  14. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-12T12:40:28Z

    On Fri, Feb 12, 2021 at 5:40 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
>
> On Fri, 12 Feb 2021 at 12:23, Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> >
> > On Thu, 11 Feb 2021 at 15:44, Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > >
> > > On Thu, Feb 11, 2021, 8:08 PM Josef Šimánek <josef.simanek@gmail.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@. 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:

postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid |  command  | io_target |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+-----------+-----------+-----------------+-------------+------------------+-----------------
 2886103 | 12977 | postgres | 16384 | COPY FROM | FILE      |
83099648 |    85777795 |          9553999 |         1111111
(1 row)

postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid |  command  | io_target |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+-----------+-----------+-----------------+-------------+------------------+-----------------
 2886103 | 12977 | postgres | 16384 | COPY FROM | STDIO     |
     0 |           0 |                0 |               0
(1 row)

postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid | command | io_target |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+---------+-----------+-----------------+-------------+------------------+-----------------
 2886103 | 12977 | postgres | 16384 | COPY TO | FILE      |
37771610 |           0 |          4999228 |               0
(1 row)

postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid |  command  | io_target |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+-----------+-----------+-----------------+-------------+------------------+-----------------
 2892816 | 12977 | postgres | 16384 | COPY FROM | CALLBACK  |
249777823 |           0 |         31888892 |               0
(1 row)

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  15. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-02-12T17:47:06Z

    On Fri, 12 Feb 2021 at 13:40, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Feb 12, 2021 at 5:40 PM Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> >
> > On Fri, 12 Feb 2021 at 12:23, Matthias van de Meent
> > <boekewurm+postgres@gmail.com> wrote:
> > >
> > > On Thu, 11 Feb 2021 at 15:44, Bharath Rupireddy
> > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > >
> > > >
> > > > On Thu, Feb 11, 2021, 8:08 PM Josef Šimánek <josef.simanek@gmail.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@. 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.

  16. Re: Improvements and additions to COPY progress reporting

    Justin Pryzby <pryzby@telsasoft.com> — 2021-02-13T00:37:32Z

    --- a/doc/src/sgml/ref/analyze.sgml                                                                                                                                                                                            
+++ b/doc/src/sgml/ref/analyze.sgml                                                                                                                                                                                            
@@ -273,6 +273,12 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea                                                                                                                          
     will not record new statistics for that table.  Any existing statistics                                                                                                                                                   
     will be retained.                                                                                                                                                                                                         
   </para>                                                                                                                                                                                                                     
+                                                                                                                                                                                                                              
+  <para>                                                                                                                                                                                                                      
+    Each backend running the <command>ANALYZE</command> command will report their                                                                                                                                             
+    progress to the <structname>pg_stat_progress_analyze</structname> view.                                                                                                                                                   
+    See <xref linkend="analyze-progress-reporting"/> for details.                                                                                                                                                             
+  </para>                                                                                                                                                                                                                     

I think this should say:

"..will report its progress to.."

Or:

"The progress of each backend running >ANALYZE< is reported in the
>pg_stat_progress_analyze< view."

-- 
Justin

  17. Re: Improvements and additions to COPY progress reporting

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-02-15T16:07:11Z

    Hi,

I agree with these changes in general - I have a couple minor comment:

1) 0001

- the SGML docs are missing a couple tags

- The blocks in copyfrom.cc/copyto.c should be reworked - I don't think
we do this in our codebase. Move the variable declarations to the
beginning, get rid of the out block. Or something like that.

- I fir the "io_target" name misleading, because in some cases it's
actually the *source*.


2) 0002

- I believe "each backend ... reports its" (not theirs), right?

- This seems more like a generic docs improvement, not quite specific to
the COPY progress patch. It's a bit buried, maybe it should be posted
separately. OTOH it's pretty small.


3) 0003

- Some whitespace noise, triggering "git am" warnings.

- Might be good to briefly explain what the regression test does with
the triggers, etc.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  18. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-02-18T15:46:58Z

    Hi,

Thank you all for the suggestions. PFA version 8 of the patchset, in
which I have applied most of your comments. Unless explicitly named
below, I have applied the suggestions.


On Mon, 15 Feb 2021 at 17:07, Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> - The blocks in copyfrom.cc/copyto.c should be reworked - I don't think
> we do this in our codebase.

I saw this being used in (re)index progress reporting, that's where I
took inspiration from. It has been fixed in the attached version.

> - I fir the "io_target" name misleading, because in some cases it's
> actually the *source*.

Yes, I was also not quite happy with this, but couldn't find a better
one at the point of writing the initial patchset. Would
"io_operations", "io_port", "operates_through" or "through" maybe be
better?


With regards,

Matthias van de Meent

  19. Re: Improvements and additions to COPY progress reporting

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-02-18T21:03:56Z

    
On 2/18/21 4:46 PM, Matthias van de Meent wrote:
> Hi,
> 
> Thank you all for the suggestions. PFA version 8 of the patchset, in
> which I have applied most of your comments. Unless explicitly named
> below, I have applied the suggestions.
> 

Thanks.

> 
> On Mon, 15 Feb 2021 at 17:07, Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> - The blocks in copyfrom.cc/copyto.c should be reworked - I don't think
>> we do this in our codebase.
> 
> I saw this being used in (re)index progress reporting, that's where I
> took inspiration from. It has been fixed in the attached version.
> 

Hmmm, good point. I haven't looked at the other places reporting
progress and I only ever saw this pattern in old code. I kinda dislike
these blocks, but admittedly that's rather subjective view. So if other
similar places do this when reporting progress, this probably should
too. What's your opinion on this?

>> - I fir the "io_target" name misleading, because in some cases it's
>> actually the *source*.
> 
> Yes, I was also not quite happy with this, but couldn't find a better
> one at the point of writing the initial patchset. Would
> "io_operations", "io_port", "operates_through" or "through" maybe be
> better?
> 

No idea. Let's see if someone has a better proposal ...


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  20. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-20T06:09:22Z

    On Fri, Feb 19, 2021 at 2:34 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
> > On Mon, 15 Feb 2021 at 17:07, Tomas Vondra
> > <tomas.vondra@enterprisedb.com> wrote:
> >>
> >> - The blocks in copyfrom.cc/copyto.c should be reworked - I don't think
> >> we do this in our codebase.
> >
> > I saw this being used in (re)index progress reporting, that's where I
> > took inspiration from. It has been fixed in the attached version.
> >
>
> Hmmm, good point. I haven't looked at the other places reporting
> progress and I only ever saw this pattern in old code. I kinda dislike
> these blocks, but admittedly that's rather subjective view. So if other
> similar places do this when reporting progress, this probably should
> too. What's your opinion on this?

Actually in the code base the style of that variable declaration and
usage of pgstat_progress_update_multi_param is a mix. For instance, in
lazy_scan_heap, ReindexRelationConcurrently, the variables are
declared at the start of the function. And in _bt_spools_heapscan,
index_build, validate_index, perform_base_backup, the variables are
declared within a separate block.

IMO, we can have the arrays declared at the start of the functions
i.e. the way it's done in v8-0001, because we can extend them for
reporting some other parameter(maybe in future).

> >> - I fir the "io_target" name misleading, because in some cases it's
> >> actually the *source*.
> >
> > Yes, I was also not quite happy with this, but couldn't find a better
> > one at the point of writing the initial patchset. Would
> > "io_operations", "io_port", "operates_through" or "through" maybe be
> > better?
> >
>
> No idea. Let's see if someone has a better proposal ...

 For COPY TO the name "source_type" column and for COPY FROM the name
"destination_type" makes sense. To have a combined column name for
both, how about naming that column as "io_type"?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  21. Re: Improvements and additions to COPY progress reporting

    Michael Paquier <michael@paquier.xyz> — 2021-02-20T07:19:20Z

    On Sat, Feb 20, 2021 at 11:39:22AM +0530, Bharath Rupireddy wrote:
> Actually in the code base the style of that variable declaration and
> usage of pgstat_progress_update_multi_param is a mix. For instance, in
> lazy_scan_heap, ReindexRelationConcurrently, the variables are
> declared at the start of the function. And in _bt_spools_heapscan,
> index_build, validate_index, perform_base_backup, the variables are
> declared within a separate block.

I think that we should encourage the use of
pgstat_progress_update_multi_param() where we can, as it makes
consistent the updates to all the parameters according to
st_changecount.  That's also usually cleaner to store all the
parameters that are changed if these are updated multiple times like
the REINDEX CONCURRENTLY ones.  The context of the code also matters,
of course.

Scanning through the patch set, 0002 is a good idea taken
independently.
--
Michael

  22. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-20T08:59:44Z

    On Sat, Feb 20, 2021 at 12:49 PM Michael Paquier <michael@paquier.xyz>
wrote:
>
> On Sat, Feb 20, 2021 at 11:39:22AM +0530, Bharath Rupireddy wrote:
> > Actually in the code base the style of that variable declaration and
> > usage of pgstat_progress_update_multi_param is a mix. For instance, in
> > lazy_scan_heap, ReindexRelationConcurrently, the variables are
> > declared at the start of the function. And in _bt_spools_heapscan,
> > index_build, validate_index, perform_base_backup, the variables are
> > declared within a separate block.
>
> I think that we should encourage the use of
> pgstat_progress_update_multi_param() where we can, as it makes
> consistent the updates to all the parameters according to
> st_changecount.  That's also usually cleaner to store all the
> parameters that are changed if these are updated multiple times like
> the REINDEX CONCURRENTLY ones.  The context of the code also matters,
> of course.

Yeah. We could use pgstat_progress_update_multi_param instead of
pgstat_progress_update_param to update multiple params.

On a quick scan through the code, I found that we can do the following. If
okay, I can start a new thread so that we don't divert the main thread
here. Thoughts?

@@ -3686,12 +3686,18 @@ reindex_index(Oid indexId, bool
skip_constraint_checks, char persistence,
      if (progress)
     {
+        const int    progress_cols[] = {
+            PROGRESS_CREATEIDX_COMMAND,
+            PROGRESS_CREATEIDX_INDEX_OID
+        };
+        const int64    progress_vals[] = {
+            PROGRESS_CREATEIDX_COMMAND_REINDEX,
+            indexId
+        };
+
         pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
                                       heapId);
-        pgstat_progress_update_param(PROGRESS_CREATEIDX_COMMAND,
-                                     PROGRESS_CREATEIDX_COMMAND_REINDEX);
-        pgstat_progress_update_param(PROGRESS_CREATEIDX_INDEX_OID,
-                                     indexId);
+        pgstat_progress_update_multi_param(2, progress_cols,
progress_vals);
     }
@@ -1457,10 +1457,21 @@ DefineIndex(Oid relationId,
         set_indexsafe_procflags();
     /*
-     * The index is now visible, so we can report the OID.
+     * The index is now visible, so we can report the OID. And also, report
+     * Phase 2 of concurrent index build.
      */
-    pgstat_progress_update_param(PROGRESS_CREATEIDX_INDEX_OID,
-                                 indexRelationId);
+    {
+        const int    progress_cols[] = {
+            PROGRESS_CREATEIDX_INDEX_OID,
+            PROGRESS_CREATEIDX_PHASE
+        };
+        const int64    progress_vals[] = {
+            indexRelationId,
+            PROGRESS_CREATEIDX_PHASE_WAIT_1
+        };
+
+        pgstat_progress_update_multi_param(2, progress_cols,
progress_vals);
+    }
@@ -284,12 +284,9 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams
*params)
     CHECK_FOR_INTERRUPTS();
      pgstat_progress_start_command(PROGRESS_COMMAND_CLUSTER, tableOid);
-    if (OidIsValid(indexOid))
-        pgstat_progress_update_param(PROGRESS_CLUSTER_COMMAND,
-                                     PROGRESS_CLUSTER_COMMAND_CLUSTER);
-    else
-        pgstat_progress_update_param(PROGRESS_CLUSTER_COMMAND,
-                                     PROGRESS_CLUSTER_COMMAND_VACUUM_FULL);
+    pgstat_progress_update_param(PROGRESS_CLUSTER_COMMAND,
+                                 OidIsValid(indexOid) ?
PROGRESS_CLUSTER_COMMAND_CLUSTER :
+                                 PROGRESS_CLUSTER_COMMAND_VACUUM_FULL);

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  23. Re: Improvements and additions to COPY progress reporting

    Josef Šimánek <josef.simanek@gmail.com> — 2021-02-20T13:17:30Z

    so 20. 2. 2021 v 7:09 odesílatel Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> napsal:
>
> On Fri, Feb 19, 2021 at 2:34 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
> > > On Mon, 15 Feb 2021 at 17:07, Tomas Vondra
> > > <tomas.vondra@enterprisedb.com> wrote:
> > >>
> > >> - The blocks in copyfrom.cc/copyto.c should be reworked - I don't think
> > >> we do this in our codebase.
> > >
> > > I saw this being used in (re)index progress reporting, that's where I
> > > took inspiration from. It has been fixed in the attached version.
> > >
> >
> > Hmmm, good point. I haven't looked at the other places reporting
> > progress and I only ever saw this pattern in old code. I kinda dislike
> > these blocks, but admittedly that's rather subjective view. So if other
> > similar places do this when reporting progress, this probably should
> > too. What's your opinion on this?
>
> Actually in the code base the style of that variable declaration and
> usage of pgstat_progress_update_multi_param is a mix. For instance, in
> lazy_scan_heap, ReindexRelationConcurrently, the variables are
> declared at the start of the function. And in _bt_spools_heapscan,
> index_build, validate_index, perform_base_backup, the variables are
> declared within a separate block.
>
> IMO, we can have the arrays declared at the start of the functions
> i.e. the way it's done in v8-0001, because we can extend them for
> reporting some other parameter(maybe in future).
>
> > >> - I fir the "io_target" name misleading, because in some cases it's
> > >> actually the *source*.
> > >
> > > Yes, I was also not quite happy with this, but couldn't find a better
> > > one at the point of writing the initial patchset. Would
> > > "io_operations", "io_port", "operates_through" or "through" maybe be
> > > better?
> > >
> >
> > No idea. Let's see if someone has a better proposal ...
>
>  For COPY TO the name "source_type" column and for COPY FROM the name
> "destination_type" makes sense. To have a combined column name for
> both, how about naming that column as "io_type"?

+1 on "io_type", that is my best candidate as well

> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com

  24. Re: Improvements and additions to COPY progress reporting

    Michael Paquier <michael@paquier.xyz> — 2021-02-20T23:23:02Z

    On Sat, Feb 20, 2021 at 02:29:44PM +0530, Bharath Rupireddy wrote:
> Yeah. We could use pgstat_progress_update_multi_param instead of
> pgstat_progress_update_param to update multiple params.
> 
> On a quick scan through the code, I found that we can do the following. If
> okay, I can start a new thread so that we don't divert the main thread
> here. Thoughts?

Having a separate thread to discuss this part would be right.  This
way any patches sent would attract the correct audience.
--
Michael

  25. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-02-21T19:10:09Z

    On Sat, 20 Feb 2021 at 07:09, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
>  For COPY TO the name "source_type" column and for COPY FROM the name
> "destination_type" makes sense. To have a combined column name for
> both, how about naming that column as "io_type"?

Thank you, that's way better! PFA what I believe is a finalized
patchset v9, utilizing io_type terminology instead of io_target.


With regards,

Matthias van de Meent

  26. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-22T04:49:32Z

    On Mon, Feb 22, 2021 at 12:40 AM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
>
> On Sat, 20 Feb 2021 at 07:09, Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> >  For COPY TO the name "source_type" column and for COPY FROM the name
> > "destination_type" makes sense. To have a combined column name for
> > both, how about naming that column as "io_type"?
>
> Thank you, that's way better! PFA what I believe is a finalized
> patchset v9, utilizing io_type terminology instead of io_target.

Thanks for the patches. I reviewed them.

0001 -  I think there's a bug. See COPY TO stdout doesn't print
io_type as "STDIO".

postgres=# select * from pg_stat_progress_copy;
  pid   | datid | datname  | relid | command | io_type |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
--------+-------+----------+-------+---------+---------+-----------------+-------------+------------------+-----------------
 977510 | 13003 | postgres | 16384 | COPY TO |         |
23961591 |           0 |          2662399 |               0
(1 row)

we should do below(like we do for copyfrom.c):

@@ -702,7 +710,10 @@ BeginCopyTo(ParseState *pstate,
     if (pipe)
     {
        progress_vals[1] = PROGRESS_COPY_IO_TYPE_STDIO;
         Assert(!is_program);    /* the grammar does not allow this */
         if (whereToSendOutput != DestRemote)
             cstate->copy_file = stdout;
     }

Because if "pipe" is true, that means the transfer is between STDIO.
See below comment:

 * If <pipe> is false, transfer is between the table and the file named
 * <filename>.  Otherwise, transfer is between the table and our regular
 * input/output stream. The latter could be either stdin/stdout or a
 * socket, depending on whether we're running under Postmaster control.
 *

0002 patch looks good to me.

0003 - patch:
I'm doubtful if the "bytes_total": 79 i.e. test file size will be the
same across different platforms and file systems types, if true, then
the below tests will not be stable. Do we also want to exclude the
bytes_total from the output, just to be on the safer side? Thoughts?

copy progress_reporting from stdin;
+INFO:  progress: {"command": "COPY FROM", "datname": "regression",
"io_type": "STDIO", "bytes_total": 0, "bytes_processed": 79,
"tuples_excluded": 0, "tuples_processed": 3}
+-- reporting of FILE imports, and correct reporting of tuples-excluded
+copy progress_reporting from '@abs_srcdir@/data/emp.data'
+    where (salary < 2000);
+INFO:  progress: {"command": "COPY FROM", "datname": "regression",
"io_type": "FILE", "bytes_total": 79, "bytes_processed": 79,
"tuples_excluded": 1, "tuples_processed": 2}
+-- cleanup progress_reporting

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  27. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-02-23T09:27:24Z

    On Mon, 22 Feb 2021 at 05:49, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Feb 22, 2021 at 12:40 AM Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> >
> > On Sat, 20 Feb 2021 at 07:09, Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > >  For COPY TO the name "source_type" column and for COPY FROM the name
> > > "destination_type" makes sense. To have a combined column name for
> > > both, how about naming that column as "io_type"?
> >
> > Thank you, that's way better! PFA what I believe is a finalized
> > patchset v9, utilizing io_type terminology instead of io_target.
>
> Thanks for the patches. I reviewed them.
>
> 0001 -  I think there's a bug. See COPY TO stdout doesn't print
> io_type as "STDIO".

Fixed in attached

> 0003 - patch:
> I'm doubtful if the "bytes_total": 79 i.e. test file size will be the
> same across different platforms and file systems types, if true, then
> the below tests will not be stable. Do we also want to exclude the
> bytes_total from the output, just to be on the safer side? Thoughts?

I'm fairly certain that input files of the regression tests are
considered 'binary files' to the test framework and that contents
don't change between different architectures or OSes. I also think
that any POSIX-compliant file system would report anything but the
size of the file contents, i.e. the size of the blob that is the file,
and that is correctly reported here. Other than that, if bytes_total
wouldn't be stable, then bytes_processed wouldn't make sense either.

For STDIN / STDOUT you might also have a point (different input
methods might have different length encodings for the specified
input), but insofar that I understand the test framework and the
expected git configurations, the tests run using UTF-8 / ascii only,
with a single style of newlines[+]. Sadly, I cannot provide examples
nor outputs for other test framework settings due to my lack of
experience with running the tests with non-standard settings.

Note, I'm happy to be proven wrong here, in which case I don't
disagree, but according to my limited knowledge, these outputs should
be stable.


With regards,

Matthias van de Meent

[+] Except when explicitly configured to run using non-standard
configurations, in which case there are different expected output
values to be configured for that configuration.

  28. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-24T03:12:14Z

    On Tue, Feb 23, 2021 at 2:57 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> On Mon, 22 Feb 2021 at 05:49, Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Mon, Feb 22, 2021 at 12:40 AM Matthias van de Meent
> > <boekewurm+postgres@gmail.com> wrote:
> > >
> > > On Sat, 20 Feb 2021 at 07:09, Bharath Rupireddy
> > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > >
> > > >  For COPY TO the name "source_type" column and for COPY FROM the name
> > > > "destination_type" makes sense. To have a combined column name for
> > > > both, how about naming that column as "io_type"?
> > >
> > > Thank you, that's way better! PFA what I believe is a finalized
> > > patchset v9, utilizing io_type terminology instead of io_target.
> >
> > Thanks for the patches. I reviewed them.
> >
> > 0001 -  I think there's a bug. See COPY TO stdout doesn't print
> > io_type as "STDIO".
>
> Fixed in attached

Thanks.

> > 0003 - patch:
> > I'm doubtful if the "bytes_total": 79 i.e. test file size will be the
> > same across different platforms and file systems types, if true, then
> > the below tests will not be stable. Do we also want to exclude the
> > bytes_total from the output, just to be on the safer side? Thoughts?
>
> I'm fairly certain that input files of the regression tests are
> considered 'binary files' to the test framework and that contents
> don't change between different architectures or OSes. I also think
> that any POSIX-compliant file system would report anything but the
> size of the file contents, i.e. the size of the blob that is the file,
> and that is correctly reported here. Other than that, if bytes_total
> wouldn't be stable, then bytes_processed wouldn't make sense either.
>
> For STDIN / STDOUT you might also have a point (different input
> methods might have different length encodings for the specified
> input), but insofar that I understand the test framework and the
> expected git configurations, the tests run using UTF-8 / ascii only,
> with a single style of newlines[+]. Sadly, I cannot provide examples
> nor outputs for other test framework settings due to my lack of
> experience with running the tests with non-standard settings.
>
> Note, I'm happy to be proven wrong here, in which case I don't
> disagree, but according to my limited knowledge, these outputs should
> be stable.

I'm no expert in different OS architectures, but I see that the
patches are passing on cf bot where they get tested on Windows,
FreeBSD, Linux and macOS platforms.

I have no further comments on the v10 patch set. I tested the patches,
they work as expected. I will mark the cf entry as "Ready for
Committer".

Below are some snapshots from testing:

postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid |  command  | io_type |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+-----------+---------+-----------------+-------------+------------------+-----------------
 1089927 | 13003 | postgres | 16384 | COPY FROM | FILE    |
104660992 |   888888898 |         12861112 |               0
 1089969 | 13003 | postgres | 16384 | COPY FROM | FILE    |
76611584 |   888888898 |          9712999 |               0

postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid |  command  | io_type |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+-----------+---------+-----------------+-------------+------------------+-----------------
 1089927 | 13003 | postgres | 16384 | COPY FROM | FILE    |
203161600 |   888888898 |                0 |        23804080
 1089969 | 13003 | postgres | 16384 | COPY FROM | FILE    |
150601728 |   888888898 |                0 |        17961241


 postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid | command | io_type |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+---------+---------+-----------------+-------------+------------------+-----------------
 1089927 | 13003 | postgres | 16384 | COPY TO | FILE    |
66806479 |           0 |          7422942 |               0
 1089969 | 13003 | postgres | 16384 | COPY TO | FILE    |
29803951 |           0 |          3311550 |               0

 postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid | command | io_type |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+---------+---------+-----------------+-------------+------------------+-----------------
 1089927 | 13003 | postgres | 16384 | COPY TO | STDIO   |
5998293 |           0 |           666477 |               0
 1089969 | 13003 | postgres | 16384 | COPY TO | STDIO   |
2780586 |           0 |           308954 |               0

 postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid | command | io_type |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+---------+---------+-----------------+-------------+------------------+-----------------
 1089927 | 13003 | postgres |     0 | COPY TO | FILE    |
124447239 |           0 |         13827471 |               0
 1089969 | 13003 | postgres |     0 | COPY TO | FILE    |
90992466 |           0 |         10110274 |               0

postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid |  command  | io_type |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+-----------+---------+-----------------+-------------+------------------+-----------------
 1090927 | 13003 | postgres | 16384 | COPY FROM | STDIO   |
492465897 |           0 |         55952999 |               0
 1091000 | 13003 | postgres | 16384 | COPY FROM | STDIO   |
30494360 |           0 |          3950683 |               0

 postgres=# select * from pg_stat_progress_copy;
   pid   | datid | datname  | relid |  command  | io_type |
bytes_processed | bytes_total | tuples_processed | tuples_excluded
---------+-------+----------+-------+-----------+---------+-----------------+-------------+------------------+-----------------
 1091217 | 13003 | postgres | 16384 | COPY FROM | STDIO   |
230516127 |           0 |                0 |        26847469
 1091224 | 13003 | postgres | 16384 | COPY FROM | STDIO   |
212020065 |           0 |                0 |        24792351

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  29. Re: Improvements and additions to COPY progress reporting

    Michael Paquier <michael@paquier.xyz> — 2021-02-24T07:46:57Z

    On Tue, Feb 23, 2021 at 10:27:24AM +0100, Matthias van de Meent wrote:
> Note, I'm happy to be proven wrong here, in which case I don't
> disagree, but according to my limited knowledge, these outputs should
> be stable.

I am planning to look more at 0001 and 0003, but for now I have been
looking at 0002 which is interesting on its own.

+    <structname>pg_stat_progress_vacuum</structname> view. Backends running
+    <command>VACUUM</command> with the <literal>FULL</literal> option report
+    progress in the <structname>pg_stat_progress_cluster</structname> instead.
You have missed one "view" after pg_stat_progress_cluster here.
Except that, this stuff looks fine.  So I'd like to apply it if there
are no objections.
--
Michael

  30. Re: Improvements and additions to COPY progress reporting

    Justin Pryzby <pryzby@telsasoft.com> — 2021-02-24T07:53:03Z

    On Sun, Feb 21, 2021 at 08:10:09PM +0100, Matthias van de Meent wrote:
> Subject: [PATCH v9 1/3] Add progress-reported components for COPY progress
>  reporting

>  			/* Increment amount of processed tuples and update the progress */
>  	/* Increment amount of processed tuples and update the progress */

Ideally, this would say "number of processed tuples"

> Subject: [PATCH v9 2/3] Add backlinks to progress reporting documentation

I think these should say that they report their progress *in* the view (not
"to"):

> +    Each backend running <command>ANALYZE</command> will report its progress to
> +    the <structname>pg_stat_progress_analyze</structname> view. See

> +    Each backend running <command>CLUSTER</command> will report its progress to
> +    the <structname>pg_stat_progress_cluster</structname> view. See

> +    Each backend running <command>COPY</command> will report its progress to
> +    the <structname>pg_stat_progress_copy</structname> view. See

> +    Each backend running <command>CREATE INDEX</command> will report its
> +    progress to the <structname>pg_stat_progress_create_index</structname>

> +    Each backend running <command>REINDEX</command> will report its progress
> +    to the <structname>pg_stat_progress_create_index</structname> view. See

Like this one:

> +    Each backend running <command>VACUUM</command> without the
> +    <literal>FULL</literal> option will report its progress in the

I'm sorry I didn't include that in last week's message.  You could also write:

|"The progress of each backend running >ANALYZE< is reported in the >pg_stat_progress_analyze< view."

Looking at the existing docs:

https://www.postgresql.org/docs/devel/progress-reporting.html#COPY-PROGRESS-REPORTING
| OID of the table on which the COPY command is executed

Maybe it should say ".. is executing".  Or ".. being executed":
| OID of the table on which the COPY command is being executed

-- 
Justin

  31. Re: Improvements and additions to COPY progress reporting

    Michael Paquier <michael@paquier.xyz> — 2021-02-25T08:14:35Z

    On Wed, Feb 24, 2021 at 01:53:03AM -0600, Justin Pryzby wrote:
> On Sun, Feb 21, 2021 at 08:10:09PM +0100, Matthias van de Meent wrote:
> I think these should say that they report their progress *in* the view (not
> "to"):
> 
> > +    Each backend running <command>ANALYZE</command> will report its progress to
> > +    the <structname>pg_stat_progress_analyze</structname> view. See

What is proposed in the patch is:
"Each backend running <command>blah</> will report its progress to the
pg_stat_progress_blah view."

What you propose is:
"Each backend running <command>blah</> will report its progress in the
pg_stat_progress_blah view."

What pg_basebackup tells is:
"Whenever <application>pg_basebackup</application> is taking a base
backup, the server's pg_stat_progress_basebackup view will report the
progress of the backup."

Here is an extra idea:
"Whenever a backend runs <command>blah</>, the server's
pg_stat_progress_blah will report the progress of this command."
--
Michael

  32. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-04T06:46:17Z

    On Thu, Feb 25, 2021 at 1:44 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Feb 24, 2021 at 01:53:03AM -0600, Justin Pryzby wrote:
> > On Sun, Feb 21, 2021 at 08:10:09PM +0100, Matthias van de Meent wrote:
> > I think these should say that they report their progress *in* the view (not
> > "to"):
> >
> > > +    Each backend running <command>ANALYZE</command> will report its progress to
> > > +    the <structname>pg_stat_progress_analyze</structname> view. See
>
> What is proposed in the patch is:
> "Each backend running <command>blah</> will report its progress to the
> pg_stat_progress_blah view."
>
> What you propose is:
> "Each backend running <command>blah</> will report its progress in the
> pg_stat_progress_blah view."

IMO, the phrasing proposed by Justin upthread looks good. It's like this:

> +    Each backend running <command>ANALYZE</command> will report its progress in
> +    the <structname>pg_stat_progress_analyze</structname> view. See

> +    Each backend running <command>CREATE INDEX</command> will report its
> +    progress in the <structname>pg_stat_progress_create_index</structname> view

...
...

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  33. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-04T06:51:40Z

    On Wed, Feb 24, 2021 at 1:23 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Sun, Feb 21, 2021 at 08:10:09PM +0100, Matthias van de Meent wrote:
> > Subject: [PATCH v9 1/3] Add progress-reported components for COPY progress
> >  reporting
>
> >                       /* Increment amount of processed tuples and update the progress */
> >       /* Increment amount of processed tuples and update the progress */
>
> Ideally, this would say "number of processed tuples"

Correct. It's introduced by the original COPY progress reporting
patch. Having said that, we could just correct them in the 0001 patch.

> Looking at the existing docs:
>
> https://www.postgresql.org/docs/devel/progress-reporting.html#COPY-PROGRESS-REPORTING
> | OID of the table on which the COPY command is executed
>
> Maybe it should say ".. is executing".  Or ".. being executed":
> | OID of the table on which the COPY command is being executed

+1 for changing it to "OID of the table on which the COPY command is
being executed."

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  34. Re: Improvements and additions to COPY progress reporting

    Michael Paquier <michael@paquier.xyz> — 2021-03-04T10:38:08Z

    On Thu, Mar 04, 2021 at 12:16:17PM +0530, Bharath Rupireddy wrote:
> IMO, the phrasing proposed by Justin upthread looks good. It's like this:
> 
> > +    Each backend running <command>ANALYZE</command> will report its progress in
> > +    the <structname>pg_stat_progress_analyze</structname> view. See

No objections to just go with that.  As a new patch set is needed, I
am switching the CF entry to "Waiting on Author".
--
Michael

  35. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-04T11:32:38Z

    On Thu, 4 Mar 2021 at 11:38, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Mar 04, 2021 at 12:16:17PM +0530, Bharath Rupireddy wrote:
> > IMO, the phrasing proposed by Justin upthread looks good. It's like this:
> >
> > > +    Each backend running <command>ANALYZE</command> will report its progress in
> > > +    the <structname>pg_stat_progress_analyze</structname> view. See
>
> No objections to just go with that.  As a new patch set is needed, I
> am switching the CF entry to "Waiting on Author".

Thanks for all your comments, and sorry for the delayed response.
Please find attached a new version of the patch set, that is rebased
and contains the requested changes:

1/3:
Docs:
- on which the COPY command is executed
+ on which the COPY command is being executed
Reworded existing commment:
- /* Increment amount of processed tuples and update the progress */
+ /* Increment the number of processed tuples, and report the progress */

2/3:
Docs:
- ... report its progress to ...
+ ... report its progress in ...
- report its progress to the >pg_stat_progress_cluster< ...
+ report its progress in the >pg_stat_progress_cluster< view ...

3/3:
No changes

I believe that that was the extent of the not-yet-resolved comments
and suggestions.


With regards,

Matthias van de Meent.

  36. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-04T12:36:45Z

    On Thu, Mar 4, 2021 at 5:02 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
>
> On Thu, 4 Mar 2021 at 11:38, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Thu, Mar 04, 2021 at 12:16:17PM +0530, Bharath Rupireddy wrote:
> > > IMO, the phrasing proposed by Justin upthread looks good. It's like this:
> > >
> > > > +    Each backend running <command>ANALYZE</command> will report its progress in
> > > > +    the <structname>pg_stat_progress_analyze</structname> view. See
> >
> > No objections to just go with that.  As a new patch set is needed, I
> > am switching the CF entry to "Waiting on Author".
>
> Thanks for all your comments, and sorry for the delayed response.
> Please find attached a new version of the patch set, that is rebased
> and contains the requested changes:
>
> 1/3:
> Docs:
> - on which the COPY command is executed
> + on which the COPY command is being executed
> Reworded existing commment:
> - /* Increment amount of processed tuples and update the progress */
> + /* Increment the number of processed tuples, and report the progress */

LGTM.

> 2/3:
> Docs:
> - ... report its progress to ...
> + ... report its progress in ...
> - report its progress to the >pg_stat_progress_cluster< ...
> + report its progress in the >pg_stat_progress_cluster< view ...

+   <para>
+    Each backend running <command>VACUUM</command> without the
+    <literal>FULL</literal> option will report its progress in the
+    <structname>pg_stat_progress_vacuum</structname> view. Backends running
+    <command>VACUUM</command> with the <literal>FULL</literal> option report
+    progress in the <structname>pg_stat_progress_cluster</structname> view
+    instead. See <xref linkend="vacuum-progress-reporting"/> and
+    <xref linkend="cluster-progress-reporting"/> for details.
+   </para>

I think a typo, missing "will" between option and report - it's "with
the <literal>FULL</literal> option will report"

Except the above typo, 0002 LGTM.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  37. Re: Improvements and additions to COPY progress reporting

    Josef Šimánek <josef.simanek@gmail.com> — 2021-03-04T13:44:26Z

    čt 4. 3. 2021 v 12:32 odesílatel Matthias van de Meent
<boekewurm+postgres@gmail.com> napsal:
>
> On Thu, 4 Mar 2021 at 11:38, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Thu, Mar 04, 2021 at 12:16:17PM +0530, Bharath Rupireddy wrote:
> > > IMO, the phrasing proposed by Justin upthread looks good. It's like this:
> > >
> > > > +    Each backend running <command>ANALYZE</command> will report its progress in
> > > > +    the <structname>pg_stat_progress_analyze</structname> view. See
> >
> > No objections to just go with that.  As a new patch set is needed, I
> > am switching the CF entry to "Waiting on Author".
>
> Thanks for all your comments, and sorry for the delayed response.
> Please find attached a new version of the patch set, that is rebased
> and contains the requested changes:
>
> 1/3:
> Docs:
> - on which the COPY command is executed
> + on which the COPY command is being executed
> Reworded existing commment:
> - /* Increment amount of processed tuples and update the progress */
> + /* Increment the number of processed tuples, and report the progress */
>
> 2/3:
> Docs:
> - ... report its progress to ...
> + ... report its progress in ...
> - report its progress to the >pg_stat_progress_cluster< ...
> + report its progress in the >pg_stat_progress_cluster< view ...
>
> 3/3:
> No changes
>
> I believe that that was the extent of the not-yet-resolved comments
> and suggestions.

LGTM, special thanks for taking over the work on initial COPY progress
regression tests.

>
> With regards,
>
> Matthias van de Meent.

  38. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-04T16:19:18Z

    On Thu, 4 Mar 2021 at 13:36, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> +   <para>
> +    Each backend running <command>VACUUM</command> without the
> +    <literal>FULL</literal> option will report its progress in the
> +    <structname>pg_stat_progress_vacuum</structname> view. Backends running
> +    <command>VACUUM</command> with the <literal>FULL</literal> option report
> +    progress in the <structname>pg_stat_progress_cluster</structname> view
> +    instead. See <xref linkend="vacuum-progress-reporting"/> and
> +    <xref linkend="cluster-progress-reporting"/> for details.
> +   </para>
>
> I think a typo, missing "will" between option and report - it's "with
> the <literal>FULL</literal> option will report"

"Backends running [...] report progress to [...] instead" is,
a.f.a.i.k., correct English. Adding 'will' would indeed still be
correct, but it would in my opinion also be decremental to the
readability of the section due to the repeated use of the same
template sentence structure. I think that keeping it as-is is just
fine.



With regards,

Matthias van de Meent.

  39. Re: Improvements and additions to COPY progress reporting

    Justin Pryzby <pryzby@telsasoft.com> — 2021-03-04T16:29:12Z

    On Thu, Mar 04, 2021 at 05:19:18PM +0100, Matthias van de Meent wrote:
> On Thu, 4 Mar 2021 at 13:36, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > +   <para>
> > +    Each backend running <command>VACUUM</command> without the
> > +    <literal>FULL</literal> option will report its progress in the
> > +    <structname>pg_stat_progress_vacuum</structname> view. Backends running
> > +    <command>VACUUM</command> with the <literal>FULL</literal> option report
> > +    progress in the <structname>pg_stat_progress_cluster</structname> view
> > +    instead. See <xref linkend="vacuum-progress-reporting"/> and
> > +    <xref linkend="cluster-progress-reporting"/> for details.
> > +   </para>
> >
> > I think a typo, missing "will" between option and report - it's "with
> > the <literal>FULL</literal> option will report"
> 
> "Backends running [...] report progress to [...] instead" is,
> a.f.a.i.k., correct English. Adding 'will' would indeed still be
> correct, but it would in my opinion also be decremental to the
> readability of the section due to the repeated use of the same
> template sentence structure. I think that keeping it as-is is just
> fine.

I'd prefer to see the same thing repeated, since then it's easy to compare, for
readers, and also future doc authors.  That's normal in technical documentation
to have redundancy.  It's easy to read.

I'd suggest to move "instead" into the middle of the sentence,
and combine VACUUM+FULL, and add "their":

> > +    ... Backends running <command>VACUUM FULL</literal> will instead report
> > +    their progress in the <structname>pg_stat_progress_cluster</structname> view.

-- 
Justin

  40. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-04T16:45:50Z

    On Thu, 4 Mar 2021 at 17:29, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Mar 04, 2021 at 05:19:18PM +0100, Matthias van de Meent wrote:
> >
> > "Backends running [...] report progress to [...] instead" is,
> > a.f.a.i.k., correct English. Adding 'will' would indeed still be
> > correct, but it would in my opinion also be decremental to the
> > readability of the section due to the repeated use of the same
> > template sentence structure. I think that keeping it as-is is just
> > fine.
>
> I'd prefer to see the same thing repeated, since then it's easy to compare, for
> readers, and also future doc authors.  That's normal in technical documentation
> to have redundancy.  It's easy to read.
>
> I'd suggest to move "instead" into the middle of the sentence,
> and combine VACUUM+FULL, and add "their":
>
> > > +    ... Backends running <command>VACUUM FULL</literal> will instead report
> > > +    their progress in the <structname>pg_stat_progress_cluster</structname> view.

Sure, I'm convinced. PFA the patchset with this change applied.

With regards,

Matthias van de Meent

  41. Re: Improvements and additions to COPY progress reporting

    Michael Paquier <michael@paquier.xyz> — 2021-03-05T06:00:14Z

    On Thu, Mar 04, 2021 at 05:45:50PM +0100, Matthias van de Meent wrote:
> Sure, I'm convinced. PFA the patchset with this change applied.

0002 looks fine to me, and in line with the discussion, so applied.
--
Michael

  42. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-07T11:20:31Z

    On Fri, Mar 5, 2021 at 11:30 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Mar 04, 2021 at 05:45:50PM +0100, Matthias van de Meent wrote:
> > Sure, I'm convinced. PFA the patchset with this change applied.
>
> 0002 looks fine to me, and in line with the discussion, so applied.

Thanks.

Attaching remaining patches 0001 and 0003 from the v11 patch
set(posted upthread) here to make cfbot happier.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  43. Re: Improvements and additions to COPY progress reporting

    Michael Paquier <michael@paquier.xyz> — 2021-03-08T08:23:50Z

    On Sun, Mar 07, 2021 at 04:50:31PM +0530, Bharath Rupireddy wrote:
> Attaching remaining patches 0001 and 0003 from the v11 patch
> set(posted upthread) here to make cfbot happier.

Looking at patch 0002, the location of each progress report looks good
to me.  I have some issues with some of the names chosen though, so I
would like to suggest a few changes to simplify things:
- PROGRESS_COPY_IO_TYPE_* => PROGRESS_COPY_TYPE_*
- PROGRESS_COPY_IO_TYPE => PROGRESS_COPY_TYPE
- PROGRESS_COPY_TYPE_STDIO => PROGRESS_COPY_TYPE_PIPE
- In pg_stat_progress_copy, io_type => type

It seems a bit confusing to not count insertions on foreign tables
where nothing happened.  I am fine to live with that, but can I ask if
this has been thought about?
--
Michael

  44. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-08T16:33:40Z

    On Mon, 8 Mar 2021 at 09:24, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sun, Mar 07, 2021 at 04:50:31PM +0530, Bharath Rupireddy wrote:
> > Attaching remaining patches 0001 and 0003 from the v11 patch
> > set(posted upthread) here to make cfbot happier.
>
> Looking at patch 0002, the location of each progress report looks good
> to me.  I have some issues with some of the names chosen though, so I
> would like to suggest a few changes to simplify things:
> - PROGRESS_COPY_IO_TYPE_* => PROGRESS_COPY_TYPE_*
> - PROGRESS_COPY_IO_TYPE => PROGRESS_COPY_TYPE
> - PROGRESS_COPY_TYPE_STDIO => PROGRESS_COPY_TYPE_PIPE
> - In pg_stat_progress_copy, io_type => type

Seems reasonable. PFA updated patches. I've renamed the previous 0003
to 0002 to keep git-format-patch easy.

> It seems a bit confusing to not count insertions on foreign tables
> where nothing happened.  I am fine to live with that, but can I ask if
> this has been thought about?

This is keeping current behaviour of the implementation as committed
with 8a4f618e, with the rationale of that patch being that this number
should mirror the number returned by the copy command.

I am not opposed to adding another column for `tuples_inserted` and
changing the logic accordingly (see prototype 0003), but that was not
in the intended scope of this patchset. Unless you think that this
should be included in this current patchset, I'll spin that patch out
into a different thread, but I'm not sure that would make it into
pg14.

With regards,

Matthias van de Meent.

  45. Re: Improvements and additions to COPY progress reporting

    Michael Paquier <michael@paquier.xyz> — 2021-03-09T05:34:52Z

    On Mon, Mar 08, 2021 at 05:33:40PM +0100, Matthias van de Meent wrote:
> Seems reasonable. PFA updated patches. I've renamed the previous 0003
> to 0002 to keep git-format-patch easy.

Thanks for updating the patch.  0001 has been applied, after tweaking
a bit comments, indentation and the docs.

> This is keeping current behaviour of the implementation as committed
> with 8a4f618e, with the rationale of that patch being that this number
> should mirror the number returned by the copy command.
> 
> I am not opposed to adding another column for `tuples_inserted` and
> changing the logic accordingly (see prototype 0003), but that was not
> in the intended scope of this patchset. Unless you think that this
> should be included in this current patchset, I'll spin that patch out
> into a different thread, but I'm not sure that would make it into
> pg14.

Okay, point taken.  If there is demand for it in the future, we could
extend the existing set of columns.  After thinking more about it the
usecase if not completely clear to me from a monitoring point of
view.

I have not looked at 0002 in details yet, but I am wondering first if
the size estimations in the expected output are actually portable.
Second, I doubt a bit that the extra cycles spent on that are actually
worth the coverage, even if the trick with an AFTER INSERT trigger is
interesting.
--
Michael

  46. Re: Improvements and additions to COPY progress reporting

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-09T08:02:58Z

    On Tue, Mar 9, 2021 at 11:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> > This is keeping current behaviour of the implementation as committed
> > with 8a4f618e, with the rationale of that patch being that this number
> > should mirror the number returned by the copy command.
> >
> > I am not opposed to adding another column for `tuples_inserted` and
> > changing the logic accordingly (see prototype 0003), but that was not
> > in the intended scope of this patchset. Unless you think that this
> > should be included in this current patchset, I'll spin that patch out
> > into a different thread, but I'm not sure that would make it into
> > pg14.
>
> Okay, point taken.  If there is demand for it in the future, we could
> extend the existing set of columns.  After thinking more about it the
> usecase if not completely clear to me from a monitoring point of
> view.

I think, for now, having better comments on what's included and what's
not in the existing tuples_processed column would help.

> I have not looked at 0002 in details yet, but I am wondering first if
> the size estimations in the expected output are actually portable.
> Second, I doubt a bit that the extra cycles spent on that are actually
> worth the coverage, even if the trick with an AFTER INSERT trigger is
> interesting.

I was having the same doubt [1], please have a look at it and the few
threads that follow. IMO, we can have the tests without the
"bytes_total" column if we think that it's not stable across all OS
platforms. I think those tests don't take much time to run and AFAIK
it's the first progress report command to have tests because others
such as analyse, vacuum, cluster, base backup don't have.

[1] - https://www.postgresql.org/message-id/flat/CALj2ACUyNREth8f3M7wXrHVeycfnqBn5pVygYOoBVs%3Difo8V4A%40mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  47. Re: Improvements and additions to COPY progress reporting

    Josef Šimánek <josef.simanek@gmail.com> — 2021-03-09T10:39:48Z

    út 9. 3. 2021 v 6:34 odesílatel Michael Paquier <michael@paquier.xyz> napsal:
>
> On Mon, Mar 08, 2021 at 05:33:40PM +0100, Matthias van de Meent wrote:
> > Seems reasonable. PFA updated patches. I've renamed the previous 0003
> > to 0002 to keep git-format-patch easy.
>
> Thanks for updating the patch.  0001 has been applied, after tweaking
> a bit comments, indentation and the docs.
>
> > This is keeping current behaviour of the implementation as committed
> > with 8a4f618e, with the rationale of that patch being that this number
> > should mirror the number returned by the copy command.
> >
> > I am not opposed to adding another column for `tuples_inserted` and
> > changing the logic accordingly (see prototype 0003), but that was not
> > in the intended scope of this patchset. Unless you think that this
> > should be included in this current patchset, I'll spin that patch out
> > into a different thread, but I'm not sure that would make it into
> > pg14.
>
> Okay, point taken.  If there is demand for it in the future, we could
> extend the existing set of columns.  After thinking more about it the
> usecase if not completely clear to me from a monitoring point of
> view.
>
> I have not looked at 0002 in details yet, but I am wondering first if
> the size estimations in the expected output are actually portable.
> Second, I doubt a bit that the extra cycles spent on that are actually
> worth the coverage, even if the trick with an AFTER INSERT trigger is
> interesting.

Those extra cycles are in there to cover at least parts of the COPY
progress from being accidentally broken. I have seen various patches
modifying COPY command being currently in progress. It would be nice
to ensure at least basic functionality works well in an automated way.
On my machine there is no huge overhead added by adding those tests
(they finish almost instantly).

> --
> Michael

  48. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-10T08:35:10Z

    On Tue, 9 Mar 2021 at 06:34, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Mar 08, 2021 at 05:33:40PM +0100, Matthias van de Meent wrote:
> > Seems reasonable. PFA updated patches. I've renamed the previous 0003
> > to 0002 to keep git-format-patch easy.
>
> Thanks for updating the patch.  0001 has been applied, after tweaking
> a bit comments, indentation and the docs.

Thanks!

> > This is keeping current behaviour of the implementation as committed
> > with 8a4f618e, with the rationale of that patch being that this number
> > should mirror the number returned by the copy command.
> >
> > I am not opposed to adding another column for `tuples_inserted` and
> > changing the logic accordingly (see prototype 0003), but that was not
> > in the intended scope of this patchset. Unless you think that this
> > should be included in this current patchset, I'll spin that patch out
> > into a different thread, but I'm not sure that would make it into
> > pg14.
>
> Okay, point taken.  If there is demand for it in the future, we could
> extend the existing set of columns.  After thinking more about it the
> usecase if not completely clear to me from a monitoring point of
> view.
>
> I have not looked at 0002 in details yet, but I am wondering first if
> the size estimations in the expected output are actually portable.
> Second, I doubt a bit that the extra cycles spent on that are actually
> worth the coverage, even if the trick with an AFTER INSERT trigger is
> interesting.

There are examples in which pg_stat_progress_* -views report
inaccurate data. I think it is fairly reasonable to at least validate
some part of the progress reporting, as it is one of the few methods
for administrators to look at the state of currently running
administrative tasks, and as such, this user-visible api should be
validated.


With regards,

Matthias van de Meent

  49. Re: Improvements and additions to COPY progress reporting

    Michael Paquier <michael@paquier.xyz> — 2021-03-15T04:53:43Z

    On Wed, Mar 10, 2021 at 09:35:10AM +0100, Matthias van de Meent wrote:
> There are examples in which pg_stat_progress_* -views report
> inaccurate data. I think it is fairly reasonable to at least validate
> some part of the progress reporting, as it is one of the few methods
> for administrators to look at the state of currently running
> administrative tasks, and as such, this user-visible api should be
> validated.

Looking closer at 0002, the size numbers are actually incorrect on
Windows for the second query.  The CRLFs at the end of each line of
emp.data add three bytes to the report of COPY FROM, so this finishes
with 82 bytes for bytes_total and bytes_processed instead of 79.
Let's make this useful but simpler here, so I propose to check that
the counters are higher than zero instead of an exact number.  Let's
also add the relation name relid::regclass while on it.

The tests introduced are rather limited, but you are right that
something is better than nothing here, and I have slightly updated
what the tests sent previously as per the attached.  What do you
think?
--
Michael

  50. Re: Improvements and additions to COPY progress reporting

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-15T11:43:40Z

    On Mon, 15 Mar 2021 at 05:53, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Mar 10, 2021 at 09:35:10AM +0100, Matthias van de Meent wrote:
> > There are examples in which pg_stat_progress_* -views report
> > inaccurate data. I think it is fairly reasonable to at least validate
> > some part of the progress reporting, as it is one of the few methods
> > for administrators to look at the state of currently running
> > administrative tasks, and as such, this user-visible api should be
> > validated.
>
> Looking closer at 0002, the size numbers are actually incorrect on
> Windows for the second query.  The CRLFs at the end of each line of
> emp.data add three bytes to the report of COPY FROM, so this finishes
> with 82 bytes for bytes_total and bytes_processed instead of 79.

Hmm, does CFBot not run checkout on windows with crlf line endings? I
had expected it to do as such.

> Let's make this useful but simpler here, so I propose to check that
> the counters are higher than zero instead of an exact number.  Let's
> also add the relation name relid::regclass while on it.

+1, I hadn't thought of casting relid to its regclass to get a stable
identifier.

> The tests introduced are rather limited, but you are right that
> something is better than nothing here, and I have slightly updated
> what the tests sent previously as per the attached.  What do you
> think?

That seems great, thanks for picking this up.


With regards,

Matthias van de Meent

  51. Re: Improvements and additions to COPY progress reporting

    Michael Paquier <michael@paquier.xyz> — 2021-03-16T01:00:37Z

    On Mon, Mar 15, 2021 at 12:43:40PM +0100, Matthias van de Meent wrote:
> Hmm, does CFBot not run checkout on windows with crlf line endings? I
> had expected it to do as such.

This is environment-sensitive, so I am not surprised that Appveyor
changes the way newlines are handled there.  I could see the
difference by running the tests manually on Windows command prompt for
example.

> That seems great, thanks for picking this up.

Okay.  Applied, then.
--
Michael