Re: adding status for COPY progress report

Hi,

On Thu, May 26, 2022 at 11:35 AM Zhihong Yu <zyu@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