At Fri, 28 Jan 2022 10:41:28 +0900 (JST), Kyotaro Horiguchi
<horikyota....@gmail.com> wrote in
> Sorry, the last message lacks one citation.
>
> At Thu, 27 Jan 2022 19:09:29 +0800, Julien Rouhaud <rjuju...@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
