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
