NotificationsYou must be signed in to change notification settings
Fork6
Star31

Commitdd72882

committed

Fix locking problem in _hash_squeezebucket() / _hash_freeovflpage().

A bucket squeeze operation needs to lock each page of the bucketbefore releasing the prior page, but the previous coding fumbled thelocking when freeing an overflow page during a bucket squeezeoperation. Commit6d46f47introduced this bug.Amit Kapila, with help from Kuntal Ghosh and Dilip Kumar, afteran initial trouble report by Jeff Janes. Reviewed by me. I alsofixed a problem with a comment.

1 parent668dbbe commitdd72882Copy full SHA for dd72882

File tree

2 files changed

+13

-30

lines changed

src
- backend/access/hash
  - hashovfl.c
- include/access
  - hash.h

2 files changed

+13

-30

lines changed

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

Lines changed: 12 additions & 29 deletions

Original file line number	Diff line number	Diff line change
`@@ -377,12 +377,11 @@ _hash_firstfreebit(uint32 map)`
`377`	`377`	`*NB: caller must not hold lock on metapage, nor on page, that's next to`
`378`	`378`	`*ovflbuf in the bucket chain. We don't acquire the lock on page that's`
`379`	`379`	`*prior to ovflbuf in chain if it is same as wbuf because the caller already`
`380`		`- *has a lock on same. This function releases the lock on wbuf and caller`
`381`		`- *is responsible for releasing the pin on same.`
	`380`	`+ *has a lock on same.`
`382`	`381`	`*/`
`383`	`382`	`BlockNumber`
`384`	`383`	`_hash_freeovflpage(Relationrel,Bufferovflbuf,Bufferwbuf,`
`385`		`-boolwbuf_dirty,BufferAccessStrategybstrategy)`
	`384`	`+BufferAccessStrategybstrategy)`
`386`	`385`	`{`
`387`	`386`	`HashMetaPagemetap;`
`388`	`387`	`Buffermetabuf;`
`@@ -447,24 +446,10 @@ _hash_freeovflpage(Relation rel, Buffer ovflbuf, Buffer wbuf,`
`447`	`446`	`Assert(prevopaque->hasho_bucket==bucket);`
`448`	`447`	`prevopaque->hasho_nextblkno=nextblkno;`
`449`	`448`
	`449`	`+MarkBufferDirty(prevbuf);`
`450`	`450`	`if (prevblkno!=writeblkno)`
`451`		`-{`
`452`		`-MarkBufferDirty(prevbuf);`
`453`	`451`	`_hash_relbuf(rel,prevbuf);`
`454`		`-}`
`455`		`-else`
`456`		`-{`
`457`		`-/* ensure to mark prevbuf as dirty */`
`458`		`-wbuf_dirty= true;`
`459`		`-}`
`460`	`452`	`}`
`461`		`-`
`462`		`-/* write and unlock the write buffer */`
`463`		`-if (wbuf_dirty)`
`464`		`-_hash_chgbufaccess(rel,wbuf,HASH_WRITE,HASH_NOLOCK);`
`465`		`-else`
`466`		`-_hash_chgbufaccess(rel,wbuf,HASH_READ,HASH_NOLOCK);`
`467`		`-`
`468`	`453`	`if (BlockNumberIsValid(nextblkno))`
`469`	`454`	`{`
`470`	`455`	`Buffernextbuf=_hash_getbuf_with_strategy(rel,`
`@@ -783,30 +768,28 @@ _hash_squeezebucket(Relation rel,`
`783`	`768`	`* Tricky point here: if our read and write pages are adjacent in the`
`784`	`769`	`* bucket chain, our write lock on wbuf will conflict with`
`785`	`770`	`* _hash_freeovflpage's attempt to update the sibling links of the`
`786`		`- * removed page. In that case, we don't need to lock it again and we`
`787`		`- * always release the lock on wbuf in _hash_freeovflpage and then`
`788`		`- * retake it again here. This will not only simplify the code, but is`
`789`		`- * required to atomically log the changes which will be helpful when`
`790`		`- * we write WAL for hash indexes.`
	`771`	`+ * removed page. In that case, we don't need to lock it again.`
`791`	`772`	`*/`
`792`	`773`	`rblkno=ropaque->hasho_prevblkno;`
`793`	`774`	`Assert(BlockNumberIsValid(rblkno));`
`794`	`775`
`795`	`776`	`/* free this overflow page (releases rbuf) */`
`796`		`-_hash_freeovflpage(rel,rbuf,wbuf,wbuf_dirty,bstrategy);`
	`777`	`+_hash_freeovflpage(rel,rbuf,wbuf,bstrategy);`
	`778`	`+`
	`779`	`+if (wbuf_dirty)`
	`780`	`+MarkBufferDirty(wbuf);`
`797`	`781`
`798`	`782`	`/* are we freeing the page adjacent to wbuf? */`
`799`	`783`	`if (rblkno==wblkno)`
`800`	`784`	`{`
`801`	`785`	`/* retain the pin on primary bucket page till end of bucket scan */`
`802`		`-if (wblkno!=bucket_blkno)`
`803`		`-_hash_dropbuf(rel,wbuf);`
	`786`	`+if (wblkno==bucket_blkno)`
	`787`	`+_hash_chgbufaccess(rel,wbuf,HASH_READ,HASH_NOLOCK);`
	`788`	`+else`
	`789`	`+_hash_relbuf(rel,wbuf);`
`804`	`790`	`return;`
`805`	`791`	`}`
`806`	`792`
`807`		`-/* lock the overflow page being written, then get the previous one */`
`808`		`-_hash_chgbufaccess(rel,wbuf,HASH_NOLOCK,HASH_WRITE);`
`809`		`-`
`810`	`793`	`rbuf=_hash_getbuf_with_strategy(rel,`
`811`	`794`	`rblkno,`
`812`	`795`	`HASH_WRITE,`

`‎src/include/access/hash.h‎`

Lines changed: 1 addition & 1 deletion

Original file line number	Diff line number	Diff line change
`@@ -314,7 +314,7 @@ extern OffsetNumber _hash_pgaddtup(Relation rel, Buffer buf,`
`314`	`314`	`/* hashovfl.c */`
`315`	`315`	`externBuffer_hash_addovflpage(Relationrel,Buffermetabuf,Bufferbuf,boolretain_pin);`
`316`	`316`	`externBlockNumber_hash_freeovflpage(Relationrel,Bufferovflbuf,Bufferwbuf,`
`317`		`-boolwbuf_dirty,BufferAccessStrategybstrategy);`
	`317`	`+BufferAccessStrategybstrategy);`
`318`	`318`	`externvoid_hash_initbitmap(Relationrel,HashMetaPagemetap,`
`319`	`319`	`BlockNumberblkno,ForkNumberforkNum);`
`320`	`320`	`externvoid_hash_squeezebucket(Relationrel,`

0 commit comments

Comments

(0)

Movatterモバイル変換

Navigation Menu

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Commitdd72882

File tree

2 files changed

2 files changed

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

`‎src/include/access/hash.h‎`

0 commit comments