Re: Improvements and additions to COPY progress reporting

On Tue, 9 Feb 2021 at 08:12, Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> 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.

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(dot)simanek(at)gmail(dot)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