Thread: pgsql: Add assertions that we hold some relevant lock during relationo
Add assertions that we hold some relevant lock during relation open. Opening a relation with no lock at all is unsafe; there's no guarantee that we'll see a consistent state of the relevant catalog entries. While use of MVCC scans to read the catalogs partially addresses that complaint, it's still possible to switch to a new catalog snapshot partway through loading the relcache entry. Moreover, whether or not you trust the reasoning behind sometimes using less than AccessExclusiveLock for ALTER TABLE, that reasoning is certainly not valid if concurrent users of the table don't hold a lock corresponding to the operation they want to perform. Hence, add some assertion-build-only checks that require any caller of relation_open(x, NoLock) to hold at least AccessShareLock. This isn't a full solution, since we can't verify that the lock level is semantically appropriate for the action --- but it's definitely of some use, because it's already caught two bugs. We can also assert that callers of addRangeTableEntryForRelation() hold at least the lock level specified for the new RTE. Amit Langote and Tom Lane Discussion: https://postgr.es/m/16565.1538327894@sss.pgh.pa.us Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/b04aeb0a053e7cf7faad89f7d47844d8ba0dc839 Modified Files -------------- src/backend/access/heap/heapam.c | 14 ++++++++++++ src/backend/parser/parse_relation.c | 4 +++- src/backend/storage/lmgr/lmgr.c | 45 +++++++++++++++++++++++++++++++++++++ src/backend/storage/lmgr/lock.c | 24 ++++++++++++++++++++ src/include/storage/lmgr.h | 2 ++ src/include/storage/lock.h | 1 + src/include/storage/lockdefs.h | 8 +++++-- 7 files changed, 95 insertions(+), 3 deletions(-)
On 2018/10/02 1:43, Tom Lane wrote: > Add assertions that we hold some relevant lock during relation open. > > Opening a relation with no lock at all is unsafe; there's no guarantee > that we'll see a consistent state of the relevant catalog entries. > While use of MVCC scans to read the catalogs partially addresses that > complaint, it's still possible to switch to a new catalog snapshot > partway through loading the relcache entry. Moreover, whether or not > you trust the reasoning behind sometimes using less than > AccessExclusiveLock for ALTER TABLE, that reasoning is certainly not > valid if concurrent users of the table don't hold a lock corresponding > to the operation they want to perform. > > Hence, add some assertion-build-only checks that require any caller > of relation_open(x, NoLock) to hold at least AccessShareLock. This > isn't a full solution, since we can't verify that the lock level is > semantically appropriate for the action --- but it's definitely of > some use, because it's already caught two bugs. > > We can also assert that callers of addRangeTableEntryForRelation() > hold at least the lock level specified for the new RTE. > > Amit Langote and Tom Lane > > Discussion: https://postgr.es/m/16565.1538327894@sss.pgh.pa.us Thanks for committing. I'm sorry if it wasn't clear, but the lock manager code was added to the original 0001 patch by David Rowley; see this message where he posted the updated version of my patch containing the new code: https://www.postgresql.org/message-id/CAKJS1f83VJjrnN6hQtZ7vJ%2BE4_%2BtH%3DU2VK6hgr74fZN-%3DDq3Ag%40mail.gmail.com Thanks, Amit
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/10/02 1:43, Tom Lane wrote:
>> Add assertions that we hold some relevant lock during relation open.
>> ...
>> Amit Langote and Tom Lane
> I'm sorry if it wasn't clear, but the lock manager code was added to the
> original 0001 patch by David Rowley; see this message where he posted the
> updated version of my patch containing the new code:
Wups. My apologies to David for not researching the pieces of the patch
more closely.
regards, tom lane