Re: Is there a way (except from server logs) to know the kind of on-going/last checkpoint? - Mailing list pgsql-hackers

At Fri, 28 Jan 2022 10:41:28 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Sorry, the last message lacks one citation.
> 
> At Thu, 27 Jan 2022 19:09:29 +0800, Julien Rouhaud <rjuju123@gmail.com> wrote in 
> > On Thu, Jan 27, 2022 at 06:56:57PM +0800, Julien Rouhaud wrote:
> > > 
> > > What it's showing is the "currently ongoing checkpoint or last completed
> > > checkpoint" kind.
> > 
> > Ah after double checking I see it's storing the information *after* the
> > checkpoint completion, so it's indeed the last completed checkpoint.  I'm not
> > sure how useful it can be, but ok.
> 
> I don't see it useful (but don't oppose) to record checkpoint kind in
> control file.  It is a kind of realtime noncritical info and in the
> first place retrievable from server log if needed. And I'm skeptical
> that it is needed such frequently.  Checkpoint kind is useful to check
> max_wal_size's appropriateness if it is in a summarized form as in
> pg_stat_bgwriter. On the other hand showing the same in a stats view
> or the output of pg_control_checkpoint() is fine by me.
> 
> > > Also, it's only showing the initial triggering conditions of checkpoints.
> > > For instance, if a timed checkpoint is started and then a backend executes a
> > > "CHECKPOINT;", it will upgrade the ongoing checkpoint with additional flags but
> > > AFAICS those new flags won't be saved to the control file.
> > 
> > This one is still valid I think, it's only storing the initial flags and not
> > the possibly upgraded one in shmem.
> 
> Agreed.
> 
> +    snprintf(ckpt_kind, 2 * MAXPGPATH, "%s%s%s%s%s%s%s%s%s",
> +                 (flags == 0) ? "unknown" : "",
> +                 (flags & CHECKPOINT_IS_SHUTDOWN) ? "shutdown " : "",
> +                 (flags & CHECKPOINT_END_OF_RECOVERY) ? "end-of-recovery " : "",
> +                 (flags & CHECKPOINT_IMMEDIATE) ? "immediate " : "",
> +                 (flags & CHECKPOINT_FORCE) ? "force " : "",
> +                 (flags & CHECKPOINT_WAIT) ? "wait " : "",
> +                 (flags & CHECKPOINT_CAUSE_XLOG) ? "wal " : "",
> +                 (flags & CHECKPOINT_CAUSE_TIME) ? "time " : "",
> +                 (flags & CHECKPOINT_FLUSH_ALL) ? "flush-all" : "");
> 
> 
> I don't like to add this complex-but-need-in-sync blob twice. If we
> need to do that twice, I want them consolidated in any shape.
> 
> +    Datum        values[18];
> +    bool        nulls[18];
> 
> You forgot to expand these arrays.
> 
> This breaks checkpoint file format. Need to bump PG_CONTROL_VERSION,
> and pg_upgrade need to treat the change.
> 
> Even if we add checkpoint kind to control file, it would look a bit
> strange that the "checkpoint kind" shows first among all
> checkpoint-related lines.  And at least the "wait" in the line is
> really useless since it is not a property of a checkpoint. Instead, it
> doesn't show "requested" which is one of the checkpoint properties
> like "xlog" and "time".  I'm not sure we need all of the properties to
> be shown but I don't have a clear criteria for each property of it
> ought to be shown or not.
> 
> > pg_control last modified:             Fri 28 Jan 2022 09:49:46 AM JST
> > Latest checkpoint kind:               immediate force wait 
> > Latest checkpoint location:           0/172B2C8
> 
> I'd like to see the PID of the triggering process, but it is really
> not a information suitable in the control file...
> 
> 
> -  proallargtypes => '{pg_lsn,pg_lsn,text,int4,int4,bool,text,oid,xid,xid,xid,oid,xid,xid,oid,xid,xid,timestamptz}',
> +  proallargtypes =>
'{pg_lsn,pg_lsn,text,int4,int4,bool,text,oid,xid,xid,xid,oid,xid,xid,oid,xid,xid,timestamptz,text}',
> 
> I think the additional column should be text[] instead of text, but
> not sure.
> 
> And you need to edit the corresponding part of the doc.

I have an additional comment.

+    char         ckpt_kind[2 * MAXPGPATH];
..
+    snprintf(ckpt_kind, 2 * MAXPGPATH, "%s%s%s%s%s%s%s%s%s",
+                 (flags == 0) ? "unknown" : "",
+                 (flags & CHECKPOINT_IS_SHUTDOWN) ? "shutdown " : "",


The value "2 * MAXPGPATH" is utterly nonsense about bouth "2" and
"MAXPGPATH", and the product of them is apparently too large.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center