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