Skip to content

Commit 00d7fb5

Browse files
jeff-davisjeff-davis
committed
Assert that buffers are marked dirty before XLogRegisterBuffer().
Enforce the rule from transam/README in XLogRegisterBuffer(), and update callers to follow the rule. Hash indexes sometimes register clean pages as a part of the locking protocol, so provide a REGBUF_NO_CHANGE flag to support that use. Discussion: https://postgr.es/m/c84114f8-c7f1-5b57-f85a-3adc31e1a904@iki.fi Reviewed-by: Heikki Linnakangas
1 parent befe945 commit 00d7fb5

File tree

10 files changed

+118
-20
lines changed

10 files changed

+118
-20
lines changed

‎src/backend/access/gin/ginbtree.c

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -387,24 +387,22 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
387387
START_CRIT_SECTION();
388388

389389
if (RelationNeedsWAL(btree->index) && !btree->isBuild)
390-
{
391390
XLogBeginInsert();
392-
XLogRegisterBuffer(0, stack->buffer, REGBUF_STANDARD);
393-
if (BufferIsValid(childbuf))
394-
XLogRegisterBuffer(1, childbuf, REGBUF_STANDARD);
395-
}
396391

397-
/* Perform the page update, and register any extra WAL data */
392+
/*
393+
* Perform the page update, dirty and register stack->buffer, and
394+
* register any extra WAL data.
395+
*/
398396
btree->execPlaceToPage(btree, stack->buffer, stack,
399397
insertdata, updateblkno, ptp_workspace);
400398

401-
MarkBufferDirty(stack->buffer);
402-
403399
/* An insert to an internal page finishes the split of the child. */
404400
if (BufferIsValid(childbuf))
405401
{
406402
GinPageGetOpaque(childpage)->flags &= ~GIN_INCOMPLETE_SPLIT;
407403
MarkBufferDirty(childbuf);
404+
if (RelationNeedsWAL(btree->index) && !btree->isBuild)
405+
XLogRegisterBuffer(1, childbuf, REGBUF_STANDARD);
408406
}
409407

410408
if (RelationNeedsWAL(btree->index) && !btree->isBuild)

‎src/backend/access/gin/gindatapage.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -721,9 +721,12 @@ dataExecPlaceToPageLeaf(GinBtree btree, Buffer buf, GinBtreeStack *stack,
721721
/* Apply changes to page */
722722
dataPlaceToPageLeafRecompress(buf, leaf);
723723

724+
MarkBufferDirty(buf);
725+
724726
/* If needed, register WAL data built by computeLeafRecompressWALData */
725727
if (RelationNeedsWAL(btree->index) && !btree->isBuild)
726728
{
729+
XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
727730
XLogRegisterBufData(0, leaf->walinfo, leaf->walinfolen);
728731
}
729732
}
@@ -1155,6 +1158,8 @@ dataExecPlaceToPageInternal(GinBtree btree, Buffer buf, GinBtreeStack *stack,
11551158
pitem = (PostingItem *) insertdata;
11561159
GinDataPageAddPostingItem(page, pitem, off);
11571160

1161+
MarkBufferDirty(buf);
1162+
11581163
if (RelationNeedsWAL(btree->index) && !btree->isBuild)
11591164
{
11601165
/*
@@ -1167,6 +1172,7 @@ dataExecPlaceToPageInternal(GinBtree btree, Buffer buf, GinBtreeStack *stack,
11671172
data.offset = off;
11681173
data.newitem = *pitem;
11691174

1175+
XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
11701176
XLogRegisterBufData(0, (char *) &data,
11711177
sizeof(ginxlogInsertDataInternal));
11721178
}

‎src/backend/access/gin/ginentrypage.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -571,6 +571,8 @@ entryExecPlaceToPage(GinBtree btree, Buffer buf, GinBtreeStack *stack,
571571
elog(ERROR, "failed to add item to index page in \"%s\"",
572572
RelationGetRelationName(btree->index));
573573

574+
MarkBufferDirty(buf);
575+
574576
if (RelationNeedsWAL(btree->index) && !btree->isBuild)
575577
{
576578
/*
@@ -583,6 +585,7 @@ entryExecPlaceToPage(GinBtree btree, Buffer buf, GinBtreeStack *stack,
583585
data.isDelete = insertData->isDelete;
584586
data.offset = off;
585587

588+
XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
586589
XLogRegisterBufData(0, (char *) &data,
587590
offsetof(ginxlogInsertEntry, tuple));
588591
XLogRegisterBufData(0, (char *) insertData->entry,

‎src/backend/access/gin/ginfast.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -397,15 +397,16 @@ ginHeapTupleFastInsert(GinState *ginstate, GinTupleCollector *collector)
397397
}
398398

399399
Assert((ptr - collectordata) <= collector->sumsize);
400+
401+
MarkBufferDirty(buffer);
402+
400403
if (needWal)
401404
{
402405
XLogRegisterBuffer(1, buffer, REGBUF_STANDARD);
403406
XLogRegisterBufData(1, collectordata, collector->sumsize);
404407
}
405408

406409
metadata->tailFreeSize = PageGetExactFreeSpace(page);
407-
408-
MarkBufferDirty(buffer);
409410
}
410411

411412
/*

‎src/backend/access/hash/hash.c

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -824,11 +824,16 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf,
824824
XLogRegisterData((char *) &xlrec, SizeOfHashDelete);
825825

826826
/*
827-
* bucket buffer needs to be registered to ensure that we can
828-
* acquire a cleanup lock on it during replay.
827+
* bucket buffer was not changed, but still needs to be
828+
* registered to ensure that we can acquire a cleanup lock on
829+
* it during replay.
829830
*/
830831
if (!xlrec.is_primary_bucket_page)
831-
XLogRegisterBuffer(0, bucket_buf, REGBUF_STANDARD | REGBUF_NO_IMAGE);
832+
{
833+
uint8 flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_NO_CHANGE;
834+
835+
XLogRegisterBuffer(0, bucket_buf, flags);
836+
}
832837

833838
XLogRegisterBuffer(1, buf, REGBUF_STANDARD);
834839
XLogRegisterBufData(1, (char *) deletable,

‎src/backend/access/hash/hashovfl.c

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -658,11 +658,15 @@ _hash_freeovflpage(Relation rel, Buffer bucketbuf, Buffer ovflbuf,
658658
XLogRegisterData((char *) &xlrec, SizeOfHashSqueezePage);
659659

660660
/*
661-
* bucket buffer needs to be registered to ensure that we can acquire
662-
* a cleanup lock on it during replay.
661+
* bucket buffer was not changed, but still needs to be registered to
662+
* ensure that we can acquire a cleanup lock on it during replay.
663663
*/
664664
if (!xlrec.is_prim_bucket_same_wrt)
665-
XLogRegisterBuffer(0, bucketbuf, REGBUF_STANDARD | REGBUF_NO_IMAGE);
665+
{
666+
uint8 flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_NO_CHANGE;
667+
668+
XLogRegisterBuffer(0, bucketbuf, flags);
669+
}
666670

667671
XLogRegisterBuffer(1, wbuf, REGBUF_STANDARD);
668672
if (xlrec.ntups > 0)
@@ -960,11 +964,16 @@ _hash_squeezebucket(Relation rel,
960964
XLogRegisterData((char *) &xlrec, SizeOfHashMovePageContents);
961965

962966
/*
963-
* bucket buffer needs to be registered to ensure that
964-
* we can acquire a cleanup lock on it during replay.
967+
* bucket buffer was not changed, but still needs to
968+
* be registered to ensure that we can acquire a
969+
* cleanup lock on it during replay.
965970
*/
966971
if (!xlrec.is_prim_bucket_same_wrt)
967-
XLogRegisterBuffer(0, bucket_buf, REGBUF_STANDARD | REGBUF_NO_IMAGE);
972+
{
973+
int flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_NO_CHANGE;
974+
975+
XLogRegisterBuffer(0, bucket_buf, flags);
976+
}
968977

969978
XLogRegisterBuffer(1, wbuf, REGBUF_STANDARD);
970979
XLogRegisterBufData(1, (char *) itup_offsets,

‎src/backend/access/transam/xloginsert.c

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,20 @@ XLogRegisterBuffer(uint8 block_id, Buffer buffer, uint8 flags)
248248
Assert(!((flags & REGBUF_FORCE_IMAGE) && (flags & (REGBUF_NO_IMAGE))));
249249
Assert(begininsert_called);
250250

251+
/*
252+
* Ordinarily, buffer should be exclusive-locked and marked dirty before
253+
* we get here, otherwise we could end up violating one of the rules in
254+
* access/transam/README.
255+
*
256+
* Some callers intentionally register a clean page and never update that
257+
* page's LSN; in that case they can pass the flag REGBUF_NO_CHANGE to
258+
* bypass these checks.
259+
*/
260+
#ifdef USE_ASSERT_CHECKING
261+
if (!(flags & REGBUF_NO_CHANGE))
262+
Assert(BufferIsExclusiveLocked(buffer) && BufferIsDirty(buffer));
263+
#endif
264+
251265
if (block_id >= max_registered_block_id)
252266
{
253267
if (block_id >= max_registered_buffers)
@@ -1313,8 +1327,8 @@ log_newpage_range(Relation rel, ForkNumber forknum,
13131327
START_CRIT_SECTION();
13141328
for (i = 0; i < nbufs; i++)
13151329
{
1316-
XLogRegisterBuffer(i, bufpack[i], flags);
13171330
MarkBufferDirty(bufpack[i]);
1331+
XLogRegisterBuffer(i, bufpack[i], flags);
13181332
}
13191333

13201334
recptr = XLogInsert(RM_XLOG_ID, XLOG_FPI);

‎src/backend/storage/buffer/bufmgr.c

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2098,6 +2098,65 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
20982098
return first_block;
20992099
}
21002100

2101+
/*
2102+
* BufferIsExclusiveLocked
2103+
*
2104+
* Checks if buffer is exclusive-locked.
2105+
*
2106+
* Buffer must be pinned.
2107+
*/
2108+
bool
2109+
BufferIsExclusiveLocked(Buffer buffer)
2110+
{
2111+
BufferDesc *bufHdr;
2112+
2113+
if (BufferIsLocal(buffer))
2114+
{
2115+
int bufid = -buffer - 1;
2116+
2117+
bufHdr = GetLocalBufferDescriptor(bufid);
2118+
}
2119+
else
2120+
{
2121+
bufHdr = GetBufferDescriptor(buffer - 1);
2122+
}
2123+
2124+
Assert(BufferIsPinned(buffer));
2125+
return LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr),
2126+
LW_EXCLUSIVE);
2127+
}
2128+
2129+
/*
2130+
* BufferIsDirty
2131+
*
2132+
* Checks if buffer is already dirty.
2133+
*
2134+
* Buffer must be pinned and exclusive-locked. (Without an exclusive lock,
2135+
* the result may be stale before it's returned.)
2136+
*/
2137+
bool
2138+
BufferIsDirty(Buffer buffer)
2139+
{
2140+
BufferDesc *bufHdr;
2141+
2142+
if (BufferIsLocal(buffer))
2143+
{
2144+
int bufid = -buffer - 1;
2145+
2146+
bufHdr = GetLocalBufferDescriptor(bufid);
2147+
}
2148+
else
2149+
{
2150+
bufHdr = GetBufferDescriptor(buffer - 1);
2151+
}
2152+
2153+
Assert(BufferIsPinned(buffer));
2154+
Assert(LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr),
2155+
LW_EXCLUSIVE));
2156+
2157+
return pg_atomic_read_u32(&bufHdr->state) & BM_DIRTY;
2158+
}
2159+
21012160
/*
21022161
* MarkBufferDirty
21032162
*

‎src/include/access/xloginsert.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
* will be skipped) */
3838
#define REGBUF_KEEP_DATA 0x10 /* include data even if a full-page image
3939
* is taken */
40+
#define REGBUF_NO_CHANGE 0x20 /* intentionally register clean buffer */
4041

4142
/* prototypes for public functions in xloginsert.c: */
4243
extern void XLogBeginInsert(void);

‎src/include/storage/bufmgr.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,8 @@ extern Buffer ReadBufferWithoutRelcache(RelFileLocator rlocator,
179179
bool permanent);
180180
extern void ReleaseBuffer(Buffer buffer);
181181
extern void UnlockReleaseBuffer(Buffer buffer);
182+
extern bool BufferIsExclusiveLocked(Buffer buffer);
183+
extern bool BufferIsDirty(Buffer buffer);
182184
extern void MarkBufferDirty(Buffer buffer);
183185
extern void IncrBufferRefCount(Buffer buffer);
184186
extern void CheckBufferIsPinnedOnce(Buffer buffer);

0 commit comments

Comments
 (0)