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
