On Fri, Sep 1, 2023 at 8:38 PM Peter Geoghegan <p...@bowt.ie> wrote:
>
> On Fri, Sep 1, 2023 at 12:34 PM Melanie Plageman
> <melanieplage...@gmail.com> wrote:
> > I don't see why the visibility map shouldn't be updated so that all of
> > the pages show all visible and all frozen for this relation after the
> > vacuum full.
>
> There was a similar issue with COPY FREEZE. It was fixed relatively
> recently -- see commit 7db0cd21.
Thanks for digging that up for me!
My first thought after looking a bit at the vacuum full/cluster code
is that we could add an all_visible flag to the RewriteState and set
it to false in heapam_relation_copy_for_cluster() in roughly the same
cases as heap_page_is_all_visible(), then, if rwstate->all_visible is
true in raw_heap_insert(), when we need to advance to the next block,
we set the page all visible and update the VM. Either way, we reset
all_visible to true since we are advancing to the next block.
I wrote a rough outline of that idea in the attached patches. It
doesn't emit WAL for the VM update or handle toast tables or anything
(it is just a rough sketch), but I just wondered if this was in the
right direction.
- Melanie
From eda40534e169e0bac0d11d89947130b44653b925 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Sun, 3 Sep 2023 16:17:28 -0400
Subject: [PATCH v0 3/3] set all_visible in VM vac full
---
src/backend/access/heap/heapam_handler.c | 15 ++++++++++++++-
src/backend/access/heap/rewriteheap.c | 16 ++++++++++++++++
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 5a17112c91..a37be8cfef 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -791,6 +791,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
HeapTuple tuple;
Buffer buf;
bool isdead;
+ TransactionId xmin;
CHECK_FOR_INTERRUPTS();
@@ -853,10 +854,18 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
break;
case HEAPTUPLE_RECENTLY_DEAD:
*tups_recently_dead += 1;
- /* fall through */
+ isdead = false;
+ rwstate->all_visible = false;
+ break;
case HEAPTUPLE_LIVE:
/* Live or recently dead, must copy it */
+ xmin = HeapTupleHeaderGetXmin(tuple->t_data);
isdead = false;
+ if (!HeapTupleHeaderXminCommitted(tuple->t_data))
+ rwstate->all_visible = false;
+
+ else if (!TransactionIdPrecedes(xmin, OldestXmin))
+ rwstate->all_visible = false;
break;
case HEAPTUPLE_INSERT_IN_PROGRESS:
@@ -873,6 +882,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
elog(WARNING, "concurrent insert in progress within table \"%s\"",
RelationGetRelationName(OldHeap));
/* treat as live */
+ rwstate->all_visible = false;
isdead = false;
break;
case HEAPTUPLE_DELETE_IN_PROGRESS:
@@ -886,6 +896,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
RelationGetRelationName(OldHeap));
/* treat as recently dead */
*tups_recently_dead += 1;
+ rwstate->all_visible = false;
isdead = false;
break;
default:
@@ -906,6 +917,8 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
*tups_vacuumed += 1;
*tups_recently_dead -= 1;
}
+ else
+ rwstate->all_visible = false;
continue;
}
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 93e75dc7c2..b3d63200d4 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -111,6 +111,7 @@
#include "access/transam.h"
#include "access/xact.h"
#include "access/xloginsert.h"
+#include "access/visibilitymap.h"
#include "catalog/catalog.h"
#include "common/file_utils.h"
#include "lib/ilist.h"
@@ -234,6 +235,7 @@ begin_heap_rewrite(Relation old_heap, Relation new_heap, TransactionId oldest_xm
state->rs_freeze_xid = freeze_xid;
state->rs_cutoff_multi = cutoff_multi;
state->rs_cxt = rw_cxt;
+ state->all_visible = true;
/* Initialize hash tables used to track update chains */
hash_ctl.keysize = sizeof(TidHashKey);
@@ -647,6 +649,8 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
* enforce saveFreeSpace unconditionally.
*/
+ if (state->all_visible)
+ PageSetAllVisible(page);
/* XLOG stuff */
if (RelationNeedsWAL(state->rs_new_rel))
log_newpage(&state->rs_new_rel->rd_locator,
@@ -655,6 +659,18 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
page,
true);
+ if (state->all_visible)
+ {
+ Buffer vmbuffer = InvalidBuffer;
+ visibilitymap_pin(state->rs_new_rel, state->rs_blockno, &vmbuffer);
+ visibilitymap_set_unbuffered(state->rs_new_rel, state->rs_blockno, vmbuffer,
+ VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
+ if (BufferIsValid(vmbuffer))
+ ReleaseBuffer(vmbuffer);
+ }
+
+ state->all_visible = true;
+
/*
* Now write the page. We say skipFsync = true because there's no
* need for smgr to schedule an fsync for this write; we'll do it
--
2.37.2
From 9d5f8b8f9ccf759df1bb078b106047c0e15e6f01 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Sun, 3 Sep 2023 16:14:21 -0400
Subject: [PATCH v0 1/3] Extern RewriteStateData
---
src/backend/access/heap/rewriteheap.c | 29 -----------------------
src/include/access/rewriteheap.h | 33 +++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 29 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 424958912c..93e75dc7c2 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -125,35 +125,6 @@
#include "utils/memutils.h"
#include "utils/rel.h"
-/*
- * State associated with a rewrite operation. This is opaque to the user
- * of the rewrite facility.
- */
-typedef struct RewriteStateData
-{
- Relation rs_old_rel; /* source heap */
- Relation rs_new_rel; /* destination heap */
- Page rs_buffer; /* page currently being built */
- BlockNumber rs_blockno; /* block where page will go */
- bool rs_buffer_valid; /* T if any tuples in buffer */
- bool rs_logical_rewrite; /* do we need to do logical rewriting */
- TransactionId rs_oldest_xmin; /* oldest xmin used by caller to determine
- * tuple visibility */
- TransactionId rs_freeze_xid; /* Xid that will be used as freeze cutoff
- * point */
- TransactionId rs_logical_xmin; /* Xid that will be used as cutoff point
- * for logical rewrites */
- MultiXactId rs_cutoff_multi; /* MultiXactId that will be used as cutoff
- * point for multixacts */
- MemoryContext rs_cxt; /* for hash tables and entries and tuples in
- * them */
- XLogRecPtr rs_begin_lsn; /* XLogInsertLsn when starting the rewrite */
- HTAB *rs_unresolved_tups; /* unmatched A tuples */
- HTAB *rs_old_new_tid_map; /* unmatched B tuples */
- HTAB *rs_logical_mappings; /* logical remapping files */
- uint32 rs_num_rewrite_mappings; /* # in memory mappings */
-} RewriteStateData;
-
/*
* The lookup keys for the hash tables are tuple TID and xmin (we must check
* both to avoid false matches from dead tuples). Beware that there is
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 1125457053..d5108ba2d8 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -14,13 +14,46 @@
#define REWRITE_HEAP_H
#include "access/htup.h"
+#include "access/xlogdefs.h"
#include "storage/itemptr.h"
#include "storage/relfilelocator.h"
+#include "storage/bufpage.h"
#include "utils/relcache.h"
+#include "utils/hsearch.h"
/* struct definition is private to rewriteheap.c */
typedef struct RewriteStateData *RewriteState;
+/*
+ * State associated with a rewrite operation.
+ */
+typedef struct RewriteStateData
+{
+ Relation rs_old_rel; /* source heap */
+ Relation rs_new_rel; /* destination heap */
+ Page rs_buffer; /* page currently being built */
+ BlockNumber rs_blockno; /* block where page will go */
+ bool all_visible;
+ bool rs_buffer_valid; /* T if any tuples in buffer */
+ bool rs_logical_rewrite; /* do we need to do logical rewriting */
+ TransactionId rs_oldest_xmin; /* oldest xmin used by caller to determine
+ * tuple visibility */
+ TransactionId rs_freeze_xid; /* Xid that will be used as freeze cutoff
+ * point */
+ TransactionId rs_logical_xmin; /* Xid that will be used as cutoff point
+ * for logical rewrites */
+ MultiXactId rs_cutoff_multi; /* MultiXactId that will be used as cutoff
+ * point for multixacts */
+ MemoryContext rs_cxt; /* for hash tables and entries and tuples in
+ * them */
+ XLogRecPtr rs_begin_lsn; /* XLogInsertLsn when starting the rewrite */
+ HTAB *rs_unresolved_tups; /* unmatched A tuples */
+ HTAB *rs_old_new_tid_map; /* unmatched B tuples */
+ HTAB *rs_logical_mappings; /* logical remapping files */
+ uint32 rs_num_rewrite_mappings; /* # in memory mappings */
+} RewriteStateData;
+
+
extern RewriteState begin_heap_rewrite(Relation old_heap, Relation new_heap,
TransactionId oldest_xmin, TransactionId freeze_xid,
MultiXactId cutoff_multi);
--
2.37.2
From a80f1b3dbd9cc0bf060673fd860e0d59d2ca2159 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Sun, 3 Sep 2023 16:14:56 -0400
Subject: [PATCH v0 2/3] VM update for a heap block not in SB
WIP: needs to emit WAL, do error checking, and handle more conditions
than it currently does
---
src/backend/access/heap/visibilitymap.c | 29 +++++++++++++++++++++++++
src/include/access/visibilitymap.h | 3 +++
2 files changed, 32 insertions(+)
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index 2e18cd88bc..adbc62dd7a 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -315,6 +315,35 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
LockBuffer(vmBuf, BUFFER_LOCK_UNLOCK);
}
+/*
+ * TODO: emit WAL
+ */
+void
+visibilitymap_set_unbuffered(Relation rel, BlockNumber heap_block,
+ Buffer vm_buffer, uint8 flags)
+{
+ uint32 mapByte = HEAPBLK_TO_MAPBYTE(heap_block);
+ uint8 mapOffset = HEAPBLK_TO_OFFSET(heap_block);
+ Page page;
+ uint8 *map;
+
+ page = BufferGetPage(vm_buffer);
+ map = (uint8 *) PageGetContents(page);
+ LockBuffer(vm_buffer, BUFFER_LOCK_EXCLUSIVE);
+
+ if (flags != (map[mapByte] >> mapOffset & VISIBILITYMAP_VALID_BITS))
+ {
+ START_CRIT_SECTION();
+
+ map[mapByte] |= (flags << mapOffset);
+ MarkBufferDirty(vm_buffer);
+
+ END_CRIT_SECTION();
+ }
+
+ LockBuffer(vm_buffer, BUFFER_LOCK_UNLOCK);
+}
+
/*
* visibilitymap_get_status - get status of bits
*
diff --git a/src/include/access/visibilitymap.h b/src/include/access/visibilitymap.h
index daaa01a257..83ca9b3b73 100644
--- a/src/include/access/visibilitymap.h
+++ b/src/include/access/visibilitymap.h
@@ -34,6 +34,9 @@ extern bool visibilitymap_pin_ok(BlockNumber heapBlk, Buffer vmbuf);
extern void visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
XLogRecPtr recptr, Buffer vmBuf, TransactionId cutoff_xid,
uint8 flags);
+
+extern void
+visibilitymap_set_unbuffered(Relation rel, BlockNumber heap_block, Buffer vm_buffer, uint8 flags);
extern uint8 visibilitymap_get_status(Relation rel, BlockNumber heapBlk, Buffer *vmbuf);
extern void visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen);
extern BlockNumber visibilitymap_prepare_truncate(Relation rel,
--
2.37.2
