diff options
| author | Peter Geoghegan <pg@bowt.ie> | 2023-01-16 09:34:37 -0800 |
|---|---|---|
| committer | Peter Geoghegan <pg@bowt.ie> | 2023-01-16 09:34:37 -0800 |
| commit | 980ae173108e353045e5ab4a842bb21e9dfe6715 (patch) | |
| tree | ef5442d9702a03eb60a52c7a009f6153410f8491 /src/backend/access/heap/visibilitymap.c | |
| parent | 6fa66ec88ff29f5449d89e9891a00fe64afae34e (diff) | |
| download | postgresql-980ae173108e353045e5ab4a842bb21e9dfe6715.tar.gz postgresql-980ae173108e353045e5ab4a842bb21e9dfe6715.zip | |
Tighten up VACUUM's approach to setting VM bits.
Tighten up the way that visibilitymap_set() is called: request that both
the all-visible and all-frozen bits get set whenever the all-frozen bit
is set, regardless of what we think we know about the present state of
the all-visible bit. Also make sure that the page level PD_ALL_VISIBLE
flag is set in the same code path.
In practice there doesn't seem to be a concrete scenario in which the
previous approach could lead to inconsistencies. It was almost possible
in scenarios involving concurrent HOT updates from transactions that
abort, but (unlike pruning) freezing can never remove XIDs > VACUUM's
OldestXmin, even those from transactions that are known to have aborted.
That was protective here.
These issues have been around since commit a892234f83, which added the
all-frozen bit to the VM fork. There is no known live bug here, so no
backpatch.
In passing, add some defensive assertions to catch the issue, and stop
reading the existing state of the VM when setting the VM in VACUUM's
final heap pass. We already know that affected pages must have had at
least one LP_DEAD item before we set it LP_UNUSED, so there is no point
in reading the VM when it is set like this.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CAH2-WznuNGSzF8v6OsgjaC5aYsb3cZ6HW6MLm30X0d65cmSH6A@mail.gmail.com
Diffstat (limited to 'src/backend/access/heap/visibilitymap.c')
| -rw-r--r-- | src/backend/access/heap/visibilitymap.c | 9 |
1 files changed, 6 insertions, 3 deletions
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c index 1d1ca423a98..74ff01bb172 100644 --- a/src/backend/access/heap/visibilitymap.c +++ b/src/backend/access/heap/visibilitymap.c @@ -146,7 +146,9 @@ visibilitymap_clear(Relation rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags char *map; bool cleared = false; + /* Must never clear all_visible bit while leaving all_frozen bit set */ Assert(flags & VISIBILITYMAP_VALID_BITS); + Assert(flags != VISIBILITYMAP_ALL_VISIBLE); #ifdef TRACE_VISIBILITYMAP elog(DEBUG1, "vm_clear %s %d", RelationGetRelationName(rel), heapBlk); @@ -256,8 +258,11 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, #endif Assert(InRecovery || XLogRecPtrIsInvalid(recptr)); - Assert(InRecovery || BufferIsValid(heapBuf)); + Assert(InRecovery || PageIsAllVisible((Page) BufferGetPage(heapBuf))); + + /* Must never set all_frozen bit without also setting all_visible bit */ Assert(flags & VISIBILITYMAP_VALID_BITS); + Assert(flags != VISIBILITYMAP_ALL_FROZEN); /* Check that we have the right heap page pinned, if present */ if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk) @@ -299,8 +304,6 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, { Page heapPage = BufferGetPage(heapBuf); - /* caller is expected to set PD_ALL_VISIBLE first */ - Assert(PageIsAllVisible(heapPage)); PageSetLSN(heapPage, recptr); } } |