Re: Improvements and additions to COPY progress reporting

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