On 12/04/2023 01:29, Peter Geoghegan wrote:
Thanks for your help with the follow-up work. Seems like we're done
with this now.
This is still listed in the July commitfest; is there some work remaining?
I'm late to the party, but regarding commit c03c2eae0a, which added the
guidelines for writing formatting desc functions:
You moved the comment from rmgrdesc_utils.c into rmgrdesc_utils.h, but I
don't think that was a good idea. Our usual convention is to have the
function comment in the .c file, not at the declaration in the header
file. When I want to know what a function does, I jump to the .c file,
and might miss the comment in the header entirely.
Let's add a src/backend/access/rmgrdesc/README file. We don't currently
have any explanation anywhere why the rmgr desc functions are in a
separate directory. The README would be a good place to explain that,
and to have the formatting guidelines. See attached.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 5261e3b4c373aef47458bed14d339d4ea2e6df5a Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 10 Jul 2023 10:41:35 +0300
Subject: [PATCH v1 1/1] Add rmgrdesc README.
In the README, briefly explain what rmgrdesc functions are, and why they
are in a separate directory. Commit c03c2eae0a added some guidelines on
the preferred output format; move that to the README too.
Discussion: TODO
---
src/backend/access/rmgrdesc/README | 60 ++++++++++++++++++++
src/backend/access/rmgrdesc/rmgrdesc_utils.c | 4 ++
src/include/access/rmgrdesc_utils.h | 44 --------------
3 files changed, 64 insertions(+), 44 deletions(-)
create mode 100644 src/backend/access/rmgrdesc/README
diff --git a/src/backend/access/rmgrdesc/README b/src/backend/access/rmgrdesc/README
new file mode 100644
index 0000000000..abe84b9f11
--- /dev/null
+++ b/src/backend/access/rmgrdesc/README
@@ -0,0 +1,60 @@
+src/backend/access/rmgrdesc/README
+
+WAL resource manager description functions
+==========================================
+
+For debugging purposes, there is a "description function", or rmgrdesc
+function, for each WAL resource manager. The rmgrdesc function parses the WAL
+record and prints the contents of the WAL record in a somewhat human-readable
+format.
+
+The rmgrdesc functions for all resource managers are gathered in this
+directory, because they are also used in the stand-alone pg_waldump program.
+They could potentially be used by out-of-tree debugging tools too, although
+the the functions or the output format should not be considered a stable API.
+
+Guidelines for rmgrdesc output format
+=====================================
+
+The goal of these guidelines is to avoid gratuitous inconsistencies across
+each rmgr, and to allow users to parse desc output strings without too much
+difficulty. This is not an API specification or an interchange format.
+(Only heapam and nbtree desc routines follow these guidelines at present, in
+any case.)
+
+Record descriptions are similar to JSON style key/value objects. However,
+there is no explicit "string" type/string escaping. Top-level { } brackets
+should be omitted. For example:
+
+snapshotConflictHorizon: 0, flags: 0x03
+
+Record descriptions may contain variable-length arrays. For example:
+
+nunused: 5, unused: [1, 2, 3, 4, 5]
+
+Nested objects are supported via { } brackets. They generally appear inside
+variable-length arrays. For example:
+
+ndeleted: 0, nupdated: 1, deleted: [], updated: [{ off: 45, nptids: 1, ptids: [0] }]
+
+Try to output things in an order that faithfully represents the order of
+fields from the underlying physical WAL record struct. Key names should be
+unique (at the same nesting level) to make parsing easy. It's a good idea if
+the number of items in the array appears before the array.
+
+It's okay for individual WAL record types to invent their own conventions.
+For example, Heap2's PRUNE record descriptions use a custom array format for
+the record's "redirected" field:
+
+... redirected: [1->4, 5->9], dead: [10, 11], unused: [3, 7, 8]
+
+Arguably the desc routine should be using object notation for this instead.
+However, there is value in using a custom format when it conveys useful
+information about the underlying physical data structures.
+
+This ad-hoc format has the advantage of being close to the format used for
+the "dead" and "unused" arrays (which follow the standard desc convention for
+page offset number arrays). It suggests that the "redirected" elements shown
+are just pairs of page offset numbers (which is how it really works).
+
+rmgrdesc_utils.c contains some helper functions to print data in this format.
diff --git a/src/backend/access/rmgrdesc/rmgrdesc_utils.c b/src/backend/access/rmgrdesc/rmgrdesc_utils.c
index 90051f0df9..808770524d 100644
--- a/src/backend/access/rmgrdesc/rmgrdesc_utils.c
+++ b/src/backend/access/rmgrdesc/rmgrdesc_utils.c
@@ -16,6 +16,10 @@
#include "access/rmgrdesc_utils.h"
#include "storage/off.h"
+/*
+ * Helper function to print an array, in the format described in the
+ * README.
+ */
void
array_desc(StringInfo buf, void *array, size_t elem_size, int count,
void (*elem_desc) (StringInfo buf, void *elem, void *data),
diff --git a/src/include/access/rmgrdesc_utils.h b/src/include/access/rmgrdesc_utils.h
index bd414699f2..c754d55621 100644
--- a/src/include/access/rmgrdesc_utils.h
+++ b/src/include/access/rmgrdesc_utils.h
@@ -12,50 +12,6 @@
#ifndef RMGRDESC_UTILS_H_
#define RMGRDESC_UTILS_H_
-/*
- * Guidelines for rmgrdesc routine authors:
- *
- * The goal of these guidelines is to avoid gratuitous inconsistencies across
- * each rmgr, and to allow users to parse desc output strings without too much
- * difficulty. This is not an API specification or an interchange format.
- * (Only heapam and nbtree desc routines follow these guidelines at present,
- * in any case.)
- *
- * Record descriptions are similar to JSON style key/value objects. However,
- * there is no explicit "string" type/string escaping. Top-level { } brackets
- * should be omitted. For example:
- *
- * snapshotConflictHorizon: 0, flags: 0x03
- *
- * Record descriptions may contain variable-length arrays. For example:
- *
- * nunused: 5, unused: [1, 2, 3, 4, 5]
- *
- * Nested objects are supported via { } brackets. They generally appear
- * inside variable-length arrays. For example:
- *
- * ndeleted: 0, nupdated: 1, deleted: [], updated: [{ off: 45, nptids: 1, ptids: [0] }]
- *
- * Try to output things in an order that faithfully represents the order of
- * fields from the underlying physical WAL record struct. Key names should be
- * unique (at the same nesting level) to make parsing easy. It's a good idea
- * if the number of items in the array appears before the array.
- *
- * It's okay for individual WAL record types to invent their own conventions.
- * For example, Heap2's PRUNE record descriptions use a custom array format
- * for the record's "redirected" field:
- *
- * ... redirected: [1->4, 5->9], dead: [10, 11], unused: [3, 7, 8]
- *
- * Arguably the desc routine should be using object notation for this instead.
- * However, there is value in using a custom format when it conveys useful
- * information about the underlying physical data structures.
- *
- * This ad-hoc format has the advantage of being close to the format used for
- * the "dead" and "unused" arrays (which follow the standard desc convention
- * for page offset number arrays). It suggests that the "redirected" elements
- * shown are just pairs of page offset numbers (which is how it really works).
- */
extern void array_desc(StringInfo buf, void *array, size_t elem_size, int count,
void (*elem_desc) (StringInfo buf, void *elem, void *data),
void *data);
--
2.30.2
