Обсуждение: Add "format" target to make and ninja to run pgindent and pgperltidy

This tries to make running formatting a lot easier for committers, but
primarily for new contributors. You can not format the files by simply
running one of the folowing:

make format
ninja -C build format

My primary goal is to introduce Python formatting too (using ruff), and
integrate that in a similar manner. Right now we don't have many Python
files, but hopefully the pytest patch gets merged soonish and then we'll
get more and more Python files. So I'd like to get autoformatting out of
the gate.

But even without that, these rules should make it simpler to run the
current formatters:

Running pgindent is still not trivial for new users. You need to add our
pg_bsd_indent to PATH. And if you use meson to build you have to manually
specify src/ and contrib/ instead of ./ because otherwise pgindent will
try to format files in the build directory.

Running pgperltidy is even more complicated because you need to install
a specific version of perltidy. Recently we started erroring when that
version was not the one we expect.

On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> This tries to make running formatting a lot easier for committers, but
> primarily for new contributors. You can not format the files by simply
> running one of the folowing:
>
> make format
> ninja -C build format
>

I generally like the idea. Since perltidy is not enforced regularly
(like pgindent), running it usually ends up modifying files which are
not part of the patch. So I avoid it if not necessary. Do you propose
to make it optional?

--
Best Wishes,
Ashutosh Bapat

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
> On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>> This tries to make running formatting a lot easier for committers, but
>> primarily for new contributors.

> I generally like the idea. Since perltidy is not enforced regularly
> (like pgindent), running it usually ends up modifying files which are
> not part of the patch. So I avoid it if not necessary. Do you propose
> to make it optional?

The other obstacle is that not everybody will have the right version
of perltidy installed, but using some other version will create a
whole lot of extraneous noise.

On the whole I'd recommend not trying to automate the perltidy
step yet.  Cost/benefit is just not very good.

On the substance of the patch: I wonder whether we could make things
more reliable by using git metadata to figure out which .h and .c
files to point pgindent at.

            regards, tom lane

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:

On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

This tries to make running formatting a lot easier for committers, but
primarily for new contributors.

I generally like the idea. Since perltidy is not enforced regularly
(like pgindent), running it usually ends up modifying files which are
not part of the patch. So I avoid it if not necessary. Do you propose
to make it optional?

The other obstacle is that not everybody will have the right version
of perltidy installed, but using some other version will create a
whole lot of extraneous noise.

pgperltidy actually checks the version now.

On the whole I'd recommend not trying to automate the perltidy
step yet.  Cost/benefit is just not very good.

I'd kinda like to unify these universes, though. We could import the pgperltidy logic into pgindent but disable it unless some flag were given and the correct version of perltidy were present.

On the substance of the patch: I wonder whether we could make things
more reliable by using git metadata to figure out which .h and .c
files to point pgindent at.
			

The git pre-commit hook I use operates with this set of files:

    files=$(git diff --cached --name-only --diff-filter=ACMR)

pgindent just currently looks at that set of files and ignores everything that's not a .c or .h file.

I guess what you're wanting is a test to see if the file is in git or a generated file? That doesn't really arise for me as I always do vpath builds, so generated files are always elsewhere.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2025-12-31 We 10:26 AM, Tom Lane wrote:
>> On the substance of the patch: I wonder whether we could make things
>> more reliable by using git metadata to figure out which .h and .c
>> files to point pgindent at.

> I guess what you're wanting is a test to see if the file is in git or a 
> generated file? That doesn't really arise for me as I always do vpath 
> builds, so generated files are always elsewhere.

Right.  But if we're trying to make this easy, we need to make
the automation work for all three use-cases (in-tree makefiles,
vpath makefiles, meson).  I was just wondering if relying on
git would simplify getting the same results in all three.

            regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:

On 2025-12-31 We 10:26 AM, Tom Lane wrote:

On the substance of the patch: I wonder whether we could make things
more reliable by using git metadata to figure out which .h and .c
files to point pgindent at.

I guess what you're wanting is a test to see if the file is in git or a 
generated file? That doesn't really arise for me as I always do vpath 
builds, so generated files are always elsewhere.

Right.  But if we're trying to make this easy, we need to make
the automation work for all three use-cases (in-tree makefiles,
vpath makefiles, meson).  I was just wondering if relying on
git would simplify getting the same results in all three.
			

I think we could use

    git ls-files -t $file

or similar.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed Dec 31, 2025 at 4:26 PM CET, Tom Lane wrote:
> Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
>> On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>>> This tries to make running formatting a lot easier for committers, but
>>> primarily for new contributors.
>
>> I generally like the idea. Since perltidy is not enforced regularly
>> (like pgindent), running it usually ends up modifying files which are
>> not part of the patch. So I avoid it if not necessary. Do you propose
>> to make it optional?
>
> The other obstacle is that not everybody will have the right version
> of perltidy installed, but using some other version will create a
> whole lot of extraneous noise.
>
> On the whole I'd recommend not trying to automate the perltidy
> step yet.  Cost/benefit is just not very good.

I would like to get to a point where it is enforced for every commit
pushed by committers, so the same as with pgindent. I agree that the
cost/benefit is not there currently, but that's what this patchset is
trying to address. The docs in the last patch try to explain clearly how
to get and configure the correct version of perltidy. And once you've
done that it's as easy as running a single make/ninja command to format
all the files.

Do you think that's not still not easy enough for committers? What would
be needed instead? Automatically fetching and installing perltidy to the
local build path when running make/ninja format?

> On the substance of the patch: I wonder whether we could make things
> more reliable by using git metadata to figure out which .h and .c
> files to point pgindent at.

I think that would definitely be helpful. The fact that pgindent runs on
files in the meson build directory when passing "." would be solved by that.

"Jelte Fennema-Nio" <postgres@jeltef.nl> writes:
> On Wed Dec 31, 2025 at 4:26 PM CET, Tom Lane wrote:
>> On the whole I'd recommend not trying to automate the perltidy
>> step yet.  Cost/benefit is just not very good.

> I would like to get to a point where it is enforced for every commit
> pushed by committers, so the same as with pgindent.

As an affected committer, I want to push back against having such
a requirement, because I don't think it is reasonable to require
everybody to have precisely version XYZ of perltidy installed.
If that's not the version provided by their platform-of-choice,
it's an annoying hurdle.

As a comparison point, we did not start requiring pgindent cleanliness
until we imported bsdindent into our tree, so as not to have an
external dependency for that.  (But I can't see vendoring perltidy,
even if there weren't license issues involved.)

I recognize the analogy to requiring a specific version of autoconf,
but the difference is that without autoconf you just plain can't work
on the configure code.  Here, the hurdle would be erected for no
reason stronger than neatnik-ism, and IMO that's not a good enough
reason to put yet another burden on committers.

I'm even less pleased by the notion that we'd soon add still another
such requirement for python.

            regards, tom lane

On Wed, 31 Dec 2025 at 19:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> As an affected committer, I want to push back against having such
> a requirement, because I don't think it is reasonable to require
> everybody to have precisely version XYZ of perltidy installed.
> If that's not the version provided by their platform-of-choice,
> it's an annoying hurdle.

That's why I tried to make that hurdle as small as possible. All
that's needed with the current patchset is:

wget https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/Perl-Tidy-20230309.tar.gz
cpanm -l $HOME/perltidy-20230309 Perl-Tidy-20230309.tar.gz
meson setup build -DPERLTIDY=$HOME/perltidy-20230309/bin/perltidy --reconfigure

> As a comparison point, we did not start requiring pgindent cleanliness
> until we imported bsdindent into our tree, so as not to have an
> external dependency for that.  (But I can't see vendoring perltidy,
> even if there weren't license issues involved.)

I agree vendoring perltidy seems like a bad idea. Given you think
those 3 commands above are still an annoying hurdle, I'm curious what
you think would be a small enough hurdle to not be annoying? A few
options I can see:

1. src/tools/indent/get_perltidy /your/path/of/choice (and then give
this path to configure/meson)
2. make/ninja get-perltidy (downloads + installs to a known directory
inside the build dir)
3. make/ninja format (which then downloads perltidy if it's not yet available)

> Here, the hurdle would be erected for no
> reason stronger than neatnik-ism, and IMO that's not a good enough
> reason to put yet another burden on committers.

To me, the goal of autoformatting is the exact opposite of
neatnik-ism. It means devs don't have to worry about styling their
code nicely because the tool will fix it. So no mental capacity is
spent considering arbitrary styling decisions like where to put
newlines etc.

Jelte Fennema-Nio <postgres@jeltef.nl> writes:

> On Wed, 31 Dec 2025 at 19:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As an affected committer, I want to push back against having such
>> a requirement, because I don't think it is reasonable to require
>> everybody to have precisely version XYZ of perltidy installed.
>> If that's not the version provided by their platform-of-choice,
>> it's an annoying hurdle.
>
> That's why I tried to make that hurdle as small as possible. All
> that's needed with the current patchset is:
>
> wget https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/Perl-Tidy-20230309.tar.gz
> cpanm -l $HOME/perltidy-20230309 Perl-Tidy-20230309.tar.gz

You can ask cpanm to install a specific version without having to
download the tarball manually or knowing who made the release and which
archive format they used:

   cpanm -l $HOME/perltidy-20230309 Perl::Tidy@20230309

- ilmari

On Wed Dec 31, 2025 at 4:48 PM CET, Andrew Dunstan wrote:
> I'd kinda like to unify these universes, though. We could import the
> pgperltidy logic into pgindent but disable it unless some flag were
> given and the correct version of perltidy were present.

Attached is an initial patchset that does that, as well as improving
pgindent in various other ways. This has removed the make/meson
integration and instead made pgindent smarter at finding pg_bsd_indent
and perltidy. I believe that with all of these QoL improvements we could
enable perltidy in pgindent by default (after doing a full indent run).

I want to call pretty much all of this code was written by Claude Code
(i.e. an AI). I plan to do another review pass over this code soonish.
Especially the later commits, which I haven't checked in detail yet.
But for now I at least wanted to share the direction of what I've been
working on, because I saw that Peter marked himself as reviewer on the
commitfest app and I don't think it makes sense for him to look at the
previous patchset.

On 04.03.26 10:18, Jelte Fennema-Nio wrote:
> On Wed Dec 31, 2025 at 4:48 PM CET, Andrew Dunstan wrote:
>> I'd kinda like to unify these universes, though. We could import the 
>> pgperltidy logic into pgindent but disable it unless some flag were 
>> given and the correct version of perltidy were present.
> 
> Attached is an initial patchset that does that, as well as improving
> pgindent in various other ways. This has removed the make/meson
> integration and instead made pgindent smarter at finding pg_bsd_indent
> and perltidy. I believe that with all of these QoL improvements we could
> enable perltidy in pgindent by default (after doing a full indent run).
> 
> I want to call pretty much all of this code was written by Claude Code
> (i.e. an AI). I plan to do another review pass over this code soonish.
> Especially the later commits, which I haven't checked in detail yet.
> But for now I at least wanted to share the direction of what I've been
> working on, because I saw that Peter marked himself as reviewer on the
> commitfest app and I don't think it makes sense for him to look at the
> previous patchset.

Apparently, this is still work in progress, but here are some comments 
from me.

I'm very interested in the patch "pgindent: Clean up temp files on 
SIGINT", because this is an annoying problem.  But I didn't find any 
documentation about why this is the correct solution.  I didn't find 
anything on the File::Temp man page, for example.  Do you have more details?

The patch "pgindent: Use git ls-files to discover files" is also 
interested and pretty straightforward.  I guess we don't really care 
about being able to run this in non-git trees?

The other stuff I don't think I'm really on board with.  In particular, 
I don't like integrating pgperltidy into pgindent.  These things are in 
practice run at different times and the underlying tools update 
differently and require different management, so integrating them all 
might lead to various annoyances.

I kind of liked the original idea of a "make format", and then you could 
have subtargets like "make format-c" and "make format-perl" if you don't 
have all the tools.

The parallel pgindent is pretty nice, but I wonder how "use POSIX" works 
on Windows?

(But in practice I mostly use pgindent --commit=@, which is still much 
faster.)

On Thu Mar 12, 2026 at 10:10 AM CET, Peter Eisentraut wrote:
> Apparently, this is still work in progress, but here are some comments
> from me.

Thanks for the review. I went over the code in detail now myself, and
haven't found anything hugehely weird (although I did a bit of cleanup
and additional comments). As said before I'm definetly not a Perl expert
though, so I might have missed some wrong details. But all the logic at least
seems sound.

I also reorderd the commits to have the perltidy integration as the last
ones, and the general pgindent QoL imorovements first.

> I'm very interested in the patch "pgindent: Clean up temp files on
> SIGINT", because this is an annoying problem.  But I didn't find any
> documentation about why this is the correct solution.  I didn't find
> anything on the File::Temp man page, for example.  Do you have more details?

I explained this black magic better now, and also did the same for
SIGTERM. I also added a second commit which cleans up the .BAK files
that pg_bsd_indent creates. Feel free to combine those commits if you
think that's better.

> The patch "pgindent: Use git ls-files to discover files" is also
> interested and pretty straightforward.  I guess we don't really care
> about being able to run this in non-git trees?

I don't think we care. This is a tool for Postgres developers, and I
think we can assume all of those are using git to work on Postgres.

> The other stuff I don't think I'm really on board with.  In particular,
> I don't like integrating pgperltidy into pgindent.  These things are in
> practice run at different times and the underlying tools update
> differently and require different management, so integrating them all
> might lead to various annoyances.

So, to be clear. My goal is to get to a point where they ARE run at the
same time, because their goal is the same, just for different filetypes.
Afaict the primary reason that pg_bsd_indent is run on every commit, but
perltidy is not, is that the later is currently much more annoying to run.
Integrating it into pgindent is my attempt at making that barier much
lower, so that we can make it part of the standard workflow.

> I kind of liked the original idea of a "make format", and then you could
> have subtargets like "make format-c" and "make format-perl" if you don't
> have all the tools.

I liked it too initially, but I then realized that means you lose all
the nice flags that pgindent provides, e.g. --commit=@ or --check.
Ofcourse you could add a FORMATARGS variable, but at that point why not
just have people run pgindent directly? Also, perltidy is very slow to
run. Running it on the whole codebase takes about a minute for me. So
the new parallel and git ls-files support in pgindent are really helpful
to have it finish in a reasonable time.

> The parallel pgindent is pretty nice, but I wonder how "use POSIX" works
> on Windows?

Removed the POSIX thing. That wasn't needed. I'm wondering how many
people use pginden ton Windows though.

> (But in practice I mostly use pgindent --commit=@, which is still much
> faster.)

(--commit=@ should also get faster with the parallelization, because now
it can run of the files from the commit in parallel)

On 2026-03-12 Th 5:10 AM, Peter Eisentraut wrote:
> On 04.03.26 10:18, Jelte Fennema-Nio wrote:
>> On Wed Dec 31, 2025 at 4:48 PM CET, Andrew Dunstan wrote:
>>> I'd kinda like to unify these universes, though. We could import the 
>>> pgperltidy logic into pgindent but disable it unless some flag were 
>>> given and the correct version of perltidy were present.
>>
>> Attached is an initial patchset that does that, as well as improving
>> pgindent in various other ways. This has removed the make/meson
>> integration and instead made pgindent smarter at finding pg_bsd_indent
>> and perltidy. I believe that with all of these QoL improvements we could
>> enable perltidy in pgindent by default (after doing a full indent run).
>>
>> I want to call pretty much all of this code was written by Claude Code
>> (i.e. an AI). I plan to do another review pass over this code soonish.
>> Especially the later commits, which I haven't checked in detail yet.
>> But for now I at least wanted to share the direction of what I've been
>> working on, because I saw that Peter marked himself as reviewer on the
>> commitfest app and I don't think it makes sense for him to look at the
>> previous patchset.
>
> Apparently, this is still work in progress, but here are some comments 
> from me.
>
> I'm very interested in the patch "pgindent: Clean up temp files on 
> SIGINT", because this is an annoying problem.  But I didn't find any 
> documentation about why this is the correct solution.  I didn't find 
> anything on the File::Temp man page, for example.  Do you have more 
> details?
>
> The patch "pgindent: Use git ls-files to discover files" is also 
> interested and pretty straightforward.  I guess we don't really care 
> about being able to run this in non-git trees?
>
> The other stuff I don't think I'm really on board with.  In 
> particular, I don't like integrating pgperltidy into pgindent. These 
> things are in practice run at different times and the underlying tools 
> update differently and require different management, so integrating 
> them all might lead to various annoyances.
>
> I kind of liked the original idea of a "make format", and then you 
> could have subtargets like "make format-c" and "make format-perl" if 
> you don't have all the tools.
>
> The parallel pgindent is pretty nice, but I wonder how "use POSIX" 
> works on Windows?


POSIX is ok. fork() can be dubious (see `perldoc perlfork`). But we 
could disable the parallel stuff on Windows if necessary.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

> On 13 Mar 2026, at 09:29, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

> Afaict the primary reason that pg_bsd_indent is run on every commit, but
> perltidy is not, is that the later is currently much more annoying to run.

The primary reason thus far has been that pgperltidy requires a specific
version of perltidy, no other version is accepted.  Imposing the burden of
installing that on a greater subset of people than a subset of committers
didn't seem palatable.

--
Daniel Gustafsson

On Fri, 13 Mar 2026 at 11:03, Daniel Gustafsson <daniel@yesql.se> wrote:
> The primary reason thus far has been that pgperltidy requires a specific
> version of perltidy, no other version is accepted.  Imposing the burden of
> installing that on a greater subset of people than a subset of committers
> didn't seem palatable.

I think that makes sense if the burden is big, but with patch 0008 it
becomes as simple as running a single command:
src/tools/pgindent/get_perltidy

> On 13 Mar 2026, at 16:11, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Fri, 13 Mar 2026 at 11:03, Daniel Gustafsson <daniel@yesql.se> wrote:
>> The primary reason thus far has been that pgperltidy requires a specific
>> version of perltidy, no other version is accepted.  Imposing the burden of
>> installing that on a greater subset of people than a subset of committers
>> didn't seem palatable.
>
> I think that makes sense if the burden is big, but with patch 0008 it
> becomes as simple as running a single command:
> src/tools/pgindent/get_perltidy

In todays world of supply-chain attacks, don't think it's all that palatable to
take responsibility for installing code in peoples environments.  Regardless, I
don't think the patch should remove the installation instructions.

--
Daniel Gustafsson