[PATCH] remove condition always true (/src/backend/access/heap/vacuumlazy.c)
Hi,
I'm not sure that the patch is 100% correct.
But the fix is about expression about always true.
But if this patch is correct, he fix one possible bug.
The comment says:
* Perform checking of FSM after releasing lock, the fsm is
* approximate, after all.
But this is not what the code does, apparently it checks before unlocking.
best regards,
Ranier Vilela
Attachments:
remove_condition_always_true.patchapplication/octet-stream; name=remove_condition_always_true.patchDownload
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 03c43efc32..ee5fe8efa6 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1093,8 +1093,6 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
if (PageIsNew(page))
{
- bool still_new;
-
/*
* All-zeroes pages can be left over if either a backend extends
* the relation by a single page, but crashes before the newly
@@ -1118,10 +1116,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
* Perform checking of FSM after releasing lock, the fsm is
* approximate, after all.
*/
- still_new = PageIsNew(page);
UnlockReleaseBuffer(buf);
- if (still_new)
+ if (PageIsNew(page))
{
empty_pages++;
Hi,
On 2020-03-30 15:07:40 -0300, Ranier Vilela wrote:
I'm not sure that the patch is 100% correct.
This is *NOT* correct.
But the fix is about expression about always true.
But if this patch is correct, he fix one possible bug.The comment says:
* Perform checking of FSM after releasing lock, the fsm is
* approximate, after all.But this is not what the code does, apparently it checks before unlocking.
No, it doesn't. The freespace check isn't the PageIsNew(), it's the
GetRecordedFreeSpace() call. Which happens after unlocking.
Greetings,
Andres Freund
Em seg., 30 de mar. de 2020 às 18:14, Andres Freund <andres@anarazel.de>
escreveu:
Hi,
On 2020-03-30 14:10:29 -0700, Andres Freund wrote:
On 2020-03-30 17:08:01 -0300, Ranier Vilela wrote:
Em seg., 30 de mar. de 2020 às 16:05, Andres Freund <
andres@anarazel.de>
escreveu:
Hi,
On 2020-03-30 15:07:40 -0300, Ranier Vilela wrote:
I'm not sure that the patch is 100% correct.
This is *NOT* correct.
Anyway, the original source, still wrong.
What is the use of testing PageIsNew (page) twice in a row, if nothinghas
changed.
Yea, that can be reduced. It's pretty harmless though.
We used to require a cleanup lock (which requires dropping the lock,
acquiring a cleanup lock - which allows for others to make the page be
not empty) before acting on the empty page in vacuum. That's why
PageIsNew() had to be checked again.
Well, this is what the patch does, promove reduced and continue to check
PageIsNew after unlock.
regards,
Ranier Vilela
Import Notes
Reply to msg id not found: 20200330211440.53r73hsreoj3m6yg@alap3.anarazel.de