Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Wed, 22 Jul 2026 23:19:56 +0200
Message-Id: <DK5EWY6GK6Q8.1LPLR26F3RDWA@gmail.com>
To: "Antonin Houska" <ah@cybertec.at>, "Alberto Piai"
 <alberto.piai@gmail.com>
Subject: Re: Allow progress tracking of sub-commands
Cc: "Rahila Syed" <rahilasyed90@gmail.com>,
 <pgsql-hackers@lists.postgresql.org>,
 <bharath.rupireddyforpostgres@gmail.com>, <mihailnikalayeu@gmail.com>,
 <adam8157@gmail.com>
From: "Alberto Piai" <alberto.piai@gmail.com>
References: <106920.1782734246@localhost>
 <CAH2L28sbOsY_3-2J+=UCg9GFvgZjf_Ohu9y1Nk26ME9ieHc4rw@mail.gmail.com>
 <69363.1782919477@localhost> <DJYG6LBPP3RK.2EPSN5QV8WVYL@gmail.com>
 <10153.1784708881@localhost>
In-Reply-To: <10153.1784708881@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/DK5EWY6GK6Q8.1LPLR26F3RDWA%40gmail.com>
Precedence: bulk

On Wed Jul 22, 2026 at 10:28 AM CEST, Antonin Houska wrote:
>>=20
>> @@ -33,9 +37,30 @@ pgstat_progress_start_command(ProgressCommandType cmd=
type, Oid relid)
>>                 return;
>>=20
>>         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);
>>  }
>>=20
>>=20
>> 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 j=
ust
> variants of the existing ones. So I don't think the patch increases the r=
isk
> 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 i=
t,
> not the restart. Restart could be a problem if I wanted the other regress=
ion
> tests to complete before fixing the bug. However that doesn't make much s=
ense
> 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 situati=
on
> 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:
>>=20
>> 	appendStringInfo(&buf, _("delay time: %.3f ms\n"),
>> 					 (double) MyBEEntry->st_progress_param[PROGRESS_VACUUM_DELAY_TIME] =
/ 1000000.0);
>>=20
>> 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 pa=
tch
> 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

--=20
Alberto Piai
Sensational AG
Z=C3=BCrich, Switzerland