Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Make lwlocknames.h generated file less ugly
- da0f0582e81e 18.0 landed
-
lwlocknames.h beautification attempt
Gurjeet Singh <gurjeet@singh.im> — 2025-03-02T06:09:47Z
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 -
Re: lwlocknames.h beautification attempt
Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-02T06:26:07Z
Gurjeet Singh <gurjeet@singh.im> writes: > I propose the following change to the generation script, > generate-lwlocknames.pl > ... > 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) This looks reasonably in line with project style ... > Yet another format, which I prefer, can be achieved by right-aligning the > lock names. > #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) ... but that doesn't. I challenge you to provide even one example of that layout in our source tree, or to explain why it's better. regards, tom lane
-
Re: lwlocknames.h beautification attempt
Gurjeet Singh <gurjeet@singh.im> — 2025-03-02T06:35:12Z
On Sat, Mar 1, 2025 at 10:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Gurjeet Singh <gurjeet@singh.im> writes: > > I propose the following change to the generation script, > > generate-lwlocknames.pl > > ... > > 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) > > This looks reasonably in line with project style ... Should I create a commitfest entry for this patch, or is it uncontroversial enough and small enough to not warrant that? > > Yet another format, which I prefer, can be achieved by right-aligning the > > lock names. > > > #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) > > ... but that doesn't. I challenge you to provide even one example > of that layout in our source tree, or to explain why it's better. I haven't seen this style in any other project, let alone in Postgres. I just mentioned it since I personally liked it slightly better after trying a couple of different styles, because of the aligned suffix in the lock names. Best regards, Gurjeet http://Gurje.et
-
Re: lwlocknames.h beautification attempt
Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-02T07:00:33Z
Gurjeet Singh <gurjeet@singh.im> writes: > On Sat, Mar 1, 2025 at 10:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> This looks reasonably in line with project style ... > Should I create a commitfest entry for this patch, or is it > uncontroversial enough and small enough to not warrant that? The controversy would be more about whether it's worth bothering with. In the past we've taken some care to ensure that generated files would pass pgindent's ideas of correct formatting, but not more than that. AFAICT from some quick tests, pgindent has no opinions about layout of #define lines. regards, tom lane
-
Re: lwlocknames.h beautification attempt
Robert Haas <robertmhaas@gmail.com> — 2025-03-03T16:40:39Z
On Sun, Mar 2, 2025 at 1:10 AM Gurjeet Singh <gurjeet@singh.im> wrote: > 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)"; -1. That seems worse to me. -- Robert Haas EDB: http://www.enterprisedb.com -
Re: lwlocknames.h beautification attempt
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-03T17:14:45Z
On 2025-Mar-01, Gurjeet Singh wrote: > 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) This format seems more than acceptable. It'd be nice to avoid printf for such a small thing; but at least we can use the %-specifiers only where necessary, so I'd do this instead: diff --git a/src/backend/storage/lmgr/generate-lwlocknames.pl b/src/backend/storage/lmgr/generate-lwlocknames.pl index 084da2ea6e0..2927f144905 100644 --- a/src/backend/storage/lmgr/generate-lwlocknames.pl +++ b/src/backend/storage/lmgr/generate-lwlocknames.pl @@ -108,7 +108,8 @@ while (<$lwlocklist>) $continue = ",\n"; # Add a "Lock" suffix to each lock name, as the C code depends on that - print $h "#define ${lockname}Lock (&MainLWLockArray[$lockidx].lock)\n"; + printf $h "#define %-32s (&MainLWLockArray[$lockidx].lock)\n", + "${lockname}Lock"; } die -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ -
Re: lwlocknames.h beautification attempt
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-14T19:38:18Z
On 2025-Mar-01, Gurjeet Singh wrote: > 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)"; 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 -
Re: lwlocknames.h beautification attempt
Robert Haas <robertmhaas@gmail.com> — 2025-03-17T02:53:25Z
On Fri, Mar 14, 2025 at 3:38 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote: > 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
-
Re: lwlocknames.h beautification attempt
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-17T06:43:41Z
On 2025-Mar-16, Robert Haas wrote: > On Fri, Mar 14, 2025 at 3:38 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > 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? 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. > 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. 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/202401231025.gbv4nnte5fmm@alvherre.pgsql -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
-
Fwd: lwlocknames.h beautification attempt
Gurjeet Singh <gurjeet@singh.im> — 2025-03-17T06:44:07Z
(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 <robertmhaas@gmail.com> wrote: > > On Fri, Mar 14, 2025 at 3:38 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > 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, 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 <robertmhaas@gmail.com> wrote: > > On Sun, Mar 2, 2025 at 1:10 AM Gurjeet Singh <gurjeet@singh.im> wrote: > > 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)"; > > -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 -
Re: Fwd: lwlocknames.h beautification attempt
Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-17T06:58:32Z
Gurjeet Singh <gurjeet@singh.im> writes: > On Sun, Mar 16, 2025 at 7:53 PM Robert Haas <robertmhaas@gmail.com> 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. 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
-
Re: Fwd: lwlocknames.h beautification attempt
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-17T08:20:40Z
On 2025-Mar-16, Gurjeet Singh wrote: > (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) 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. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
-
Re: lwlocknames.h beautification attempt
Robert Haas <robertmhaas@gmail.com> — 2025-03-17T13:10:06Z
On Mon, Mar 17, 2025 at 2:43 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote: > 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. 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. -- Robert Haas EDB: http://www.enterprisedb.com
-
Re: lwlocknames.h beautification attempt
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-17T15:38:52Z
On 2025-Mar-17, Robert Haas wrote: > On Mon, Mar 17, 2025 at 2:43 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > 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. > > 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. Ah, okay. > 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. 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. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/