Date: Fri, 6 Jun 2025 11:33:36 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Christoph Berg <myon@debian.org>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>,
	Andres Freund <andres@anarazel.de>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: CHECKPOINT unlogged data
Message-ID: <aEMYYEkQb2VeQoBo@nathan>
References: <aDnaKTEf-0dLiEfz@msg.df7cb.de>
 <vp4cewuyo2cw4fk5sxjaovllhe3dscixxo2utu3cgqhzydrqec@wc2nhgx4khpk>
 <aDnl5N1u7UBYVaSt@nathan>
 <aDnpeJ99HhoMgJ40@msg.df7cb.de>
 <7x6gsmev36zzh6lfzydkddbhpnxqgu2k2l2ci2k6foxhrstvj2@bn55g54wjrl4>
 <aEK88AwaYBNK1p-O@msg.df7cb.de>
 <fc1ed66f-e95a-4d10-a4f7-7fa4bb9a7084@oss.nttdata.com>
 <aEL6sDr9RxdSkPm-@msg.df7cb.de>
 <aEMIrLUDOKgmE_0P@nathan>
 <aEMVRbmqqg-aaxAN@msg.df7cb.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <aEMVRbmqqg-aaxAN@msg.df7cb.de>
Archived-At: <https://www.postgresql.org/message-id/aEMYYEkQb2VeQoBo%40nathan>
Precedence: bulk

On Fri, Jun 06, 2025 at 06:20:21PM +0200, Christoph Berg wrote:
> Re: Nathan Bossart
>> I don't understand why we need to add both FAST and IMMEDIATE.
> 
> We have both:
> 
> =# checkpoint ;
> 2025-06-06 18:09:25.743 CEST [872379] LOG:  checkpoint starting: immediate force wait
> 
> pg_basebackup --checkpoint=fast
> 
> Could we settle for one official name for that? Then we could use that
> name in all contexts.

That seems like a good idea to me.  I'm tempted to say that "fast" more
accurately describes what's happening than "immediate."  "Immediate" sounds
like it happens instantaneously, but it's actually just happening "fast,"
i.e., as fast as possible.

>> > +    <term><literal>FLUSH_ALL</literal></term>
>> 
>> Could we rename this to something like FLUSH_UNLOGGED or INCLUDE_UNLOGGED?
>> IMHO that's more descriptive.
> 
> That's again coming from what the log message is saying:
> 
> =# checkpoint (flush_all);
> 2025-06-06 18:12:46.298 CEST [873436] LOG:  checkpoint starting: immediate force wait flush-all
> 
> I think we should be consistent there.
> 
> #define CHECKPOINT_FLUSH_ALL    0x0010  /* Flush all pages, including those
>                                          * belonging to unlogged tables */
> 
> Maybe CHECKPOINT_FLUSH_UNLOGGED would be more explicit?

WFM.

>> My attempt at this patch back in 2020 included the following note, which
>> seems relevant here:
>> 
>> +   Note that the server may consolidate concurrently requested checkpoints or
>> +   restartpoints.  Such consolidated requests will contain a combined set of
>> +   options.  For example, if one session requested an immediate checkpoint and
>> +   another session requested a non-immediate checkpoint, the server may combine
>> +   these requests and perform one immediate checkpoint.
> 
> The CHECKPOINT documentation links to `28.5. WAL Configuration`,
> should this be mentioned there instead?

I thought it would make sense to put it closer to where these options are
described, since it'll be most evident for manually-initiated checkpoints.

>> We might also want to make sure it's clear that CHECKPOINT does nothing if
>> there's been no database activity since the last one (or, in the case of a
>> restartpoint, if there hasn't been a checkpoint record).
> 
> That's taken care of by "force":
> 
> #define CHECKPOINT_FORCE        0x0008  /* Force even if no activity */

Oh, I see that we always specify that for CHECKPOINT commands, except for
restartpoints.  IIRC even if you do specify CHECKPOINT_FORCE for a
restartpoint, it'll have no effect.  It's proably worth mentioning that
case, at least.

-- 
nathan