pgsql: Fix inadequate locking during get_rel_oids().

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-committers(at)postgresql(dot)org
Subject: pgsql: Fix inadequate locking during get_rel_oids().
Date: 2017-09-29 20:26:42
Message-ID: E1dy1ru-0002RK-8q@gemulon.postgresql.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

Fix inadequate locking during get_rel_oids().

get_rel_oids used to not take any relation locks at all, but that stopped
being a good idea with commit 3c3bb9933, which inserted a syscache lookup
into the function. A concurrent DROP TABLE could now produce "cache lookup
failed", which we don't want to have happen in normal operation. The best
solution seems to be to transiently take a lock on the relation named by
the RangeVar (which also makes the result of RangeVarGetRelid a lot less
spongy). But we shouldn't hold the lock beyond this function, because we
don't want VACUUM to lock more than one table at a time. (That would not
be a big problem right now, but it will become one after the pending
feature patch to allow multiple tables to be named in VACUUM.)

In passing, adjust vacuum_rel and analyze_rel to document that we don't
trust the passed RangeVar to be accurate, and allow the RangeVar to
possibly be NULL --- which it is anyway for a whole-database VACUUM,
though we accidentally didn't crash for that case.

The passed RangeVar is in fact inaccurate when dealing with a child
partition, as of v10, and it has been wrong for a whole long time in the
case of vacuum_rel() recursing to a TOAST table. None of these things
present visible bugs up to now, because the passed RangeVar is in fact
only consulted for autovacuum logging, and in that particular context it's
always accurate because autovacuum doesn't let vacuum.c expand partitions
nor recurse to toast tables. Still, this seems like trouble waiting to
happen, so let's nail the door at least partly shut. (Further cleanup
is planned, in HEAD only, as part of the pending feature patch.)

Fix some sadly inaccurate/obsolete comments too. Back-patch to v10.

Michael Paquier and Tom Lane

Discussion: https://postgr.es/m/25023.1506107590@sss.pgh.pa.us

Branch
------
REL_10_STABLE

Details
-------
https://git.postgresql.org/pg/commitdiff/2aab70205be012d06f7d077dd1fa5e6afea9d19c

Modified Files
--------------
src/backend/commands/analyze.c | 7 ++++-
src/backend/commands/vacuum.c | 60 +++++++++++++++++++++++++++---------------
2 files changed, 45 insertions(+), 22 deletions(-)

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Peter Eisentraut 2017-09-29 20:52:35 pgsql: Use Py_RETURN_NONE where suitable
Previous Message Robert Haas 2017-09-29 20:03:38 pgsql: psql: Don't try to print a partition constraint we didn't fetch.

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2017-09-29 20:33:04 Re: pg_prepared_xact_status
Previous Message David Fetter 2017-09-29 20:19:03 Re: alter server for foreign table