Date: Mon, 1 Jul 2024 13:52:50 -0700
From: Noah Misch <noah@leadboat.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Peter Smith <smithpb2250@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	vignesh C <vignesh21@gmail.com>, Andres Freund <andres@anarazel.de>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
	Melanie Plageman <melanieplageman@gmail.com>
Subject: Re: Relation bulk write facility
Message-ID: <20240701205250.62.nmisch@google.com>
References: <30e8f366-58b3-b239-c521-422122dd5150@iki.fi>
 <c8bbc68e-1e5f-4d62-9c3a-7513f4b3ce72@iki.fi>
 <20231119000416.fuyrbo6av7dytmf6@awork3.anarazel.de>
 <93e91b02-e07f-4205-8de6-3942b91760cb@iki.fi>
 <CALDaNm0WYTR4mnvb8nRCWBinFw1kP9x0aSqmdswHj_Nr50YQug@mail.gmail.com>
 <98832b4f-441f-4691-b9d4-9725eea8aed9@iki.fi>
 <CAHut+Pt3WatNznD8q4wmm1GSR7vAHLNBmHERAmguH9N9LCBx1Q@mail.gmail.com>
 <6db39a5e-67ab-495d-9dfb-38b596df0d58@iki.fi>
 <a9708f51-e8db-44d7-b522-a90e0b0e40e1@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a9708f51-e8db-44d7-b522-a90e0b0e40e1@iki.fi>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: <https://www.postgresql.org/message-id/20240701205250.62.nmisch%40google.com>
Precedence: bulk

On Fri, Feb 23, 2024 at 04:27:34PM +0200, Heikki Linnakangas wrote:
> Committed this. Thanks everyone!

Commit 8af2565 wrote:
> --- /dev/null
> +++ b/src/backend/storage/smgr/bulk_write.c

> +/*
> + * Finish bulk write operation.
> + *
> + * This WAL-logs and flushes any remaining pending writes to disk, and fsyncs
> + * the relation if needed.
> + */
> +void
> +smgr_bulk_finish(BulkWriteState *bulkstate)
> +{
> +	/* WAL-log and flush any remaining pages */
> +	smgr_bulk_flush(bulkstate);
> +
> +	/*
> +	 * When we wrote out the pages, we passed skipFsync=true to avoid the
> +	 * overhead of registering all the writes with the checkpointer.  Register
> +	 * the whole relation now.
> +	 *
> +	 * There is one hole in that idea: If a checkpoint occurred while we were
> +	 * writing the pages, it already missed fsyncing the pages we had written
> +	 * before the checkpoint started.  A crash later on would replay the WAL
> +	 * starting from the checkpoint, therefore it wouldn't replay our earlier
> +	 * WAL records.  So if a checkpoint started after the bulk write, fsync
> +	 * the files now.
> +	 */
> +	if (!SmgrIsTemp(bulkstate->smgr))
> +	{

Shouldn't this be "if (bulkstate->use_wal)"?  The GetRedoRecPtr()-based
decision is irrelevant to the !wal case.  Either we don't need fsync at all
(TEMP or UNLOGGED) or smgrDoPendingSyncs() will do it (wal_level=minimal).  I
don't see any functional problem, but this likely arranges for an unnecessary
sync when a checkpoint starts between mdcreate() and here.  (The mdcreate()
sync may also be unnecessary, but that's longstanding.)

> +		/*
> +		 * Prevent a checkpoint from starting between the GetRedoRecPtr() and
> +		 * smgrregistersync() calls.
> +		 */
> +		Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0);
> +		MyProc->delayChkptFlags |= DELAY_CHKPT_START;
> +
> +		if (bulkstate->start_RedoRecPtr != GetRedoRecPtr())
> +		{
> +			/*
> +			 * A checkpoint occurred and it didn't know about our writes, so
> +			 * fsync() the relation ourselves.
> +			 */
> +			MyProc->delayChkptFlags &= ~DELAY_CHKPT_START;
> +			smgrimmedsync(bulkstate->smgr, bulkstate->forknum);
> +			elog(DEBUG1, "flushed relation because a checkpoint occurred concurrently");
> +		}
> +		else
> +		{
> +			smgrregistersync(bulkstate->smgr, bulkstate->forknum);
> +			MyProc->delayChkptFlags &= ~DELAY_CHKPT_START;
> +		}
> +	}
> +}

This is an elegant optimization.