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
