On Thu, Aug 4, 2022 at 6:02 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <robertmh...@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
