Subject: lwlocknames.h beautification attempt

From: Gurjeet Singh <[email-masked]>
Date: Sat, 1 Mar 2025 22:09:47 -0800

Currently the contents of lwlocknames.h look like this:

#define ShmemIndexLock (&MainLWLockArray[1].lock)
#define OidGenLock (&MainLWLockArray[2].lock)
#define XidGenLock (&MainLWLockArray[3].lock)
#define ProcArrayLock (&MainLWLockArray[4].lock)
#define SInvalReadLock (&MainLWLockArray[5].lock)
...

This makes it a bit hard to read, since there is no attempt at vertical
alignment along the opening parentheses. It gets worse if the editor
performs syntax highlighting, and the various colored characters in the
definitions appear haphazardly arranged.

Had this been hand-written, I can imagine the author making an attempt at
aligning the definitions, but since this is a generated file, the lack of
that attempt is understandable.

I propose the following change to the generation script,
generate-lwlocknames.pl

-    print $h "#define ${lockname}Lock (&MainLWLockArray[$lockidx].lock)\n";
+    printf $h "#define %-30s %s\n", "${lockname}Lock",
"(&MainLWLockArray[$lockidx].lock)";

which produces the lock names in this format

#define ShmemIndexLock                 (&MainLWLockArray[1].lock)
#define OidGenLock                     (&MainLWLockArray[2].lock)
#define XidGenLock                     (&MainLWLockArray[3].lock)
#define ProcArrayLock                  (&MainLWLockArray[4].lock)
#define SInvalReadLock                 (&MainLWLockArray[5].lock)
...

Yet another format, which I prefer, can be achieved by right-aligning the
lock names. In addition to aligning the definitions, it also aligns the
'Lock' suffix in all the names. But as they say beauty is in the eye of the
beholder, so this one may be actively disliked by others.

-    print $h "#define ${lockname}Lock (&MainLWLockArray[$lockidx].lock)\n";
+    printf $h "#define %30s %s\n", "${lockname}Lock",
"(&MainLWLockArray[$lockidx].lock)";

#define                 ShmemIndexLock (&MainLWLockArray[1].lock)
#define                     OidGenLock (&MainLWLockArray[2].lock)
#define                     XidGenLock (&MainLWLockArray[3].lock)
#define                  ProcArrayLock (&MainLWLockArray[4].lock)
#define                 SInvalReadLock (&MainLWLockArray[5].lock)
...

The lockname column needs to be at least 30 chars wide to accommodate the
longest of the lock names 'DynamicSharedMemoryControlLock'.

Best regards,
Gurjeet
http://Gurje.et

---

From: Álvaro Herrera <[email-masked]>
Date: Fri, 14 Mar 2025 20:38:18 +0100

> "(&MainLWLockArray[$lockidx].lock)";

I forgot to send a note here that I pushed this patch.  Thank you.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
[…] indem ich in meinem Leben oft an euch gedacht, euch glücklich zu machen. Seyd es!
A menudo he pensado en vosotros, en haceros felices. ¡Sedlo, pues!
		Heiligenstädter Testament, L. v. Beethoven, 1802
		https://de.wikisource.org/wiki/Heiligenstädter_Testament

---

From: Robert Haas <[email-masked]>
Date: Sun, 16 Mar 2025 22:53:25 -0400

> I forgot to send a note here that I pushed this patch.  Thank you.

I'm confused. Tom and I both said we didn't like this change, so you
committed the patch without further discussion?

I mean, this is a pretty unimportant detail, so I don't really want to
fight about it too much, but that really doesn't seem like a consensus
to me.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Álvaro Herrera <[email-masked]>
Date: Mon, 17 Mar 2025 07:43:41 +0100

> committed the patch without further discussion?

Tom didn't say he didn't like this change.  He said he didn't like a
different change, which is not the one I committed.  And your opinion
was quite thin on arguments, and you didn't reply for 11 days after I
expressed intention to apply a simplified version of Gurjeet's patch.

> fight about it too much, but that really doesn't seem like a consensus

Would you have objected if I had proposed to use that style to begin with?
You can see that the thread where this was discussed [1] was pretty
light on coding style/output style discussion.  It seems hard to argue
that the original had achieved any kind of consensus.

[1] https://postgr.es/m/[email-masked]

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

---

From: Gurjeet Singh <[email-masked]>
Date: Sun, 16 Mar 2025 23:44:07 -0700

(resending the email because it was held for moderation; replaced image
attachment with a link, which might be the reason for being put in the
moderation queue)

On Sun, Mar 16, 2025 at 7:53 PM Robert Haas <[email-masked]> wrote:
> I'm confused. Tom and I both said we didn't like this change,

To me, Tom's feedback felt as being between ambivalent to the change and perhaps
agree with the change, as long as pgindent did not throw a fit, which
it did not.
It definitely did not feel like a -1.

On Mon, Mar 3, 2025 at 8:40 AM Robert Haas <[email-masked]> wrote:
>
> On Sun, Mar 2, 2025 at 1:10 AM Gurjeet Singh <[email-masked]> wrote:
> > I propose the following change to the generation script, generate-lwlocknames.pl
> >
> > + printf $h "#define %-30s %s\n", "${lockname}Lock", "(&MainLWLockArray[$lockidx].lock)";
>
> -1. That seems worse to me.

I read your objection to be about the perl code change, whereas the real change
the patch was seeking is in the generated output (lwlocknames.h). I'm guessing
some others may also have taken your feedback to mean the same as I did.

Alvaro committed the following patch, which is better code than mine. If your
objection was about the perl code, perhaps this addresses your concern.

- print $h "#define ${lockname}Lock (&MainLWLockArray[$lockidx].lock)\n";
+ printf $h "#define %-32s (&MainLWLockArray[$lockidx].lock)\n",
+ $lockname . "Lock";

I have ~attached~ linked a comparison of before and after screenshots of the
generated output. It's hard to argue that the generated code in the left/before
image is better than the right/after image.

https://photos.app.goo.gl/hNL3FaUMuEwnaYTt9

Best regards,
Gurjeet
http://Gurje.et

---

From: Tom Lane <[email-masked]>
Date: Mon, 17 Mar 2025 02:58:32 -0400

> agree with the change, as long as pgindent did not throw a fit, which

Geez, are we really going to argue about whitespace in a generated
file?  But anyway, what I said was that I didn't like the style with
the define'd symbol right-justified, because it didn't look like
anything else in our code.  I'm entirely okay with the committed
version, which has plenty of precedent formatting-wise.

			regards, tom lane

---

From: Álvaro Herrera <[email-masked]>
Date: Mon, 17 Mar 2025 09:20:40 +0100

> attachment with a link, which might be the reason for being put in the

Kindly don't do this (specifically: cancel the original message and send
a different copy).  Just have patience with the moderation queue.
Messages are typically approved within a day anyway.

---

From: Robert Haas <[email-masked]>
Date: Mon, 17 Mar 2025 09:10:06 -0400

On Mon, Mar 17, 2025 at 2:43 AM Álvaro Herrera <[email-masked]> wrote:
> different change, which is not the one I committed.

Sorry, I should have read the emails more carefully. I missed the fact
that there were two different proposals. It was the idea of
right-aligning things that I was unhappy about.

So, no objection to what you actually committed... except that I don't
think that using % specifiers in SOME places in a format string is
better than using them in ALL the places. It's not broken because
$lockidx can't contain a % sign, but in general I think when we switch
from print to printf it's better for us to have the format string be a
constant so that it's clear that we can't accidentally get an extra %
escape in there depending on the values of variables being
interpolated.

---

From: Álvaro Herrera <[email-masked]>
Date: Mon, 17 Mar 2025 16:38:52 +0100

> right-aligning things that I was unhappy about.

Ah, okay.

> escape in there depending on the values of variables being

I suppose this is a reasonable complaint; however, I don't see an actual
problem here.  Even if I hack the regexp in generate-lwlocknames.pl to
accept a %-sign in the lock name (and introduce a matching % in
wait_event_names.txt), then that % is emitted verbatim rather than
attempted to further expand.  Is this because this is Perl rather than
C?  I'm not sure.

Note that a % in the lock number (which also needs a regexp hack) can't
cause a problem either, because of the check that the lock numbers are
an ordered sequence.

I think it's quite difficult to cause actual problems here.