From: Antonin Houska <ah@cybertec.at>
To: Sami Imseih <samimseih@gmail.com>
cc: pgsql-hackers@postgresql.org
Subject: Re: Conflicting updates of command progress
In-reply-to: <CAA5RZ0v5tYwsdSxQaFUh0xkHPnY62m55qZ=a7k6EewbP-zCMvA@mail.gmail.com>
References: <117147.1744380740@localhost> <CAA5RZ0s_Hta6PWDwY9Lf-c+14F+C13BK2ax=B6R7bmDNhN6xdQ@mail.gmail.com> <17632.1745403972@localhost> <CAA5RZ0v5tYwsdSxQaFUh0xkHPnY62m55qZ=a7k6EewbP-zCMvA@mail.gmail.com>
Comments: In-reply-to Sami Imseih <samimseih@gmail.com>
   message dated "Wed, 23 Apr 2025 15:20:27 -0500."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4745.1745487225.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 24 Apr 2025 11:33:45 +0200
Message-ID: <4746.1745487225@localhost>
Archived-At: <https://www.postgresql.org/message-id/4746.1745487225%40localhost>
Precedence: bulk

Sami Imseih <samimseih@gmail.com> wrote:

> >> pgstat_progress_start_command should only be called once by the entry
> >> point for the
> >> command. In theory, we could end up in a situation where start_comman=
d
> >> is called multiple times during the same top-level command;
> =

> > Not only in theory - it actually happens when CLUSTER is rebuilding in=
dexes.
> =

> In the case of CLUSTER, pgstat_progress_start_command is only called onc=
e,
> but pgstat_progress_update_param is called in the context of both CLUSTE=
R
> and CREATE INDEX.

pgstat_progress_start_command() is called twice: First with
cmdtype=3DPROGRESS_COMMAND_CLUSTER, second with
PROGRESS_COMMAND_CREATE_INDEX. The first happens in cluster_rel() the seco=
nd
in cluster_rel() -> rebuild_relation() -> finish_heap_swap() ->
reindex_relation() -> reindex_index().

It does not matter though, the current design only expects one command.

> > That's a possible approach. However, if the index build takes long tim=
e in the
> > CLUSTER case, the user will probably be interested in details about th=
e index
> > build.
> =

> I agree,
> =

> >> Is there a repro that you can share that shows the weird values? It s=
ounds like
> >> the repro is on top of [1]. Is that right?
> =

> >> You can reproduce the similar problem by creating a trigger function =
that
> >> runs a progress-reporting command like COPY, and then COPY data into
> >> a table that uses that trigger.
> =

> >> [2] https://commitfest.postgresql.org/patch/5282/
> =

> In this case, pgstat_progress_start_command is actually called
> twice in the life of a single COPY command; the upper-level COPY
> command calls pgstat_progress_start_command and then the nested COPY
> command also does calls pgstat_progress_start_command.
> =

> > I think that can be implemented by moving the progress related fields =
from
> > PgBackendStatus into a new structure and by teaching the backend to in=
sert a
> > new instance of that structure into a shared hash table (dshash.c)
> =

> I think this is a good idea in general to move the backend progress to
> shared memory.
> and with a new API that will deal with scenarios as described above.
> 1/ an (explicit) nested
> command was started by a top-level command, such as the COPY case above.
> 2/ a top-level command triggered some other progress code implicitly, su=
ch as
> CLUSTER triggering CREATE INDEX code.

Yes, I mean a new API. I imagine pgstat_progress_start_command() to initia=
lize
the shared memory to track the "nested" command and to put the existing va=
lue
of MyBEEntry onto a stack. pgstat_progress_end_command() would then restor=
e
the original value.

However, special care is needed for [2] because that's not necessarily
nesting: consider merge-joining two foreign tables, both using file_fdw. I=
n
this case the pointers to the progress tracking shared memory would probab=
ly
have to be passed as function arguments.

(Note that the current progress tracking system also uses shared memory,
however in a less flexible way.)

> I also like the shared memory approach because we can then not have to u=
se
> a message like the one introduced in f1889729dd3ab0 to support parallel =
index
> vacuum progress 46ebdfe164c61.

I didn't know about these patches. I'm not sure though if this needs to be
removed. Even if each worker updated the progress information separately
(would users appreciate that?), it should still send the progress informat=
ion
to the leader before it (i.e. the worker) exits.

-- =

Antonin Houska
Web: https://www.cybertec-postgresql.com