Horton

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

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
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-05T02:24:30Z
Lists: pgsql-hackers 
On Thu, Aug 4, 2022 at 6:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > I have reviewed this patch and I don't see a problem with it. However,
> > it would be nice if Andres or someone else who understands this area
> > well (Tom? Thomas?) would also review it, because I also reviewed
> > what's in the tree now and that turns out to be buggy, which leads me
> > to conclude that I don't understand this area as well as would be
> > desirable.
>
> FWIW, I approve of getting rid of the use of CreateFakeRelcacheEntry
> here, because I do not think that mechanism is meant to be used
> outside of WAL replay.  However, this patch fails to remove it from
> CreateAndCopyRelationData, which seems likely to be just as much
> at risk.

It looks to me like it does?

> The "invalidation" comment bothered me for awhile, but I think it's
> fine: we know that no other backend can connect to the source DB
> because we have it locked, and we know that no other backend can
> connect to the destination DB because it doesn't exist yet according
> to the catalogs, so nothing could possibly occur to invalidate our
> idea of where the physical files are.  It would be nice to document
> these assumptions, though, rather than merely remove all the relevant
> commentary.

I don't think that's the point. We could always suffer a sinval reset
or a PROCSIGNAL_BARRIER_SMGRRELEASE. But since the code avoids ever
reusing the smgr, it should be OK. I think.

> While I'm at it, I would like to strenuously object to the current
> framing of CreateAndCopyRelationData as a general-purpose copying
> mechanism.  Because of the above assumptions, I think it's utterly
> unsafe to use anywhere except in CREATE DATABASE.  The header comment
> fails to warn about that at all, and placing it in bufmgr.c rather
> than static in dbcommands.c is just an invitation to future misuse.
> Perhaps I'm overly sensitive to that because I just finished cleaning
> up somebody's misuse of non-general-purpose code (1aa8dad41), but
> as this stands I think it's positively dangerous.

OK. No objection to you revising the comments however you feel is appropriate.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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.