From: Antonin Houska <ah@cybertec.at>
To: "Alberto Piai" <alberto.piai@gmail.com>
cc: "Rahila Syed" <rahilasyed90@gmail.com>,
    pgsql-hackers@lists.postgresql.org,
    bharath.rupireddyforpostgres@gmail.com, mihailnikalayeu@gmail.com,
    adam8157@gmail.com
Subject: Re: Allow progress tracking of sub-commands
In-reply-to: <DJYG6LBPP3RK.2EPSN5QV8WVYL@gmail.com>
References: <106920.1782734246@localhost>
 <CAH2L28sbOsY_3-2J+=UCg9GFvgZjf_Ohu9y1Nk26ME9ieHc4rw@mail.gmail.com>
 <69363.1782919477@localhost> <DJYG6LBPP3RK.2EPSN5QV8WVYL@gmail.com>
Comments: In-reply-to "Alberto Piai" <alberto.piai@gmail.com>
   message dated "Tue, 14 Jul 2026 18:50:53 +0200."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <10152.1784708881.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 22 Jul 2026 10:28:01 +0200
Message-ID: <10153.1784708881@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/10153.1784708881%40localhost>
Precedence: bulk

Alberto Piai <alberto.piai@gmail.com> wrote:

> thanks for working on this, as a user I find this a worthy improvement.
> =

> >> /*
> >> + * Some commands have a sub-command, e.g. REPACK (re)builds indexes.=
 The
> >> + * target can be different, e.g. when the sub-command builds an inde=
x on
> >> + * TOAST relation.
> >> + */
> >> + ProgressCommandType st_progress_command2;
> >> + Oid st_progress_command_target2;
> >> + int64 st_progress_param2[PGSTAT_NUM_PROGRESS_PARAM];
> >> =

> >> This approach only works for a nesting depth of 2, not for 3 or more
> >> levels of subcommands, for instance.
> >> I am not aware of a concrete example of such a command but I think we=
 should
> >> keep the design generic enough to accommodate this.
> ...
> > The typical problem occurs when REPACK performs reindexing (CREATE_IND=
EX). I
> > could only think of one case where the depth would be more than 2: an =
index
> > function (executed during the build) running another monitored command=
. In a
> > development build (with my patch applied), such a case would fire an a=
ssertion
> > in pgstat_progress_start_command(), while in a production build that i=
nnermost
> > command would only overwrite the status of the CREATE INDEX command. I=
 don't
> > consider such a crazy case worth more effort.
> =

> I tend to agree that (for now, at least) a nesting level of 3 is an edge
> case, but I have another concern with the current approach:
> =

> @@ -33,9 +37,30 @@ pgstat_progress_start_command(ProgressCommandType cmd=
type, Oid relid)
>                 return;
> =

>         PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
> -       beentry->st_progress_command =3D cmdtype;
> -       beentry->st_progress_command_target =3D relid;
> -       MemSet(&beentry->st_progress_param, 0, sizeof(beentry->st_progre=
ss_param));
> +       /* Sub-command should not be started w/o parent command. */
> +       if (beentry->st_progress_command =3D=3D PROGRESS_COMMAND_INVALID=
)
> +       {
> +               Assert(beentry->st_progress_command2 =3D=3D PROGRESS_COM=
MAND_INVALID);
> +
> +               beentry->st_progress_command =3D cmdtype;
> +               beentry->st_progress_command_target =3D relid;
> +               MemSet(&beentry->st_progress_param, 0,
> +                          sizeof(beentry->st_progress_param));
> +       }
> +       else if (beentry->st_progress_command2 =3D=3D PROGRESS_COMMAND_I=
NVALID)
> +       {
> +               Assert(beentry->st_progress_command !=3D PROGRESS_COMMAN=
D_INVALID);
> +
> +               beentry->st_progress_command2 =3D cmdtype;
> +               beentry->st_progress_command_target2 =3D relid;
> +               MemSet(&beentry->st_progress_param2, 0,
> +                          sizeof(beentry->st_progress_param2));
> +       }
> +       else
> +       {
> +               /* Only one level of nesting is supported. */
> +               Assert(false);
> +       }
>         PGSTAT_END_WRITE_ACTIVITY(beentry);
>  }
> =

> =

> The comments of the two macros warn about the fact that they create a
> critical section, and code within the critical section should be kept as
> simple as possible because any error would be promoted to PANIC. Now
> sure, Assert() doesn't matter in production builds, but still I think
> there is value in getting rid of a possible error path.

The patch only adds an if-else construct, but the other lines added are ju=
st
variants of the existing ones. So I don't think the patch increases the ri=
sk
of an error ERROR (promoted to PANIC).

> (And a restart to clean up shared memory would be pretty annoying even i=
n
> debug builds.)

If Assert() fires, then what I consider annoying is the bug that caused it=
,
not the restart. Restart could be a problem if I wanted the other regressi=
on
tests to complete before fixing the bug. However that doesn't make much se=
nse
to me because all the regression tests should be run again as soon as the =
bug
is fixed.

> Assuming the start()/end() calls are properly nested (which I'd assume
> they are given the structure of the if/else conditions in start()/end()
> in this patch), what about the following (names are all strawmen, as
> this is just a suggestion):
> =

> - group command, target and params into a new struct, let's say Progress
> - track the current level of nesting in the beentry, initially 0
> - track an array of Progress structs in the beentry, of MAX_NESTING size
>   (2, for now)
> =

> Then ...start_command() would do something like the following:
> =

> PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
> beentry->nesting++;
> if beentry->nesting <=3D MAX_NESTING
>     beentry->progress[beentry->nesting-1].command =3D ...
>     ...
> PGSTAT_END_WRITE_ACTIVITY(beentry);
> =

> ...end_command() and the update routines would have to reflect this of
> course, and any reader would have to be changed. Readers would have to
> go through an accessor that clamps the index to [0,MAX_NESTING) and make
> this a noop otherwise.

Now that I compare my approach to yours, I'm not really opposed to this. T=
he
fact that the coding is more generic doesn't imply that MAX_NESTING needs =
to
be very large.

> One advantage of this would be that it makes the case of an unsupported
> nesting level a noop, instead of an error.

I don't see an advantage exactly here: my patch does not raise ERROR, and =
I
think that Assert() is needed to check the nesting level even if it's
implemented in your way.

> The main advantage would be that the compiler would yell if we forgot to
> change a reader.

Not sure I understand the difference. Can you please describe the situatio=
n
w/o this advantage, i.e. w/o the compiler errors?

> While looking into this for example, I ran into some code in
> vacuumlazy.c (heap_vacuum_rel()) which writes into st_progress_param
> directly:
> =

> 	appendStringInfo(&buf, _("delay time: %.3f ms\n"),
> 					 (double) MyBEEntry->st_progress_param[PROGRESS_VACUUM_DELAY_TIME] =
/ 1000000.0);
> =

> Purely theoretical of course, but if this was done by code which could
> run both as a main command or a subcommand, this would end up writing
> to the wrong spot.

This looks like reading rather than writing, but I think I understand what=
 you
mean.

> Regarding the maximum level of nesting: right now we could say that
> anything above 2 is not worth the bytes in MyBEEntry, and if we ever
> encountered a case where it's worth reporting progress of a second
> subcommand, this could be raised to 3.

Yes, we should be careful because MAX_NESTING items of the array would be
allocated for every single backend.

> What do you think, is something like this feasible? Maybe it would
> address Rahila's point, while not being too much extra work / complex
> code.

Yes, I think it's worth at least a prototype. Do you want to adjust my pat=
ch
yourself or should I do?

Thanks for your review anyway!

-- =

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