Allow progress tracking of sub-commands

Hi Antonin,

Thanks for suggesting enhancements to the progress reporting framework.

I have comments on the solution's approach.

/*
+ * 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 index 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.

Shall we consider reporting the progress counters of a sub-command as
additional parameters
in the existing st_progress_param[] array? A top level command can
define the progress parameters
based on the number and type of sub-commands it expects to see. This
way we can even restrict or alter the counters we would like to report
for a sub-command. This also avoids changing PgBackendStatus every
time we encounter a command requiring deeper nesting levels.

Thank you,
Rahila Syed

Rahila Syed <rahilasyed90@gmail.com> wrote:

Thanks for suggesting enhancements to the progress reporting framework.

I have comments on the solution's approach.

Thanks.

/*
+ * 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 index 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.

I thought about all the current command types

typedef enum ProgressCommandType
{
PROGRESS_COMMAND_INVALID,
PROGRESS_COMMAND_VACUUM,
PROGRESS_COMMAND_ANALYZE,
PROGRESS_COMMAND_CREATE_INDEX,
PROGRESS_COMMAND_BASEBACKUP,
PROGRESS_COMMAND_COPY,
PROGRESS_COMMAND_REPACK,
} ProgressCommandType;

The typical problem occurs when REPACK performs reindexing (CREATE_INDEX). 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 assertion
in pgstat_progress_start_command(), while in a production build that innermost
command would only overwrite the status of the CREATE INDEX command. I don't
consider such a crazy case worth more effort.

Shall we consider reporting the progress counters of a sub-command as
additional parameters
in the existing st_progress_param[] array? A top level command can
define the progress parameters
based on the number and type of sub-commands it expects to see. This
way we can even restrict or alter the counters we would like to report
for a sub-command. This also avoids changing PgBackendStatus every
time we encounter a command requiring deeper nesting levels.

It might be possible to make sure in some cases that parameter numbers not
used by the main command are used by the sub-command, but I think it would
make coding quite tricky. And if you wanted to be sure that there are no
collisions of parameter numbers, you'd need st_progress_param[] to be twice as
large as the maximum number of parameters per command anyway. So this approach
wouldn't help as long as you're concerned of memory consumption.

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

Hi Antonin and Rahila,

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 index 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_INDEX). 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 assertion
in pgstat_progress_start_command(), while in a production build that innermost
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 cmdtype, Oid relid)
return;

        PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
-       beentry->st_progress_command = cmdtype;
-       beentry->st_progress_command_target = relid;
-       MemSet(&beentry->st_progress_param, 0, sizeof(beentry->st_progress_param));
+       /* Sub-command should not be started w/o parent command. */
+       if (beentry->st_progress_command == PROGRESS_COMMAND_INVALID)
+       {
+               Assert(beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID);
+
+               beentry->st_progress_command = cmdtype;
+               beentry->st_progress_command_target = relid;
+               MemSet(&beentry->st_progress_param, 0,
+                          sizeof(beentry->st_progress_param));
+       }
+       else if (beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID)
+       {
+               Assert(beentry->st_progress_command != PROGRESS_COMMAND_INVALID);
+
+               beentry->st_progress_command2 = cmdtype;
+               beentry->st_progress_command_target2 = 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. (And a restart
to clean up shared memory would be pretty annoying even in debug
builds.)

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 <= MAX_NESTING
beentry->progress[beentry->nesting-1].command = ...
...
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.

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

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

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.

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.

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.

Regards,

Alberto

--
Alberto Piai
Sensational AG
Zürich, Switzerland

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 index 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_INDEX). 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 assertion
in pgstat_progress_start_command(), while in a production build that innermost
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 cmdtype, Oid relid)
return;

PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
-       beentry->st_progress_command = cmdtype;
-       beentry->st_progress_command_target = relid;
-       MemSet(&beentry->st_progress_param, 0, sizeof(beentry->st_progress_param));
+       /* Sub-command should not be started w/o parent command. */
+       if (beentry->st_progress_command == PROGRESS_COMMAND_INVALID)
+       {
+               Assert(beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID);
+
+               beentry->st_progress_command = cmdtype;
+               beentry->st_progress_command_target = relid;
+               MemSet(&beentry->st_progress_param, 0,
+                          sizeof(beentry->st_progress_param));
+       }
+       else if (beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID)
+       {
+               Assert(beentry->st_progress_command != PROGRESS_COMMAND_INVALID);
+
+               beentry->st_progress_command2 = cmdtype;
+               beentry->st_progress_command_target2 = 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 just
variants of the existing ones. So I don't think the patch increases the risk
of an error ERROR (promoted to PANIC).

(And a restart to clean up shared memory would be pretty annoying even in
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 regression
tests to complete before fixing the bug. However that doesn't make much sense
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 <= MAX_NESTING
beentry->progress[beentry->nesting-1].command = ...
...
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. The
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 situation
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 patch
yourself or should I do?

Thanks for your review anyway!

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

On Wed Jul 22, 2026 at 10:28 AM CEST, Antonin Houska wrote:

@@ -33,9 +37,30 @@ pgstat_progress_start_command(ProgressCommandType cmdtype, Oid relid)
return;

PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
-       beentry->st_progress_command = cmdtype;
-       beentry->st_progress_command_target = relid;
-       MemSet(&beentry->st_progress_param, 0, sizeof(beentry->st_progress_param));
+       /* Sub-command should not be started w/o parent command. */
+       if (beentry->st_progress_command == PROGRESS_COMMAND_INVALID)
+       {
+               Assert(beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID);
+
+               beentry->st_progress_command = cmdtype;
+               beentry->st_progress_command_target = relid;
+               MemSet(&beentry->st_progress_param, 0,
+                          sizeof(beentry->st_progress_param));
+       }
+       else if (beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID)
+       {
+               Assert(beentry->st_progress_command != PROGRESS_COMMAND_INVALID);
+
+               beentry->st_progress_command2 = cmdtype;
+               beentry->st_progress_command_target2 = 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 just
variants of the existing ones. So I don't think the patch increases the risk
of an error ERROR (promoted to PANIC).

(And a restart to clean up shared memory would be pretty annoying even in
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 regression
tests to complete before fixing the bug. However that doesn't make much sense
to me because all the regression tests should be run again as soon as the bug
is fixed.

Sorry, I could have been clearer. I was referring specifically to the
Assert(), which would be hit in the edge case you mentioned (a command
trying to report progress past the 2nd level of nesting).

I think it would be nicer if attempts at progress reporting past the
maximum supported level of nesting would be a noop rather than an error,
and in general if the command didn't need to know whether it's reporting
progress at level 1 or level 2.

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 situation
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.

What I meant to say: with your patch, existing code referring to
st_progress_command would not break, but might find itself working with
st_progress_param, when it should be st_progress_param2 (depending on
how the command is nested).

If we instead changed to an array of structs, it would be a breaking
change and we would have to fix all the callers.

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 patch
yourself or should I do?

Please go on, I was just trying to help as a reviewer. Looking forward
to the second version then :)

Regards,

Alberto

--
Alberto Piai
Sensational AG
Zürich, Switzerland