Re: please update ps display for recovery checkpoint

On Wed, Dec 09, 2020 at 06:37:22PM +0000, Bossart, Nathan wrote:
> On 12/8/20, 10:16 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
>> Yeah, I'd rather leave out the part of the patch where we have the
>> checkpointer say "idle".  So I still think that what v3 did upthread,
>> by just resetting the ps display in all cases, feels more natural.  So
>> we should remove the parts of v5 from checkpointer.c.
>
> That seems fine to me.  I think it is most important that the end-of-
> recovery and shutdown checkpoints are shown.  I'm not sure there's
> much value in updating the process title for automatic checkpoints and
> checkpoints triggered via the CHECKPOINT command, so IMO we could skip
> those, too.  I made these changes in the new version of the patch.

It would be possible to use pg_stat_activity in most cases here, so I
agree to settle down to the minimum we can agree on for now, and maybe
discuss separately if this should be extended in some or another in
the future if there is a use-case for that.  So I'd say that what you
have here is logically fine.

> > +   snprintf(activitymsg, sizeof(activitymsg), "creating %s%scheckpoint",
> > [...]
> > +   snprintf(activitymsg, sizeof(activitymsg), "creating %srestartpoint",
> > FWIW, I still fing "running" to sound better than "creating" here.  An
> > extra term I can think of that could be adapted is "performing".
>
> I think I prefer "performing" over "running" because that's what the
> docs use [0].  I've changed it to "performing" in the new version of
> the patch.

That's also used in the code comments, FWIW.

@@ -9282,6 +9296,7 @@ CreateRestartPoint(int flags)
    XLogRecPtr  endptr;
    XLogSegNo   _logSegNo;
    TimestampTz xtime;
+   bool        shutdown = (flags & CHECKPOINT_IS_SHUTDOWN);
You are right that CHECKPOINT_END_OF_RECOVERY should not be called for
a restart point, so that's correct.

However, I think that it would be better to have all those four code
paths controlled by a single routine, where we pass down the
checkpoint flags and fill in the ps string directly depending on what
the caller wants to do.  This way, we will avoid any inconsistent
updates if this stuff gets extended in the future, and there will be
all the information at hand to extend the logic.  So I have simplified
your patch as per the attached.  Thoughts?
--
Michael