CI: Add task that runs pgindent
At PGConf.dev 2025 one thing that came up in the "Scaling PostgreSQL
Development" unconference session is that new hackers don't know all the
details of our development flow by heart yet. Of course it's documented
on the wiki, but even if they find the relevant wiki pages they often
still miss/forget things. One of the things they often forget is
formatting their code. The consensus at that session was that it was
probably worth adding a CI task for this to nudge newcomers to indent
their code.
We're not too worried about this new requirement scaring away newcomers,
since autoformatting has become fairly commonplace in open source
development. Also committers can of course still choose to format the
patch themselves before committing if the formatting is failing.
This might also help reduce the number of unindented commits that
committers push, which require a follow up "fix indent" commit to make
the koel buildfarm animal happy again.
Attachments:
v1-0001-CI-Add-task-that-runs-pgindent.patchtext/x-patch; charset=utf-8; name=v1-0001-CI-Add-task-that-runs-pgindent.patchDownload+37-1
Hi,
On Tue, 21 Oct 2025 at 15:19, Jelte Fennema-Nio <me@jeltef.nl> wrote:
At PGConf.dev 2025 one thing that came up in the "Scaling PostgreSQL
Development" unconference session is that new hackers don't know all the
details of our development flow by heart yet. Of course it's documented
on the wiki, but even if they find the relevant wiki pages they often
still miss/forget things. One of the things they often forget is
formatting their code. The consensus at that session was that it was
probably worth adding a CI task for this to nudge newcomers to indent
their code.We're not too worried about this new requirement scaring away newcomers,
since autoformatting has become fairly commonplace in open source
development. Also committers can of course still choose to format the
patch themselves before committing if the formatting is failing.This might also help reduce the number of unindented commits that
committers push, which require a follow up "fix indent" commit to make
the koel buildfarm animal happy again.
Thank you for working on this! I think this is a great idea.
What do you think about moving this inside of another CI task instead
of creating a new one? I think that creating a new VM and cloning
Postgres into it would be unnecessary but I could not decide which
task would be a good choice for this.
Do you think that this task should format perl files as well?
Other than the things I mentioned above, the code looks good to me.
--
Regards,
Nazir Bilal Yavuz
Microsoft
On 21 Oct 2025, at 15:22, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
Do you think that this task should format perl files as well?
+1, if we do this we should run pgperltidy as well.
--
Daniel Gustafsson
On 21 Oct 2025, at 3:19 PM, Jelte Fennema-Nio <me@jeltef.nl> wrote:
At PGConf.dev 2025 one thing that came up in the "Scaling PostgreSQL
Development" unconference session is that new hackers don't know all the
details of our development flow by heart yet. Of course it's documented
on the wiki, but even if they find the relevant wiki pages they often
still miss/forget things. One of the things they often forget is
formatting their code. The consensus at that session was that it was
probably worth adding a CI task for this to nudge newcomers to indent
their code.We're not too worried about this new requirement scaring away newcomers,
since autoformatting has become fairly commonplace in open source
development. Also committers can of course still choose to format the
patch themselves before committing if the formatting is failing.This might also help reduce the number of unindented commits that
committers push, which require a follow up "fix indent" commit to make
the koel buildfarm animal happy again.
<v1-0001-CI-Add-task-that-runs-pgindent.patch>
I wonder if adding a pre-commit Git hook (for example under .git/hooks/pre-commit) might be an equally or even more suitable way to handle this?
That wouldn’t preclude having a CI task as well, of course.
The hook would mainly help contributors catch formatting issues locally, while the CI task would serve as a failsafe for committers.
On 21 Oct 2025, at 15:39, Florents Tselai <florents.tselai@gmail.com> wrote:
On 21 Oct 2025, at 3:19 PM, Jelte Fennema-Nio <me@jeltef.nl> wrote:
At PGConf.dev 2025 one thing that came up in the "Scaling PostgreSQL
Development" unconference session is that new hackers don't know all the
details of our development flow by heart yet. Of course it's documented
on the wiki, but even if they find the relevant wiki pages they often
still miss/forget things. One of the things they often forget is
formatting their code. The consensus at that session was that it was
probably worth adding a CI task for this to nudge newcomers to indent
their code.We're not too worried about this new requirement scaring away newcomers,
since autoformatting has become fairly commonplace in open source
development. Also committers can of course still choose to format the
patch themselves before committing if the formatting is failing.This might also help reduce the number of unindented commits that
committers push, which require a follow up "fix indent" commit to make
the koel buildfarm animal happy again.
<v1-0001-CI-Add-task-that-runs-pgindent.patch>I wonder if adding a pre-commit Git hook (for example under .git/hooks/pre-commit) might be an equally or even more suitable way to handle this?
That wouldn’t preclude having a CI task as well, of course.
The hook would mainly help contributors catch formatting issues locally, while the CI task would serve as a failsafe for committers.
One common argument against enforcing proper indentation in submissions is that
it adds a barrier to entry, it's the committers job to ensure the code is
according to project policy before committing, flagging patches as red in CI
doesn't really help as all committers will do the extra step regardless.
Conforming to indentation rules in v1 of a patchset isn't the most interesting
aspect of a submission, especially for WIP and POC style patches.
--
Daniel Gustafsson
Hi,
On Tue, 21 Oct 2025 at 16:46, Daniel Gustafsson <daniel@yesql.se> wrote:
On 21 Oct 2025, at 15:39, Florents Tselai <florents.tselai@gmail.com> wrote:
That wouldn’t preclude having a CI task as well, of course.
The hook would mainly help contributors catch formatting issues locally, while the CI task would serve as a failsafe for committers.One common argument against enforcing proper indentation in submissions is that
it adds a barrier to entry,
I think this patch targets two type of people, those who know how to run CI but:
1- Do not know how to run indentation checks
2- Forgot to run indentation checks
For #1, perhaps we could print a note suggesting people check
"src/tools/pgindent/README" when formatting fails. This might help
lower the barrier. Though it is possible it could raise it instead, I
am not entirely sure.
For #2, I think this patch is clearly helpful. We can discuss if it is
worth it to spend CI credits, though.
--
Regards,
Nazir Bilal Yavuz
Microsoft
On Tue, 21 Oct 2025 at 15:27, Daniel Gustafsson <daniel@yesql.se> wrote:
On 21 Oct 2025, at 15:22, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
Do you think that this task should format perl files as well?
+1, if we do this we should run pgperltidy as well.
I definitely agree that we should run that too. But currently the
master branch isn't correctly formatted according to pgperltidy (at
least with the pgperltidy on my machine I get a bunch of changes). So
I guess koel doesn't complain about that yet, and I didn't want to
conflate possible objectsions to that with objections to running
pgindent in CI. Once this CI job is in I wanted to start that
discussion for pgindent (and similarly a discussion on adding a python
formatter/linter).
On Tue, 21 Oct 2025 at 15:22, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
What do you think about moving this inside of another CI task instead
of creating a new one? I think that creating a new VM and cloning
Postgres into it would be unnecessary but I could not decide which
task would be a good choice for this.
I think the main benefit of keeping it separate is that it makes it
clear to both reviewers and authors that only the formatting is
incorrect, and no functionality is broken. The task is pretty quick
(~40 seconds) so I wouldn't be too worried about the resources it
uses.
Daniel Gustafsson <daniel@yesql.se> writes:
Conforming to indentation rules in v1 of a patchset isn't the most interesting
aspect of a submission, especially for WIP and POC style patches.
I have a more concrete argument: sometimes, it's helpful to submit
an un-pgindent'd patch because correct indentation will require
reindenting a large amount of existing code (because of addition or
removal of a layer of braces). Showing the effects of that in a
patch meant for review only makes the reviewer's life harder.
So I think there is plenty of room for workflows where the committer
is expected to reindent just before commit.
That's not to say that it couldn't be helpful for CI to point out
the need for indent. It's just to say that the test mustn't get
set up so that other tests don't run, or so that it looks like
there is any severe problem. That leads me to think it ought to be
a separate task.
regards, tom lane
On Tue, 21 Oct 2025 at 16:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Conforming to indentation rules in v1 of a patchset isn't the most interesting
aspect of a submission, especially for WIP and POC style patches.I have a more concrete argument: sometimes, it's helpful to submit
an un-pgindent'd patch because correct indentation will require
reindenting a large amount of existing code (because of addition or
removal of a layer of braces). Showing the effects of that in a
patch meant for review only makes the reviewer's life harder.
So I think there is plenty of room for workflows where the committer
is expected to reindent just before commit.
Interesting, but yeah that makes sense.
That's not to say that it couldn't be helpful for CI to point out
the need for indent. It's just to say that the test mustn't get
set up so that other tests don't run, or so that it looks like
there is any severe problem. That leads me to think it ought to be
a separate task.
Makes sense. By having it be a separate job I can easily make the
cfbot and commitfest app report it as "yellow" instead of "red" if
this job fails.
On 21 Oct 2025, at 16:31, Jelte Fennema-Nio <me@jeltef.nl> wrote:
On Tue, 21 Oct 2025 at 15:27, Daniel Gustafsson <daniel@yesql.se> wrote:
On 21 Oct 2025, at 15:22, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
Do you think that this task should format perl files as well?
+1, if we do this we should run pgperltidy as well.
I definitely agree that we should run that too. But currently the
master branch isn't correctly formatted according to pgperltidy (at
least with the pgperltidy on my machine I get a bunch of changes). So
I guess koel doesn't complain about that yet,
Indeed there is, I admittedly was under the impression that koel ran pgperltidy
but clearly I was wrong.
--
Daniel Gustafsson
Jelte Fennema-Nio <me@jeltef.nl> writes:
On Tue, 21 Oct 2025 at 16:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
So I think there is plenty of room for workflows where the committer
is expected to reindent just before commit.
Interesting, but yeah that makes sense.
Also, it's far from un-heard-of to actually make two separate
commits, so that the mechanical-reindenting step can be put
into .git-blame-ignore-revs. I had a recent example at
80aa9848b/73873805f/db6461b1c.
regards, tom lane
On 21.10.25 14:19, Jelte Fennema-Nio wrote:
At PGConf.dev 2025 one thing that came up in the "Scaling PostgreSQL
Development" unconference session is that new hackers don't know all the
details of our development flow by heart yet. Of course it's documented
on the wiki, but even if they find the relevant wiki pages they often
still miss/forget things. One of the things they often forget is
formatting their code. The consensus at that session was that it was
probably worth adding a CI task for this to nudge newcomers to indent
their code.
Good idea.
Additional suggestions:
Maybe there is a way to cache the pg_bsd_indent build. In my testing,
the configure and build steps each take 1/3 of the build time. Could be
worth it.
Also run the git whitespace check:
git diff-tree --check `git hash-object -t tree /dev/null` HEAD
On Tue, Oct 21, 2025 at 05:04:50PM +0200, Daniel Gustafsson wrote:
Indeed there is, I admittedly was under the impression that koel ran pgperltidy
but clearly I was wrong.
The original thread about forcing a stronger indentation policy was
that we could begin with the C code, leaving everything that is not C
up to the committer who pushes a change.
Another one would be a check on reformat-dat-files. Personally, I
tend to be careful with the dat files, or the indentation of the perl
code before pushing anything. The trend I am seeing is that a lot of
committers are very careful about that as well, so perhaps we could go
one step higher. Fireproof vest is on and ready for impact.
--
Michael
On Tue, 21 Oct 2025 at 16:49, Jelte Fennema-Nio <me@jeltef.nl> wrote:
By having it be a separate job I can easily make the
cfbot and commitfest app report it as "yellow" instead of "red" if
this job fails.
I set "allow_failures: true" in the cirrus task, so that a formatting
failure won't show the whole build as failed. I updated the commitfest
app to now show it with an orange cross instead of a red one (see
attached png). I included a patch that breaks indentation so I can
easily test this new behaviour end-to-end.
On 22 Oct 2025, at 10:41, Jelte Fennema-Nio <me@jeltef.nl> wrote:
I updated the commitfest
app to now show it with an orange cross instead of a red one (see
attached png).
Orange cross and Red cross are probably too similar to be easily discernible
for people with color blindness. Since the patch is technically "green", what
do you think about just changing the checkmark on such patches?
--
Daniel Gustafsson
On Wed, 22 Oct 2025 at 10:49, Daniel Gustafsson <daniel@yesql.se> wrote:
Orange cross and Red cross are probably too similar to be easily discernible
for people with color blindness. Since the patch is technically "green", what
do you think about just changing the checkmark on such patches?
Good point. I changed it to an orange triangle + exclamation point now
(see attached). And everything also seems to work as expected in the
commitfest app prod environment, because this patch now shows as
correctly as yellow: https://commitfest.postgresql.org/patch/6148/
Attachments:
On 22 Oct 2025, at 11:24, Jelte Fennema-Nio <me@jeltef.nl> wrote:
On Wed, 22 Oct 2025 at 10:49, Daniel Gustafsson <daniel@yesql.se> wrote:
Orange cross and Red cross are probably too similar to be easily discernible
for people with color blindness. Since the patch is technically "green", what
do you think about just changing the checkmark on such patches?Good point. I changed it to an orange triangle + exclamation point now
(see attached). And everything also seems to work as expected in the
commitfest app prod environment, because this patch now shows as
correctly as yellow: https://commitfest.postgresql.org/patch/6148/
I like the different shape and symbol, but I would probably keep it green to
indicate that it's informational rather than actionable. We don't want a
flurry of patch re-submissions with only whitespace changes eating CI resources
when the previous build was successful. Just my €0,02.
--
Daniel Gustafsson
On Wed, 22 Oct 2025 at 14:06, Daniel Gustafsson <daniel@yesql.se> wrote:
I like the different shape and symbol, but I would probably keep it green to
indicate that it's informational rather than actionable. We don't want a
flurry of patch re-submissions with only whitespace changes eating CI resources
when the previous build was successful. Just my €0,02.
I understand the concern. So I tried out making the icon green now
(see attached), but it looks a bit weird imo. Unless some others
prefer the green too (or have some other idea for an icon), I'm
inclined to keep it the yellow/orange color it is now. If we actually
do get a bunch of useless re-submissions, we can consider changing it.
Attachments:
On 23 Oct 2025, at 11:40, Jelte Fennema-Nio <me@jeltef.nl> wrote:
On Wed, 22 Oct 2025 at 14:06, Daniel Gustafsson <daniel@yesql.se> wrote:
I like the different shape and symbol, but I would probably keep it green to
indicate that it's informational rather than actionable. We don't want a
flurry of patch re-submissions with only whitespace changes eating CI resources
when the previous build was successful. Just my €0,02.I understand the concern. So I tried out making the icon green now
(see attached), but it looks a bit weird imo. Unless some others
prefer the green too (or have some other idea for an icon), I'm
inclined to keep it the yellow/orange color it is now. If we actually
do get a bunch of useless re-submissions, we can consider changing it.
If the set of icons grows from the self-explanatory red/green we should perhaps
document what the diffent colors mean in the help page?
--
Daniel Gustafsson
