[Alberto Piai <alberto.piai@gmail.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