Relations being opened without any lock whatever

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Relations being opened without any lock whatever
Date: 2018-09-30 19:20:44
Message-ID: 2038.1538335244@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
alter-table-fix.patch text/x-diff 1.4 KB
pageinspect-fix.patch text/x-diff 1.4 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Christoph Moench-Tegeder 2018-09-30 20:59:20 Function for listing archive_status directory
Previous Message Tom Lane 2018-09-30 17:18:14 Re: executor relation handling