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.
