Re: [RFC PATCH v2 RESEND 04/10] umbra: add patch 3 metadata disk format and identity mapping bootstrap
Hi Álvaro, all, Thanks for the comments and help so far. First, thanks to Tom, Bruce, and Robert for helping me get the submission format into better shape. I will keep using the tar.gz attachment format for future versions. Robert, I agree that the current per-patch subjects are not enough. In v3 I will at least add real commit messages to each patch, so that each patch explains what it adds and why it is split that way. Álvaro, I think your fork-abstraction point is a good question. It made me think more about what the common code should provide and what the owner of relation-local storage should provide. Looking at the patch again, I think part of the problem is that I mixed too many things into the smgr interface. There are lifecycle hooks, runtime mapping calls, WAL/redo calls, background maintenance, and a few statistics counters. That probably makes the code harder to understand than it needs to be. smgrisinternalfork() is one example where the boundary is not clean enough. I am also not sure that the map statistics counters should be in the smgr-facing API. So before trying to solve the much larger owner-defined fork problem, I think I should first clean up the Umbra smgr interface. The smgr may still be the right owner for the lblk-to-pblk metadata, because code above smgr should not know physical block numbers. But the way the prototype exposes that metadata today needs work. I also do not see the current smgr placement only as a shortcut around the fork abstraction problem. The lblk-to-pblk map is part of the physical placement policy of the storage manager, not table-AM or index-AM contents. Even if PostgreSQL eventually has a more general owner-defined fork facility, I think this particular kind of metadata may still naturally be owned by the smgr implementation. One complication is buffering. The metadata fork adds a separate shared cache for MAP pages, outside PostgreSQL's normal buffer manager for relation pages. That follows from making the remapped smgr own the mapping metadata: the MAP cache stores translation metadata, while the normal buffer manager continues to cache relation pages by logical block identity. This is another reason why the owner-defined fork question looks larger than just allowing an AM to declare more forks. The reason Umbra needs this metadata is to make the logical/physical split durable and redoable. For some ordinary updates after checkpoint, PostgreSQL normally needs a full-page image in WAL because redo needs a safe page image to start from. Umbra tries to replace that inline WAL page image, in eligible cases, with a preserved old physical page. In that model, old_pblk is the content baseline and new_pblk is the WAL-owned physical target. WAL records the old physical block, the new physical block, and the resulting mapping state. During redo, the old physical page can be used as the baseline, the WAL delta is applied, and the reconstructed page is written to the new physical location. So the idea is not to disable full_page_writes globally. It is to move the recovery baseline from an inline full-page image in WAL to an old physical page preserved by the remap and reclaim machinery. For v3, the immediate change I will make is to add real commit messages to every patch. I will also make the cover letter point more directly to the relevant design notes, especially the smgr-private metadata boundary and the intended review scope. I will also take the comments about the smgr-facing interfaces being hard to understand into account. Larger changes to the interface shape, patch boundaries, or code structure will need separate follow-up work, and I expect to improve those parts incrementally. I will try to make the next version easier to review. Thanks again for the help so far. I think this direction is worth exploring, and I would be very happy to keep working through the open questions with others and see whether this approach can be made to work. Regards, Mingwei
Re: [RFC PATCH v2 RESEND 04/10] umbra: add patch 3 metadata disk format and identity mapping bootstrap
On 2026-Jun-02, Mingwei Jia wrote: > diff --git a/src/backend/storage/map/map.c b/src/backend/storage/map/map.c > new file mode 100644 > index 00..563f38b21a > --- /dev/null > +++ b/src/backend/storage/map/map.c > @@ -0,0 +1,162 @@ > +/*- > + * > + * map.c > + * Umbra metadata-fork disk layout helpers. > + * > + * This file contains address-translation and in-page access routines for the > + * metadata fork disk layout. > + * > + * src/backend/storage/map/map.c > + * > + *- > + */ I find this pretty difficult to understand, and I think it's because the fork system is the wrong abstraction. The current system is too obviously centered around heapam and nbtree, to the detriment of everything else. I would like a system whereby each (index, table) AM can determine which forks exist and how to deal with each. For instance, BRIN indexes have a "revmap" at the start of the main fork; when it needs one more "revmap" page but it's already used by regular index tuples, it needs to move all those existing index tuples to another page so that the page can be used as a revmap page (I think this is called "evacuate"). That's absurd and overcomplicated. If the revmap had its own fork, a bunch of code and locking considerations would go simply away. I think that would also apply to this map thingy you want to create (although I can't claim to have really understood what you're trying to achieve). Anyway, if I'm pointing in roughly the right direction, then I don't like the idea of this new smgr creating yet another layer to paper over that failed abstraction. Let's fix that instead. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
[RFC PATCH v2 RESEND 04/10] umbra: add patch 3 metadata disk format and identity mapping bootstrap
---
src/backend/catalog/storage.c | 3 +
src/backend/storage/Makefile| 5 +
src/backend/storage/buffer/bufmgr.c | 2 +
src/backend/storage/map/Makefile| 19 ++
src/backend/storage/map/map.c | 162 +
src/backend/storage/map/mapsuper.c | 338
src/backend/storage/map/meson.build | 6 +
src/backend/storage/meson.build | 3 +
src/backend/storage/smgr/smgr.c | 47
src/backend/storage/smgr/umbra.c| 158 -
src/include/storage/map.h | 53 +
src/include/storage/mapsuper.h | 100
src/include/storage/smgr.h | 6 +
src/include/storage/umbra.h | 7 +
14 files changed, 906 insertions(+), 3 deletions(-)
create mode 100644 src/backend/storage/map/Makefile
create mode 100644 src/backend/storage/map/map.c
create mode 100644 src/backend/storage/map/mapsuper.c
create mode 100644 src/backend/storage/map/meson.build
create mode 100644 src/include/storage/map.h
create mode 100644 src/include/storage/mapsuper.h
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index e443a4993c..6b69329a52 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -150,6 +150,8 @@ RelationCreateStorage(RelFileLocator rlocator, char
relpersistence,
srel = smgropen(rlocator, procNumber);
smgrcreate(srel, MAIN_FORKNUM, false);
+ if (needs_wal)
+ smgrcreaterelationmetadata(srel);
if (needs_wal)
log_smgrcreate(&srel->smgr_rlocator.locator, MAIN_FORKNUM);
@@ -1014,6 +1016,7 @@ smgr_redo(XLogReaderState *record)
* log as best we can until the drop is seen.
*/
smgrcreate(reln, MAIN_FORKNUM, true);
+ smgrcreaterelationmetadata(reln);
/*
* Before we perform the truncation, update minimum recovery
point to
diff --git a/src/backend/storage/Makefile b/src/backend/storage/Makefile
index 2afb42ca96..b07ba46dbb 100644
--- a/src/backend/storage/Makefile
+++ b/src/backend/storage/Makefile
@@ -20,4 +20,9 @@ SUBDIRS = \
smgr \
sync
+ifeq ($(with_umbra), yes)
+SUBDIRS += \
+ map
+endif
+
include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/storage/buffer/bufmgr.c
b/src/backend/storage/buffer/bufmgr.c
index 3cc0b0bdd9..540f346d53 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -5505,6 +5505,8 @@ CreateAndCopyRelationData(RelFileLocator src_rlocator,
permanent);
}
}
+
+ smgrcopyrelationmetadata(src_rel, dst_rel, relpersistence);
}
/* -
diff --git a/src/backend/storage/map/Makefile b/src/backend/storage/map/Makefile
new file mode 100644
index 00..ee9603de14
--- /dev/null
+++ b/src/backend/storage/map/Makefile
@@ -0,0 +1,19 @@
+#-
+#
+# Makefile--
+#Makefile for storage/map (Umbra mapping subsystem)
+#
+# IDENTIFICATION
+#src/backend/storage/map/Makefile
+#
+#-
+
+subdir = src/backend/storage/map
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+
+OBJS = \
+ map.o \
+ mapsuper.o
+
+include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/storage/map/map.c b/src/backend/storage/map/map.c
new file mode 100644
index 00..563f38b21a
--- /dev/null
+++ b/src/backend/storage/map/map.c
@@ -0,0 +1,162 @@
+/*-
+ *
+ * map.c
+ * Umbra metadata-fork disk layout helpers.
+ *
+ * This file contains address-translation and in-page access routines for the
+ * metadata fork disk layout.
+ *
+ * src/backend/storage/map/map.c
+ *
+ *-
+ */
+#include "postgres.h"
+
+#include "storage/map.h"
+#include "storage/um_defs.h"
+
+void
+MapPageInit(MapPage *page)
+{
+ Assert(page != NULL);
+
+ MemSet(page->pblknos, 0xFF, sizeof(page->pblknos));
+}
+
+BlockNumber
+MapPageGetEntry(const MapPage *page, int entry_idx)
+{
+ Assert(page != NULL);
+
+ if (entry_idx < 0 || entry_idx >= MAP_ENTRIES_PER_PAGE)
+ elog(ERROR, "map entry index %d is out of range", entry_idx);
+
+ return page->pblknos[entry_idx];
+}
+
+void
+MapPageSetEntry(MapPage *page, int entry_idx, BlockNumber pblkno)
+{
+ Assert(page != NULL);
+
+ if (entry_idx < 0 || entry_idx >= MAP_ENTRIES_PER_PAGE)
+ elog(ERROR, "map entry index %d is out of range", entry_idx);
+
+ page->pblknos[entry_idx] = pblkno;
+}
+
+BlockNumber
+MapForkPageIndexToMapBlkno(ForkNumber forknum,
