Add "format" target to make and ninja to run pgindent and pgperltidy

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

On 2025-12-31 We 10:26 AM, 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.

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

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

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

On 13.03.26 09:29, Jelte Fennema-Nio wrote:

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.

v3-0001-pgindent-Clean-up-temp-files-created-by-File-Temp.patch
v3-0002-pgindent-Always-clean-up-.BAK-files-from-pg_bsd_i.patch

I have committed these two.

v3-0003-pgindent-Use-git-ls-files-to-discover-files.patch

This looks structurally correct, but I wonder whether this:

my @git_files = `git ls-files -- @dirs`;

could be written in a way that is more robust against funny file names.
(pgindent is also used against trees that are not stock PostgreSQL.)

v3-0004-pgindent-Default-to-indenting-the-current-directo.patch

Note that other tools also share this behavior with pgindent:

src/tools/pgindent/pgperltidy
src/tools/perlcheck/pgperlcritic
src/tools/perlcheck/pgperlsyncheck

If we change one, we should change all.

It might be that the current behavior is intentional for some nonobvious
reason, not sure.

v3-0005-pgindent-Allow-parallel-pgindent-runs.patch
v3-0006-pgindent-Try-to-find-pg_bsd_indent-binary-in-comm.patch
v3-0007-pgindent-Integrate-pgperltidy-functionality-into-.patch
v3-0008-pgindent-Add-easy-way-of-getting-perltidy.patch

Still need to check these in more detail. They will probably carry over
into PG20.

Peter Eisentraut <peter@eisentraut.org> writes:

v3-0007-pgindent-Integrate-pgperltidy-functionality-into-.patch
v3-0008-pgindent-Add-easy-way-of-getting-perltidy.patch

Still need to check these in more detail. They will probably carry over
into PG20.

I still think that 0007 and 0008 need to be rejected, not postponed.
There is no cleanup that will make them acceptable in my eyes.

We did not start expecting commits to be pgindent-clean until pgindent
was integrated into our tree, providing (a) simple management of the
One True Version to use for each branch and (b) a trivial, safe way
to get that version. We will never put perltidy into our tree, so
I don't see that it can ever be on the same footing as pgindent.
The 0008 patch doesn't fix that, and in fact I think it would be
dangerous to even provide that script in our tree. It's a supply-
chain attack waiting to happen. Even if it were guaranteed 100%
secure, too many developers are subject to (perfectly reasonable)
corporate security policies that would look with disfavor on
unauthorized installation of Perl modules.

regards, tom lane

On Fri, 27 Mar 2026 at 15:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:

We did not start expecting commits to be pgindent-clean until pgindent
was integrated into our tree

Merging these commits does not mean we force committers to run
perltidy on every commit. That a completely separate discussion that
is not worth having until after we make perltidy less of a pain to
run. Even without forcing committers to run perltidy I think 0007 and
0008 are still beneficial.

The 0008 patch doesn't fix that, and in fact I think it would be
dangerous to even provide that script in our tree. It's a supply-
chain attack waiting to happen.

I strongly disagree. Instead I think, our current pgindent README[1]https://github.com/postgres/postgres/blob/9a9998163bda0d8c17d84ea22ced6a60f8018634/src/tools/pgindent/README#L18-L27
is a supply-chain attack waiting to happen. Our pgindent README tells
people to get a tar file from the CPAN website, but WITHOUT the
signature checks that the script in 0008 includes. These added
signature checks prevent it from being a supply chain risk.

Even if it were guaranteed 100%
secure, too many developers are subject to (perfectly reasonable)
corporate security policies that would look with disfavor on
unauthorized installation of Perl modules.

I'd be curious to know which committer is not allowed to download and
run a specific signature verified perl module, but is allowed to get
the latest postgres source code from main.

[1]: https://github.com/postgres/postgres/blob/9a9998163bda0d8c17d84ea22ced6a60f8018634/src/tools/pgindent/README#L18-L27

P.S. Reading your response, I cannot help but interpret it as an
attempt to sidestep any future discussion about always running
perltidy, by pre-emptively rejecting any and all improvements that
would make perltidy easier to run.