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
