Horton

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>, Andres Freund <andres@anarazel.de>, Justin Pryzby <pryzby@telsasoft.com>, Ashutosh Sharma <ashu.coek88@gmail.com>, Maciek Sakrejda <m.sakrejda@gmail.com>, Bruce Momjian <bruce@momjian.us>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Andrew Dunstan <andrew@dunslane.net>, Heikki Linnakangas <hlinnaka@iki.fi>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Thomas Munro <thomas.munro@gmail.com>
Date: 2022-08-04T23:01:06Z
Lists: pgsql-hackers 
I wrote:
> While I'm at it, I would like to strenuously object to the current
> framing of CreateAndCopyRelationData as a general-purpose copying
> mechanism.

And while I'm piling on, how is this bit in RelationCopyStorageUsingBuffer
not completely broken?

        /* Read block from source relation. */
        srcBuf = ReadBufferWithoutRelcache(src->rd_locator, forkNum, blkno,
                                           RBM_NORMAL, bstrategy_src,
                                           permanent);
        srcPage = BufferGetPage(srcBuf);
        if (PageIsNew(srcPage) || PageIsEmpty(srcPage))
        {
            ReleaseBuffer(srcBuf);
            continue;
        }

        /* Use P_NEW to extend the destination relation. */
        dstBuf = ReadBufferWithoutRelcache(dst->rd_locator, forkNum, P_NEW,
                                           RBM_NORMAL, bstrategy_dst,
                                           permanent);

You can't skip pages just because they are empty.  Well, maybe you could
if you were doing something to ensure that you zero-fill the corresponding
blocks on the destination side.  But this isn't doing that.  It's using
P_NEW for dstBuf, which will have the effect of silently collapsing out
such pages.  Maybe in isolation a heap could withstand that, but its
indexes won't be happy (and I guess t_ctid chain links won't either).

I think you should just lose the if() stanza.  There's no optimization to
be had here that's worth any extra complication.

(This seems worth fixing before beta3, as it looks like a rather
nasty data corruption hazard.)

			regards, tom lane

Commits

  1. When using the WAL-logged CREATE DATABASE strategy, bulk extend.

  2. Avoid using a fake relcache entry to own an SmgrRelation.

  3. Fix data-corruption hazard in WAL-logged CREATE DATABASE.

  4. initdb: When running CREATE DATABASE, use STRATEGY = WAL_COPY.

  5. Simplify a needlessly-complicated regular expression.

  6. In 020_createdb.pl, change order of command-line arguments.

  7. Add new block-by-block strategy for CREATE DATABASE.

  8. Fix replay of create database records on standby

  9. Refactor code for reading and writing relation map files.

  10. Replace RelationOpenSmgr() with RelationGetSmgr().

  11. Refactor the fsync queue for wider use.