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.
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.
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.
regards, tom lane
