Horton

Thread

Commits

  1. Fix tuple_data_split() to not open a relation without any lock.

  2. Lock relation used to generate fresh data for RMV.

  3. Fix ALTER COLUMN TYPE to not open a relation without any lock.

  1. Relations being opened without any lock whatever

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-30T19:20:44Z

    Running the regression tests with the patch I showed in
https://www.postgresql.org/message-id/16565.1538327894@sss.pgh.pa.us
exposes two places where HEAD is opening relations without having
any lock at all on them:

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.)

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.

Thoughts, objections?

			regards, tom lane

  2. Re: Relations being opened without any lock whatever

    Michael Paquier <michael@paquier.xyz> — 2018-09-30T23:29:19Z

    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

  3. Re: Relations being opened without any lock whatever

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T00:08:28Z

    Michael Paquier <michael@paquier.xyz> writes:
> Okay, that's bad.  Wouldn't it be sufficient to use what the caller
> passes out as lockmode instead of enforcing AEL though?

No, because at the bottom of that function we're going to do a DROP
CONSTRAINT on the old FK constraint, and that needs AEL anyway.
If we tried to take a lesser lock first we'd just be creating a
lock-upgrade deadlock risk.

			regards, tom lane

  4. Re: Relations being opened without any lock whatever

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-01T07:19:58Z

    On 2018/10/01 4:20, Tom Lane wrote:
> Running the regression tests with the patch I showed in
> https://www.postgresql.org/message-id/16565.1538327894@sss.pgh.pa.us
> exposes two places where HEAD is opening relations without having
> any lock at all on them:

Maybe you've noticed but the relation_open calls coming from bootstrap.c
all pass NoLock which trigger the WARNING:

$ initdb -D /tmp/foo
<snip>
WARNING:  relation_open: no lock held on pg_type
WARNING:  relation_open: no lock held on pg_attrdef
WARNING:  relation_open: no lock held on pg_constraint
WARNING:  relation_open: no lock held on pg_inherits
WARNING:  relation_open: no lock held on pg_index
WARNING:  relation_open: no lock held on pg_operator
WARNING:  relation_open: no lock held on pg_opfamily
<so on>

Do we need to do something about that, like teaching boot_openrel() and
gettype() in bootstrap.c to pass AccessShareLock instead of NoLock?

Thanks,
Amit

  5. Re: Relations being opened without any lock whatever

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T13:29:18Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/10/01 4:20, Tom Lane wrote:
>> Running the regression tests with the patch I showed in
>> https://www.postgresql.org/message-id/16565.1538327894@sss.pgh.pa.us

> Maybe you've noticed but the relation_open calls coming from bootstrap.c
> all pass NoLock which trigger the WARNING:

Yeah, I'd missed noticing that at the time I posted that patch, but I
sure noticed after changing the WARNING to an Assert ;-)

> Do we need to do something about that, like teaching boot_openrel() and
> gettype() in bootstrap.c to pass AccessShareLock instead of NoLock?

No, bootstrap mode has no need for locking.  I think the right fix is
just to skip the check:
 
+	/*
+	 * If we didn't get the lock ourselves, assert that caller holds one,
+	 * except in bootstrap mode where no locks are used.
+	 */
+	Assert(lockmode != NoLock ||
+		   IsBootstrapProcessingMode() ||
+		   CheckRelationLockedByMe(r, AccessShareLock, true));

It's possible that at some point we'd decide to make bootstrap mode
do locking the same as normal mode, but that's not a change I want
to make as part of this patch.

			regards, tom lane