Thread

CI: Add task that runs pgindent

Jelte Fennema-Nio <me@jeltef.nl> — 2025-10-21T12:19:29Z

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.

Re: CI: Add task that runs pgindent

Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-10-21T13:22:42Z

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

Re: CI: Add task that runs pgindent

Daniel Gustafsson <daniel@yesql.se> — 2025-10-21T13:27:17Z

> 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

Re: CI: Add task that runs pgindent

Florents Tselai <florents.tselai@gmail.com> — 2025-10-21T13:39:43Z


> 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.

Re: CI: Add task that runs pgindent

Daniel Gustafsson <daniel@yesql.se> — 2025-10-21T13:46:23Z

> 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

Re: CI: Add task that runs pgindent

Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-10-21T14:10:15Z

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

Re: CI: Add task that runs pgindent

Jelte Fennema-Nio <me@jeltef.nl> — 2025-10-21T14:31:00Z

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).

Re: CI: Add task that runs pgindent

Jelte Fennema-Nio <me@jeltef.nl> — 2025-10-21T14:36:51Z

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.

Re: CI: Add task that runs pgindent

Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-21T14:39:57Z

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

Re: CI: Add task that runs pgindent

Jelte Fennema-Nio <me@jeltef.nl> — 2025-10-21T14:49:15Z

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.

Re: CI: Add task that runs pgindent

Daniel Gustafsson <daniel@yesql.se> — 2025-10-21T15:04:50Z

> 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

Re: CI: Add task that runs pgindent

Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-21T16:09:01Z

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

Re: CI: Add task that runs pgindent

Peter Eisentraut <peter@eisentraut.org> — 2025-10-21T17:39:39Z

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

Re: CI: Add task that runs pgindent

Michael Paquier <michael@paquier.xyz> — 2025-10-22T04:33:43Z

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

Re: CI: Add task that runs pgindent

Jelte Fennema-Nio <me@jeltef.nl> — 2025-10-22T08:41:49Z

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.

Re: CI: Add task that runs pgindent

Daniel Gustafsson <daniel@yesql.se> — 2025-10-22T08:49:27Z

> 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

Re: CI: Add task that runs pgindent

Jelte Fennema-Nio <me@jeltef.nl> — 2025-10-22T09:24:13Z

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/

Re: CI: Add task that runs pgindent

Daniel Gustafsson <daniel@yesql.se> — 2025-10-22T12:06:44Z

> 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

Re: CI: Add task that runs pgindent

Jelte Fennema-Nio <me@jeltef.nl> — 2025-10-23T09:40:12Z

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.

Re: CI: Add task that runs pgindent

Daniel Gustafsson <daniel@yesql.se> — 2025-10-23T14:27:11Z

> 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

Re: CI: Add task that runs pgindent

Peter Geoghegan <pg@bowt.ie> — 2025-10-23T15:14:18Z

On Wed, Oct 22, 2025 at 8:07 AM 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 strongly agree that this shouldn't signal to the user that they
really need to fix the problem. I'd like it if this information was
presented in the least obtrusive way possible. Constantly worrying
about a misplaced tab is a waste of time for all concerned.

-- 
Peter Geoghegan

Re: CI: Add task that runs pgindent

David G. Johnston <david.g.johnston@gmail.com> — 2025-10-23T15:58:23Z

On Thu, Oct 23, 2025, 11:14 Peter Geoghegan <pg@bowt.ie> wrote:

> On Wed, Oct 22, 2025 at 8:07 AM 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 strongly agree that this shouldn't signal to the user that they
> really need to fix the problem. I'd like it if this information was
> presented in the least obtrusive way possible. Constantly worrying
> about a misplaced tab is a waste of time for all concerned.
>


My thought is to use the new tags feature to add/remove a "Needs
Formatting" tag as the indicator.

David J.



>
>
>
>
>

Re: CI: Add task that runs pgindent

Daniel Gustafsson <daniel@yesql.se> — 2025-10-23T16:24:00Z

> On 23 Oct 2025, at 17:58, David G. Johnston <david.g.johnston@gmail.com> wrote:

> My thought is to use the new tags feature to add/remove a "Needs Formatting" tag as the indicator.

That's also an option, though I would avoid calling it "Needs" to again avoid
making in actionable instead of informative.

--
Daniel Gustafsson

Re: CI: Add task that runs pgindent

Andres Freund <andres@anarazel.de> — 2025-11-07T15:33:58Z

Hi,

On 2025-10-21 14:19:29 +0200, Jelte Fennema-Nio wrote:
> +# Check that code follows formatting standards
> +task:
> +  name: FormattingCheck

Why does this need to be a separate task, instead of being folded into the
CompilerWarnings task or such? Increasing the peak numbers of CPUs cfbot needs
is unfortunately somewhat annoying, as I've been fighting w/ GCP to increase
the quotas much higher than what they are right now.

Greetings,

Andres Freund

Re: CI: Add task that runs pgindent

Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-07T16:07:35Z

Andres Freund <andres@anarazel.de> writes:
> Why does this need to be a separate task, instead of being folded into the
> CompilerWarnings task or such? Increasing the peak numbers of CPUs cfbot needs
> is unfortunately somewhat annoying, as I've been fighting w/ GCP to increase
> the quotas much higher than what they are right now.

We had that discussion upthread, or maybe it was on Discord: we want
to keep formatting complaints separate from actual compiler warnings.
The latter should be fixed before the patch can be considered ready,
whereas pgindent might get left for the committer to do.  An example
where that's actually good practice is where the patch has to reindent
a chunk of existing code; submitting it without that makes review
simpler.

			regards, tom lane