Re: Improvements and additions to COPY progress reporting

On Tue, Feb 9, 2021 at 12:06 AM Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)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