Subject: [PATCH] pg_bsd_indent: improve formatting of multiline comments

From: Aleksander Alekseev <[email-masked]>
Date: Thu, 19 Jun 2025 17:51:36 +0300

Hi,

Michael (cc:'ed) pointed out [1] that pg_bsd_indent could do a
slightly better job when it comes to formatting multiline comments. I
prepared a patch that fixes this.

How to test it:

```
ninja -C build
cp build/src/tools/pg_bsd_indent/pg_bsd_indent ~/bin/pg_bsd_indent
cp src/tools/pgindent/pgindent ~/bin/pgindent
pgindent src/backend/utils/adt/varlena.c
git diff
```

Expected changes:

```
-/* name_text()
+/*
+ * name_text()
  * Converts a Name type to a text type.
  */
```

Everything else should work as before.

Thoughts and feedback are most welcomed.

[1]: https://postgr.es/m/CAJ7c6TN7ppSmZMPejvKZreOs%3D%2BkJEhrGQNuVpmTjj9W-%3DMjgCg%40mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

---

From: Arseniy Mukhin <[email-masked]>
Date: Thu, 19 Jun 2025 23:33:55 +0300

> [1]: https://postgr.es/m/CAJ7c6TN7ppSmZMPejvKZreOs%3D%2BkJEhrGQNuVpmTjj9W-%3DMjgCg%40mail.gmail.com

Hi Aleksander!

Thank you for the patch!

I tried it with the whole project and almost always it works great.
But I noticed two cases where it works probably not as expected:

1) comments which don't have a star on each line. For example:
file 'cube.c'

before:
/* make up a metric in which one box will be 'lower' than the other
   -- this can be useful for sorting and to determine uniqueness */

after:
/*
 * make up a metric in which one box will be 'lower' than the other
   -- this can be useful for sorting and to determine uniqueness */

2) comments where closing */ is on the last comment line. For example:
file 'crypt-blowfish.c'

before:
/* This has to be bug-compatible with the original implementation, so
 * only encode 23 of the 24 bytes. :-) */

after:
/*
 * This has to be bug-compatible with the original implementation, so
 * only encode 23 of the 24 bytes. :-) */

Best regards,
Arseniy Mukhin

---

From: Aleksander Alekseev <[email-masked]>
Date: Fri, 20 Jun 2025 16:44:08 +0300

Hi Arseniy,

>  * only encode 23 of the 24 bytes. :-) */

Thanks for the review. You are right, these comments shouldn't be affected.

It's going to be simpler to modify pgindent then. PFA the updated patch.

-- 
Best regards,
Aleksander Alekseev

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 20 Jun 2025 09:56:41 -0400

> It's going to be simpler to modify pgindent then. PFA the updated patch.

Given the quality of BSD indent code, I have _always_ found it easier to
modify pgindent.  ;-

-- 
  Bruce Momjian  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

---

From: Aleksander Alekseev <[email-masked]>
Date: Fri, 20 Jun 2025 17:01:41 +0300

Hi,

> Given the quality of BSD indent code, I have _always_ found it easier to

:D Initially I thought that the problem was simple enough to solve it
in C, but this turned out not to be true.

> It's going to be simpler to modify pgindent then. PFA the updated patch.

I noticed a mistake in v2. Here is the corrected patch. Changes
comparing to the previous version:

```
-    $lines[-1] =~ s!/(.+)\*/!/$1\n \*/!;
+    $lines[-1] =~ s!(.+) \*/!$1\n \*/!;
```

-- 
Best regards,
Aleksander Alekseev

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 20 Jun 2025 10:14:17 -0400

> in C, but this turned out not to be true.

I always found it ironic that a tool designed to make source code easier
to understand like BSD indent is so hard to understand.  Its use of
short variable names alone is confusing.

---

From: Arseniy Mukhin <[email-masked]>
Date: Sat, 21 Jun 2025 21:54:21 +0300

> comparing to the previous version:

Thanks!

Now it affects 4 times more files (380). What I noticed:
1) Most of the comments are bordered comments like this:
-/* --------------------------------------------------------------------------------
+/*
+ * --------------------------------------------------------------------------------
  * Public IO related functions operating on IO Handles
  * --------------------------------------------------------------------------------
  */

Do we want to skip such comments?
I have also seen comments with '====' border.

2) Some comments like this:

before:
/* Author: Linus Tolke
   (actually most if the code is "borrowed" from the distribution and just
   slightly modified)
 */

after:
/*
 * Author: Linus Tolke
   (actually most if the code is "borrowed" from the distribution and just
   slightly modified)
 */

I guess closing */ on the separate line is the trigger?
If I'm not wrong there are only 3 such comments, maybe it is easier to
fix them by hand?)

3) It seems all geqo related file contains such comment:
-/* contributed by:
+/*
+ * contributed by:
    =*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=
    *  Martin Utesch                             * Institute of
Automatic Control          *
    =

Best regards,
Arseniy Mukhin

---

From: Aleksander Alekseev <[email-masked]>
Date: Mon, 23 Jun 2025 13:42:32 +0300

Hi Arseniy,

Thanks for the feedback!

> 1) Most of the comments are bordered comments like this:

Personally I don't have a strong opinion on this. We can easily add an
exception for "/* ---" and "/* ===" comments if somebody believes this
is a problem. I choose not to add such an exception just yet only
because I don't like unnecessary exceptions :)

> 3) It seems all geqo related file contains such comment:

You are right, these comments shouldn't have been changed. Apparently
the script is going to need slightly more complicated checks that I
initially thought.

Here is the corrected patch v4.

-- 
Best regards,
Aleksander Alekseev

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 23 Oct 2025 15:25:15 -0500

> because I don't like unnecessary exceptions :)

+1 for adding an exception for "/* -----" style comments.  I tested running
pgindent after applying the patch, and the first thing I noticed was all
these (IMHO) unnecessary changes.  I don't think it helps readability, and
even if it did, it's arguably not worth the churn.

-- 
nathan

---

From: Aleksander Alekseev <[email-masked]>
Date: Mon, 27 Oct 2025 15:13:33 +0300

Hi Nathan,

> even if it did, it's arguably not worth the churn.

OK, here is the corrected patch v5.

-- 
Best regards,
Aleksander Alekseev
From f87137673c0b3da5aed296f88016e8d8edfd3915 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email-masked]>
Date: Fri, 20 Jun 2025 16:31:36 +0300
Subject: [PATCH v5] pgindent: improve formatting of multiline comments

Format multiline comments like this:

/* line 1
 * line 2
 */

... into:

/*
 * line 1
 * line 2
 */

This is more consistent with what we currently have in the tree.

Author: Aleksander Alekseev <[email-masked]>
Reported-by: Michael Paquier <[email-masked]>
Reviewed-by: Arseniy Mukhin <[email-masked]>
Reviewed-by: Nathan Bossart <[email-masked]>
Discussion: https://postgr.es/m/CAJ7c6TPQ0kkHQG-AqeAJ3PV_YtmDzcc7s%2B_V4%3Dt%2BxgSnZm1cFw%40mail.gmail.com
---
 src/tools/pgindent/pgindent | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index b7d71808924..670e204e917 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -281,6 +281,9 @@ sub post_indent
 	# Fix run-together comments to have a tab between them
 	$source =~ s!\*/(/\*.*\*/)$!*/\t$1!gm;
 
+	# Postprocess multiline comments except for /**... and /*-... ones
+	$source =~ s!^(/\*[^\*\-].*?\*/)!postprocess_multiline_comment($1)!mgse;
+
 	## Functions
 
 	# Use a single space before '*' in function return types
@@ -289,6 +292,39 @@ sub post_indent
 	return $source;
 }
 
+sub postprocess_multiline_comment
+{
+    my $source = shift;
+    my @lines = split "\n", $source;
+
+    # Only format comments that match the expected format,
+    # or at least that could have been the author's intent.
+    if (($lines[0] ne "/*" && $lines[-1] ne " */") or ($lines[1] !~ m!^ \*!))
+    {
+        return $source;
+    }
+
+    # Check each line except for the fist and the last one
+    for my $i ( 1 .. scalar @lines - 2 )
+    {
+        $lines[$i] = " *".$lines[$i] if $lines[$i] !~ /^ \*/;
+    }
+
+    # Keep /* === and /* --- lines as is
+    if ($lines[0] !~ m!^/\* [=-]+!) {
+        $lines[0] =~ s!/\*(.+)!/\*\n *$1!;
+    }
+
+    # Keep === */ and --- */ lines as is
+    if ($lines[-1] !~ m![=-]+ \*/$!) {
+        $lines[-1] =~ s!(.+) \*/!$1\n \*/!;
+    }
+
+    $source = join "\n", @lines;
+
+    return $source;
+}
+
 sub run_indent
 {
 	my $source = shift;
-- 
2.43.0

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 27 Oct 2025 09:55:38 -0500

> OK, here is the corrected patch v5.

Thanks.  I had to run pgindent twice before it stopped making changes, but
the results look pretty good to me.  However, I still noticed a few
oddities.

--- a/contrib/seg/seg.c
+++ b/contrib/seg/seg.c
@@ -2,9 +2,9 @@
  * contrib/seg/seg.c
  *
  ******************************************************************************
-  This file contains routines that can be bound to a Postgres backend and
-  called by the backend in the process of processing queries.  The calling
-  format for these routines is dictated by Postgres architecture.
+ *  This file contains routines that can be bound to a Postgres backend and
+ *  called by the backend in the process of processing queries.  The calling
+ *  format for these routines is dictated by Postgres architecture.
 ******************************************************************************/

This one is a little weird, but maybe it's okay...  It looks like pgindent
retains the spacing (or lack of spacing) in this situation.  It's probably
not worth the complexity to try to make it smarter here.

--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -69,7 +69,7 @@
 /*
  * DISABLE_LEADER_PARTICIPATION disables the leader's participation in
  * parallel index builds.  This may be useful as a debugging aid.
-#undef DISABLE_LEADER_PARTICIPATION
+ *#undef DISABLE_LEADER_PARTICIPATION
  */

IMHO we should either remove this line or move it out of the comment.
AFAICT we ordinarily don't #undef debugging stuff like this, presumably so
you can change it with compile flags.

In aclcheck.c and analyze.c, The **** and ==== lines are still getting
bumped down to a new line.

-.* This function just encapsulates the framing rules.
+ *.* This function just encapsulates the framing rules.

This looks like a goof in commit 25a30bb.  I'll go fix that one separately.

--- a/src/backend/storage/ipc/waiteventset.c
+++ b/src/backend/storage/ipc/waiteventset.c
@@ -2010,7 +2010,7 @@ ResOwnerReleaseWaitEventSet(Datum res)
  * NB: be sure to save and restore errno around it.  (That's standard practice
  * in most signal handlers, of course, but we used to omit it in handlers that
  * only set a flag.) XXX
-  *
+ *  *

This one looks like a goof in commit 393e0d2.  It's interesting that
pgindent chooses to add an extra * here, though.

-- 
nathan

---

From: Michael Paquier <[email-masked]>
Date: Tue, 28 Oct 2025 08:35:15 +0900

> you can change it with compile flags.

I would put this one on a separate line, outside the comment.  It's
minor, still we use this style in pg_config_manual.h.  See around
REALLOCATE_BITMAPSETS.
--
Michael

---

From: Chao Li <[email-masked]>
Date: Tue, 28 Oct 2025 08:36:33 +0800

> On Oct 28, 2025, at 07:35, Michael Paquier <[email-masked]> wrote:
> 

+1

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Chao Li <[email-masked]>
Date: Tue, 28 Oct 2025 08:57:36 +0800

> On Oct 27, 2025, at 20:13, Aleksander Alekseev <[email-masked]> wrote:
> 
> Hi Nathan,
> 
> <v5-0001-pgindent-improve-formatting-of-multiline-comments.patch>

1. I just ran the patched pgindent against a random file, then I got a lot diffs like:

```
        /*
-        * Direct advancement: avoid waking non-caught up backends that
-        * aren't interested in our notifications.
+        * Direct advancement: avoid waking non-caught up backends that aren't
+        * interested in our notifications.
         */
```

I am afraid that would generate a lot of noises for future reviews.

2. A typo in the patch

```

fist => first

3. As you are updating pgindent, I want to report an issue, you may address in a separate patch or just in this patch, up to you.

See this code:
```
            else
                 /*
                  * fetch all the rest of the page
                  */
                 copysize = QUEUE_PAGESIZE - curoffset;
```

In the “else” clause, there is a multiple-line comment block, and a single line of code. Pgindent will add an empty line between “else” and the comment block, which is weird. If the comment is one-line, then no empty line will be inserted.

Best regards,

---

From: Aleksander Alekseev <[email-masked]>
Date: Mon, 10 Nov 2025 12:28:06 +0300

Hi,

> +    # Check each line except for the fist and the last one

Thanks, fixed.

> In the “else” clause, there is a multiple-line comment block, and a single line of code. Pgindent will add an empty line between “else” and the comment block, which is weird. If the comment is one-…

I didn't manage to find this code. The closest thing I see is in
src/backend/commands/async.c:

```
            else
            {
                /* fetch all the rest of the page */
                copysize = QUEUE_PAGESIZE - curoffset;
            }
```

Last time it was touched 15 years ago and pgindent processes it as expected.

> +        * interested in our notifications.

I'm not sure if this is part of the PostgreSQL code base either. My
best guess is that something is wrong with whitespaces here (tabs vs
spaces). We have plenty of multiline comments like this and from what
I can tell they are processed correctly.

-- 
Best regards,
Aleksander Alekseev
From ad91fda8a7406f9fd5dbbfe5a895a811230e2eec Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email-masked]>
Date: Fri, 20 Jun 2025 16:31:36 +0300
Subject: [PATCH v6] pgindent: improve formatting of multiline comments

/* line 1
 * line 2
 */

... into:

/*
 * line 1
 * line 2
 */

index b7d71808924..4db12cb1d92 100755

 
+    # Check each line except for the first and the last one

---

From: Chao Li <[email-masked]>
Date: Tue, 11 Nov 2025 14:28:43 +0800

> On Nov 10, 2025, at 17:28, Aleksander Alekseev <[email-masked]> wrote:
> 

I recalled that. I ran pgindent on a file from a patch I was reviewing, so, the file was not pgindent-ed yet at the time.

I just tried to run the patched pgindent on several files and no diff generated. So this comment is resolved.

Regards,

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 11 Dec 2025 15:43:31 -0600

>> minor, still we use this style in pg_config_manual.h.  See around

Here's a patch for this.  I also changed the #undefs to #defines.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 11 Dec 2025 15:48:27 -0600

With the latest patch applied, I'm still seeing problems with **** and ====
lines in aclcheck.c and analyze.c, as mentioned upthread [0].  I think we
should also figure out what we want to do for things like this:

Could we modify the patch to leave these kinds of comments alone?

[0] https://postgr.es/m/aP-H6kSsGOxaB21k%40nathan

-- 
nathan

---

From: Aleksander Alekseev <[email-masked]>
Date: Fri, 12 Dec 2025 14:32:13 +0300

Hi Nathan,

> [...]
> I think we should also figure out what we want to do for things like this:
>

I see 3 files that have this comment:

- src/tutorial/complex.c
- contrib/seg/seg.c
- contrib/cube/cube.c

Note that cube.c is not affected by the new version of pgindent,
because the comment starts with `/****`. Such comments are kept as is.

seg.c and cube.c use a mixed format. The comments start with `/*`
followed by ` *` lines but several lines below don't have ` *` in the
beginning. I don't instantly see a way to process this situation in
pgindent. We can either let it modify these comments, or alternatively
change the first lines of the comments from `/*` to `/**`.

-- 
Best regards,
Aleksander Alekseev

---

From: Álvaro Herrera <[email-masked]>
Date: Fri, 12 Dec 2025 16:53:39 +0100

It's strange to see this thread go on about messing with the Perl script
because we're too afraid of pg_bsd_indent.  Maybe we should take
ownership of that code -- improve its own indentation to our
conventions, add comments, rename unclear variables, and so on until we
have something we can work with, and fix these weird bugs and
idiosincratic behaviors we don't like.  I just found out this code dates
back from 1976.

We have a _huge_ regression test for it: our own source code repository.
It's not like any breakage is going to go unnoticed.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Cómo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar
al alfarero y ver qué formas se pueden sacar del otro" (C. Halloway en
La Feria de las Tinieblas, R. Bradbury)

---

From: Tom Lane <[email-masked]>
Date: Fri, 12 Dec 2025 11:17:01 -0500

> idiosincratic behaviors we don't like.  I just found out this code dates

I've worked with that code a little bit, and it's mostly unreadable
spaghetti :-(.  If somebody wants to make an effort to make it not
so awful, that'd be great, but I fear it's a very nontrivial project.

Having said that, we've already basically forked it from the NetBSD
upstream, so there is no reason not to diverge further.

> It's not like any breakage is going to go unnoticed.

True.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 12 Dec 2025 10:23:14 -0600

> so awful, that'd be great, but I fear it's a very nontrivial project.

Yeah, I tried to find a bug in it recently and could do little more than
trial-and-error in the areas that seemed vaguely close (with no success).
I'm generally critical of efforts to rewrite things from scratch, but this
might be a case where it's the better option.

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Fri, 12 Dec 2025 13:37:23 -0500

> might be a case where it's the better option.

There's something to be said for that approach.  Presumably, a rewrite
could be smaller because we wouldn't need to support all the options
bsd_indent has, just the behavior PG wants.

I wonder whether it'd be possible to get rid of the need for
typedefs.list while at it.  That might be impractical --- IIRC, C
syntax is ambiguous if you don't know which identifiers are typedefs.
Still, I believe there are other indenters that get away without
that knowledge, so maybe the ambiguity isn't fatal for indentation
purposes.

With or without that nice-to-have, it'd be a lot of work with
not all that much payoff, so I'm finding it hard to recommend
that somebody go after this.  But maybe someone will find it
irresistible to scratch that itch.

			regards, tom lane

---

From: Aleksander Alekseev <[email-masked]>
Date: Sat, 13 Dec 2025 00:47:30 +0300

Hi,

> irresistible to scratch that itch.

I was thinking just recently that rewriting pgindent in Python might
be a good idea for future refactoring, but probably only if and after
we merge PyTest patchset [1]. (Which strangely enough is missing on
the nearest open commitfest.)

[1]: https://www.postgresql.org/message-id/flat/CAOYmi%2BkThkM9Z87u%3DR_Wi7fCor2i%2BUZKAyq0UCyprzCwTQvqgA%40mail.gmail.com
--
Best regards,
Aleksander Alekseev

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 19 Dec 2025 15:02:12 -0500

> so awful, that'd be great, but I fear it's a very nontrivial project.

Yes, this is why we had to wrap the pg_bsd_indent call in Perl.

---

From: Michael Paquier <[email-masked]>
Date: Mon, 22 Dec 2025 07:33:15 +0900

> Yes, this is why we had to wrap the pg_bsd_indent call in Perl.

Removing one layer, finishing with only a C function or a perl script
would be really nice.  I am wondering if removing the perl script
would not be the way to go, just owning the portion of the code we
care about, which is already in the tree anyway.

From what I can see, we rely on a set of options through $indent_opts,
it seems like one would enter a fail-and-retry repeat loop until
things get rather right: 
- Remove as much code from pg_bsd_indent as possible.
- Integrate some of the options in the perl script into the binary.

If the binary gets slim enough and more understandable, perhaps we
could even keep both, we have all the pgtypedef business which is
specific to the Postgres tree as well.
--
Michael