On Sun, Sep 30, 2018 at 03:20:44PM -0400, Tom Lane wrote: > 1. ALTER TABLE ... ALTER COLUMN TYPE, on a column that is part of > a foreign key constraint, opens the rel that has the other end of > the constraint before it's acquired a lock on said rel. > > The comment in ATPostAlterTypeCleanup claims this is "safe because the > parser won't actually look at the catalogs to detect the existing entry", > but I think that's largely horsepucky. The parser absolutely does do > relation_open, and it expects the caller to have gotten a lock sufficient > to protect that (cf transformAlterTableStmt). > > It's possible that this doesn't have any real effect. Since we're > already holding AccessExclusiveLock on our own end of the FK constraint, > it'd be impossible for another session to drop the FK constraint, or > by extension the other table. Still, running a relcache load on a > table we have no lock on seems pretty unsafe, especially so in branches > before we used MVCC for catalog reads. So I'm inclined to apply the > attached patch all the way back. (The mentioned comment also needs > rewritten; this is just the minimal code change to get rid of the test > failure.)
Okay, that's bad. Wouldn't it be sufficient to use what the caller passes out as lockmode instead of enforcing AEL though? > 2. pageinspect's tuple_data_split_internal supposes that it needs no > lock on the referenced table. Perhaps there was an expectation that > some earlier function would have taken a lock and not released it, > but this is demonstrably not happening in the module's own regression > test. I think we should just take AccessShareLock there and not try > to be cute. Again, this seems to be back-patch material. Yes, that's incorrect. So +1 on this one. -- Michael
signature.asc
Description: PGP signature