回复: [PATCH] Expose checkpoint reason to completion log messages.

Поиск
Список
Период
Сортировка
От li carol
Тема 回复: [PATCH] Expose checkpoint reason to completion log messages.
Дата
Msg-id KL1PR01MB666281A072547EC996C03CCD8185A@KL1PR01MB6662.apcprd01.prod.exchangelabs.com
обсуждение исходный текст
Ответ на Re: [PATCH] Expose checkpoint reason to completion log messages.  (Soumya S Murali <soumyamurali.work@gmail.com>)
Список pgsql-hackers
> 
> Hi all,
> 
> On Wed, Jan 7, 2026 at 1:37 PM Michael Banck <mbanck@gmx.net> wrote:
> >
> > Hi,
> >
> > On Wed, Jan 07, 2026 at 12:35:08PM +0530, Soumya S Murali wrote:
> > > I agree with your point that the checkpoint completion message
> > > should use the same terminology as the starting message ( “fast”
> > > rather than
> > > “immediate”) and I have updated the patch accordingly and have
> > > attached herewith for review. This patch aligns the checkpoint
> > > completion log wording with the existing checkpoint starting message
> > > (using “fast” instead of “immediate”) for consistency.
> >
> > First off, please send complete and not incremental patches.
> >
> > From what I can tell, you currently also do not have "wal" as reason?
> > Also, you have "timed" vs "time" etc.
> >
> > I think it would make most sense to factor out the logic for the
> > starting checkpoint reason from xlog.c ("(errmsg("checkpoint
> > starting:%s%s%s%s%s%s%s%s")") and use that both for starting and
> > stopping.
> >
> >
> > Michael
> 
> Thank you for the review and feedback.
> I have reworked the patch to factor out the checkpoint reason logic to use the
> same wording for both checkpoint start and completion log (including wal, time,
> fast, force, and wait). The format has been aligned with existing conventions
> and has been verified by running make check and recovery TAP tests including
> t/019_replslot_limit.pl successfully.
> Kindly review my patch and let me know the feedback.
> 
> Regards,
> Soumya

Hi Soumya,

Thanks for the updated patch. I agree that having the checkpoint reason in the completion log is very helpful for
performanceanalysis, as it avoids the need to manually correlate "starting" and "complete" messages in busy logs.
 
I’ve reviewed the latest version and have a few suggestions to make the refactoring more complete:
1. Consistency in LogCheckpointStart
Currently, the checkpoint branch uses the new CheckpointReasonString helper, but the restartpoint branch still uses the
oldinline triple-operator logic. It would be better to use the helper function in both places to ensure they stay in
sync.

2. Missing Reason for Restartpoints 
In LogCheckpointEnd, the checkpoint reason is added to the checkpoint complete message, but the restartpoint complete
messageremains unchanged. I think users would find the reason for restartpoints just as useful for debugging.
 

3. Global Variable vs. Passing Flags: Instead of using a file-level global checkpoint_reason_str to pass data between
thestart and end functions, would it make sense to simply pass the flags as a new argument to LogCheckpointEnd(bool
restartpoint,int flags)? 
 
This would allow you to call the helper function directly inside the ereport calls and avoid maintaining global state.

4. Whitespace Errors: I noticed a few whitespace warnings (space before tabs) when applying the patch. 
/home/carol/0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch:69: space before tab in indent.
            CheckpointReasonString(flags),
/home/carol/0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch:70: space before tab in indent.
            sizeof(checkpoint_reason_str));
/home/carol/0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch:88: space before tab in indent.
                (errmsg("checkpoint starting:%s", checkpoint_reason_str)));
warning: 3 lines add whitespace errors.

The patch can be compiled successfully and can pass make check -C src/test/recovery/

Regards,
Yuan Li(carol)

В списке pgsql-hackers по дате отправления: