Re: Improvements and additions to COPY progress reporting - Mailing list pgsql-hackers

From Matthias van de Meent
Subject Re: Improvements and additions to COPY progress reporting
Date
Msg-id CAEze2Wh753JM+W25grkM30GUXvE_bjHVEJdkdvhnL8SthNSJVA@mail.gmail.com
Whole thread Raw
In response to Re: Improvements and additions to COPY progress reporting  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
Responses Re: Improvements and additions to COPY progress reporting
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Ashutosh Bapat
Date:
Subject: Re: TRUNCATE on foreign table
Next
From: Dilip Kumar
Date:
Subject: Re: [HACKERS] Custom compression methods