On Mon, Jul 10, 2023 at 3:44 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 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.
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.
"standalone" seems the more common spelling of this adjective in the
codebase today.
+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.
You have an extra "the".
I might phrase the last bit as "neither the description functions nor
the output format should be considered part of a stable API"
+Guidelines for rmgrdesc output format
+=====================================
I noticed you used === for both headings and wondered if it was
intentional. Other READMEs I looked at in src/backend/access tend to
have a single heading underlined with ==== and then subsequent
headings are underlined with -----. I could see an argument either way
here, but I just thought I would bring it up in case it was not a
conscious choice.
Otherwise, LGTM.
- Melanie
