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: <CAA5RZ0s_Hta6PWDwY9Lf-c+14F+C13BK2ax=B6R7bmDNhN6xdQ@mail.gmail.com>
References: <117147.1744380740@localhost> <CAA5RZ0s_Hta6PWDwY9Lf-c+14F+C13BK2ax=B6R7bmDNhN6xdQ@mail.gmail.com>
Comments: In-reply-to Sami Imseih <samimseih@gmail.com>
   message dated "Mon, 14 Apr 2025 12:13:00 -0500."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <17631.1745403972.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 23 Apr 2025 12:26:12 +0200
Message-ID: <17632.1745403972@localhost>
Archived-At: <https://www.postgresql.org/message-id/17632.1745403972%40localhost>
Precedence: bulk

Sami Imseih <samimseih@gmail.com> wrote:

> > While working on [1] I realized that some field of pg_stat_progress_cl=
uste has
> > weird value.
> =

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

Yes.

> > AFAICS the current design does not consider that one progress-reportin=
g
> > command can be called by another one.
> =

> 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_command
> is called multiple times during the same top-level command;

Not only in theory - it actually happens when CLUSTER is rebuilding indexe=
s.

> > Not sure what the correct fix is. We can
> > either ignore update requests from the "nested" commands, or display t=
he
> =

> There is a pattern where we do
> =

> ```
> if (progress)
>       pgstat_progress_update_param
> ```
> =

> cluster_rel can pass down a flag to index_build or others that update pr=
ogress
> to not report progress. Therefore, only the top level command is
> updating progress.
> what do you think?

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

The other approach is that we report only on the index build, but in that =
case
the interesting row of pg_stat_progress_cluster would disappear
temporarily. That might also be confusing.

Ideally we should report on all the commands in progress, but that is not
trivial to implement.

> [1] https://commitfest.postgresql.org/patch/5117/

-- =

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