Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit b04aeb0

Browse filesBrowse files
tglsfdctglsfdc
committed
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
1 parent b66827c commit b04aeb0
Copy full SHA for b04aeb0

File tree

Expand file treeCollapse file tree

7 files changed

+95
-3
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

7 files changed

+95
-3
lines changed
Open diff view settings
Collapse file

‎src/backend/access/heap/heapam.c‎

Copy file name to clipboardExpand all lines: src/backend/access/heap/heapam.c
+14Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1137,6 +1137,14 @@ relation_open(Oid relationId, LOCKMODE lockmode)
11371137
if (!RelationIsValid(r))
11381138
elog(ERROR, "could not open relation with OID %u", relationId);
11391139

1140+
/*
1141+
* If we didn't get the lock ourselves, assert that caller holds one,
1142+
* except in bootstrap mode where no locks are used.
1143+
*/
1144+
Assert(lockmode != NoLock ||
1145+
IsBootstrapProcessingMode() ||
1146+
CheckRelationLockedByMe(r, AccessShareLock, true));
1147+
11401148
/* Make note that we've accessed a temporary relation */
11411149
if (RelationUsesLocalBuffers(r))
11421150
MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPREL;
@@ -1183,6 +1191,10 @@ try_relation_open(Oid relationId, LOCKMODE lockmode)
11831191
if (!RelationIsValid(r))
11841192
elog(ERROR, "could not open relation with OID %u", relationId);
11851193

1194+
/* If we didn't get the lock ourselves, assert that caller holds one */
1195+
Assert(lockmode != NoLock ||
1196+
CheckRelationLockedByMe(r, AccessShareLock, true));
1197+
11861198
/* Make note that we've accessed a temporary relation */
11871199
if (RelationUsesLocalBuffers(r))
11881200
MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPREL;
@@ -8084,6 +8096,8 @@ ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_changed, bool *
80848096

80858097
idx_rel = RelationIdGetRelation(replidindex);
80868098

8099+
Assert(CheckRelationLockedByMe(idx_rel, AccessShareLock, true));
8100+
80878101
/* deform tuple, so we have fast access to columns */
80888102
heap_deform_tuple(tp, desc, values, nulls);
80898103

Collapse file

‎src/backend/parser/parse_relation.c‎

Copy file name to clipboardExpand all lines: src/backend/parser/parse_relation.c
+3-1Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "parser/parse_enr.h"
2929
#include "parser/parse_relation.h"
3030
#include "parser/parse_type.h"
31+
#include "storage/lmgr.h"
3132
#include "utils/builtins.h"
3233
#include "utils/lsyscache.h"
3334
#include "utils/rel.h"
@@ -1272,7 +1273,7 @@ addRangeTableEntry(ParseState *pstate,
12721273
*
12731274
* lockmode is the lock type required for query execution; it must be one
12741275
* of AccessShareLock, RowShareLock, or RowExclusiveLock depending on the
1275-
* RTE's role within the query. The caller should always hold that lock mode
1276+
* RTE's role within the query. The caller must hold that lock mode
12761277
* or a stronger one.
12771278
*
12781279
* Note: properly, lockmode should be declared LOCKMODE not int, but that
@@ -1295,6 +1296,7 @@ addRangeTableEntryForRelation(ParseState *pstate,
12951296
Assert(lockmode == AccessShareLock ||
12961297
lockmode == RowShareLock ||
12971298
lockmode == RowExclusiveLock);
1299+
Assert(CheckRelationLockedByMe(rel, lockmode, true));
12981300

12991301
rte->rtekind = RTE_RELATION;
13001302
rte->alias = alias;
Collapse file

‎src/backend/storage/lmgr/lmgr.c‎

Copy file name to clipboardExpand all lines: src/backend/storage/lmgr/lmgr.c
+45Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,51 @@ UnlockRelation(Relation relation, LOCKMODE lockmode)
287287
LockRelease(&tag, lockmode, false);
288288
}
289289

290+
/*
291+
* CheckRelationLockedByMe
292+
*
293+
* Returns true if current transaction holds a lock on 'relation' of mode
294+
* 'lockmode'. If 'orstronger' is true, a stronger lockmode is also OK.
295+
* ("Stronger" is defined as "numerically higher", which is a bit
296+
* semantically dubious but is OK for the purposes we use this for.)
297+
*/
298+
bool
299+
CheckRelationLockedByMe(Relation relation, LOCKMODE lockmode, bool orstronger)
300+
{
301+
LOCKTAG tag;
302+
303+
SET_LOCKTAG_RELATION(tag,
304+
relation->rd_lockInfo.lockRelId.dbId,
305+
relation->rd_lockInfo.lockRelId.relId);
306+
307+
if (LockHeldByMe(&tag, lockmode))
308+
return true;
309+
310+
if (orstronger)
311+
{
312+
LOCKMODE slockmode;
313+
314+
for (slockmode = lockmode + 1;
315+
slockmode <= MaxLockMode;
316+
slockmode++)
317+
{
318+
if (LockHeldByMe(&tag, slockmode))
319+
{
320+
#ifdef NOT_USED
321+
/* Sometimes this might be useful for debugging purposes */
322+
elog(WARNING, "lock mode %s substituted for %s on relation %s",
323+
GetLockmodeName(tag.locktag_lockmethodid, slockmode),
324+
GetLockmodeName(tag.locktag_lockmethodid, lockmode),
325+
RelationGetRelationName(relation));
326+
#endif
327+
return true;
328+
}
329+
}
330+
}
331+
332+
return false;
333+
}
334+
290335
/*
291336
* LockHasWaitersRelation
292337
*
Collapse file

‎src/backend/storage/lmgr/lock.c‎

Copy file name to clipboardExpand all lines: src/backend/storage/lmgr/lock.c
+24Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,30 @@ DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2)
563563
return false;
564564
}
565565

566+
/*
567+
* LockHeldByMe -- test whether lock 'locktag' is held with mode 'lockmode'
568+
* by the current transaction
569+
*/
570+
bool
571+
LockHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode)
572+
{
573+
LOCALLOCKTAG localtag;
574+
LOCALLOCK *locallock;
575+
576+
/*
577+
* See if there is a LOCALLOCK entry for this lock and lockmode
578+
*/
579+
MemSet(&localtag, 0, sizeof(localtag)); /* must clear padding */
580+
localtag.lock = *locktag;
581+
localtag.mode = lockmode;
582+
583+
locallock = (LOCALLOCK *) hash_search(LockMethodLocalHash,
584+
(void *) &localtag,
585+
HASH_FIND, NULL);
586+
587+
return (locallock && locallock->nLocks > 0);
588+
}
589+
566590
/*
567591
* LockHasWaiters -- look up 'locktag' and check if releasing this
568592
* lock would wake up other processes waiting for it.
Collapse file

‎src/include/storage/lmgr.h‎

Copy file name to clipboardExpand all lines: src/include/storage/lmgr.h
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@ extern void UnlockRelationOid(Oid relid, LOCKMODE lockmode);
4545
extern void LockRelation(Relation relation, LOCKMODE lockmode);
4646
extern bool ConditionalLockRelation(Relation relation, LOCKMODE lockmode);
4747
extern void UnlockRelation(Relation relation, LOCKMODE lockmode);
48+
extern bool CheckRelationLockedByMe(Relation relation, LOCKMODE lockmode,
49+
bool orstronger);
4850
extern bool LockHasWaitersRelation(Relation relation, LOCKMODE lockmode);
4951

5052
extern void LockRelationIdForSession(LockRelId *relid, LOCKMODE lockmode);
Collapse file

‎src/include/storage/lock.h‎

Copy file name to clipboardExpand all lines: src/include/storage/lock.h
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -540,6 +540,7 @@ extern void LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks);
540540
extern void LockReleaseSession(LOCKMETHODID lockmethodid);
541541
extern void LockReleaseCurrentOwner(LOCALLOCK **locallocks, int nlocks);
542542
extern void LockReassignCurrentOwner(LOCALLOCK **locallocks, int nlocks);
543+
extern bool LockHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode);
543544
extern bool LockHasWaiters(const LOCKTAG *locktag,
544545
LOCKMODE lockmode, bool sessionLock);
545546
extern VirtualTransactionId *GetLockConflicts(const LOCKTAG *locktag,
Collapse file

‎src/include/storage/lockdefs.h‎

Copy file name to clipboardExpand all lines: src/include/storage/lockdefs.h
+6-2Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,15 @@ typedef int LOCKMODE;
4545
#define AccessExclusiveLock 8 /* ALTER TABLE, DROP TABLE, VACUUM FULL,
4646
* and unqualified LOCK TABLE */
4747

48+
#define MaxLockMode 8
49+
50+
51+
/* WAL representation of an AccessExclusiveLock on a table */
4852
typedef struct xl_standby_lock
4953
{
5054
TransactionId xid; /* xid of holder of AccessExclusiveLock */
51-
Oid dbOid;
52-
Oid relOid;
55+
Oid dbOid; /* DB containing table */
56+
Oid relOid; /* OID of table */
5357
} xl_standby_lock;
5458

5559
#endif /* LOCKDEF_H_ */

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.