Re: Allow progress tracking of sub-commands

Alberto Piai <alberto(dot)piai(at)gmail(dot)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