On Tue, May 31, 2022 at 1:20 AM Amit Langote <amitlangot...@gmail.com>
wrote:
> Hi,
>
> On Thu, May 26, 2022 at 11:35 AM Zhihong Yu <z...@yugabyte.com> wrote:
> >> The changes in pgstat_progress_end_command() and
> >> pg_stat_get_progress_info() update st_progress_command_target
> >> depending on the command type involved, breaking the existing contract
> >> of those routines, particularly the fact that the progress fields
> >> *should* be reset in an error stack.
>
> +1 to what Michael said here. I don't think the following changes are
> acceptable:
>
> @@ -106,7 +106,13 @@ pgstat_progress_end_command(void)
> return;
>
> PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
> - beentry->st_progress_command = PROGRESS_COMMAND_INVALID;
> - beentry->st_progress_command_target = InvalidOid;
> + if (beentry->st_progress_command == PROGRESS_COMMAND_COPY)
> + // We want to show the relation for the most recent COPY command
> + beentry->st_progress_command = PROGRESS_COMMAND_COPY_DONE;
> + else
> + {
> + beentry->st_progress_command = PROGRESS_COMMAND_INVALID;
> + beentry->st_progress_command_target = InvalidOid;
> + }
> PGSTAT_END_WRITE_ACTIVITY(beentry);
> }
>
> pgstat_progress_end_command() is generic infrastructure and there
> shouldn't be anything COPY-specific there.
>
> > I searched the code base for how st_progress_command_target is used.
> > In case there is subsequent command following the COPY,
> st_progress_command_target would be set to the Oid
> > of the subsequent command.
> > In case there is no subsequent command following the COPY command, it
> seems leaving st_progress_command_target as
> > the Oid of the COPY command wouldn't hurt.
> >
> > Maybe you can let me know what side effect not resetting
> st_progress_command_target would have.
> >
> > As an alternative, upon seeing PROGRESS_COMMAND_COPY_DONE, we can
> transfer the value of
> > st_progress_command_target to a new field called, say,
> st_progress_command_previous_target (
> > and resetting st_progress_command_target as usual).
>
> That doesn't sound like a good idea.
>
> As others have said, there's no point in adding a status field to
> pg_stat_progress_copy that only tells whether a COPY is running or
> not. You can already do that by looking at the output of `select *
> from pg_stat_progress_copy`. If the COPY you're interested in is
> running, you'll find the corresponding row in the view. The row is
> made to disappear from the view the instance the COPY finishes, either
> successfully or due to an error. Whichever is the case will be known
> in the connection that initiated the COPY and you may find it in the
> server log. I don't think we should make Postgres remember anything
> about that in the shared memory, or at least not with one-off
> adjustments of the shared progress reporting state like in the
> proposed patch.
>
> --
> Thanks, Amit Langote
> EDB: http://www.enterprisedb.com
Hi, Matthias, Michael and Amit:
Thanks for your time reviewing my patch.
I took note of what you said.
If I can make the changes more general, I would circle back.
Cheers
