s/pg_attribute_always_inline/pg_always_inline/?

On Wed, Apr 8, 2026 at 5:09 PM Andres Freund <andres@anarazel.de> wrote:

When reviewing the index prefetching patch I got a bit dismayed at how long
pg_attribute_always_inline is due to the way it triggers pgindent to format
stuff.

I had to significantly reorder function prototypes to avoid overly
long prototypes that had pg_attribute_always_inline. While perhaps not
strictly necessary, we shouldn't have to work around the fact that
pg_attribute_always_inline is an absurdly verbose symbol name.

I propose that we remove the _attribute_ part of the name.

+1

--
Peter Geoghegan

On 08.04.26 23:09, Andres Freund wrote:

Hi,

When reviewing the index prefetching patch I got a bit dismayed at how long
pg_attribute_always_inline is due to the way it triggers pgindent to format
stuff.

I propose that we remove the _attribute_ part of the name.

Given that it is implemented for compilers that don't use the
__attribute__((always_inline)) spelling, so the shorter name seems better
anyway.

Yes, and the existing name is also kind of wrong even on GCC because the
macro does not expand to only an attribute but also includes the "inline".

Hi,

On 2026-04-09 02:00:37 +0200, Peter Eisentraut wrote:

On 08.04.26 23:09, Andres Freund wrote:

Hi,

When reviewing the index prefetching patch I got a bit dismayed at how long
pg_attribute_always_inline is due to the way it triggers pgindent to format
stuff.

I propose that we remove the _attribute_ part of the name.

Given that it is implemented for compilers that don't use the
__attribute__((always_inline)) spelling, so the shorter name seems better
anyway.

Yes, and the existing name is also kind of wrong even on GCC because the
macro does not expand to only an attribute but also includes the "inline".

It didn't start out that way, that only came in a bit later, in
434e6e148441...

Created a CF entry, to reduce the chances of me forgetting about committing
this early in the 20 cycle.

Greetings,

Andres Freund

On Thu, Apr 9, 2026 at 10:40 AM Andres Freund <andres@anarazel.de> wrote:

Created a CF entry, to reduce the chances of me forgetting about committing
this early in the 20 cycle.

We already have a pg_noinline. How about renaming
pg_attribute_always_inline to pg_mustinline? That is an alternative
that is both consistent with pg_noinline, and even terser than your
proposal.

I have no intention of holding this patch up with bikeshedding. But I
noticed that even your proposed pg_always_inline rename still leaves
function prototypes over the column limit with moderately verbose
function names. It seems better to avoid that outcome.

--
Peter Geoghegan

On 5/28/26 00:17, Peter Geoghegan wrote:

On Thu, Apr 9, 2026 at 10:40 AM Andres Freund <andres@anarazel.de> wrote:

Created a CF entry, to reduce the chances of me forgetting about committing
this early in the 20 cycle.

We already have a pg_noinline. How about renaming
pg_attribute_always_inline to pg_mustinline? That is an alternative
that is both consistent with pg_noinline, and even terser than your
proposal.

I agree we should shorten pg_attribute_always_inline, it's way too
verbose. And other attributes don't include the _attribute_ either (like
the pg_noinline mentioned here).

I'm not sure about pg_mustinline. It seems weird to me, and I'm not sure
saving the 3 characters is worth it, pg_always_inline seems better.

I have no intention of holding this patch up with bikeshedding. But I
noticed that even your proposed pg_always_inline rename still leaves
function prototypes over the column limit with moderately verbose
function names. It seems better to avoid that outcome.

Yeah, we should not do such weird stuff just because of unnecessarily
long attribute names.

Question - do we plan to do this in master only, or was the plan to
backpatch the change? I'm not sure if these labels are used outside the
core code, that might be an issue for backpatching.

regards

--
Tomas Vondra

On Fri, Jun 12, 2026 at 11:23 AM Tomas Vondra <tomas@vondra.me> wrote:

I'm not sure about pg_mustinline. It seems weird to me, and I'm not sure
saving the 3 characters is worth it, pg_always_inline seems better.

I'm not going to make a fuss about it.

Yeah, we should not do such weird stuff just because of unnecessarily
long attribute names.

Right. Basically, I don't want to be told that I must completely
change the order of function definitions because I used
pg_[attribute]_always_inline. It's just not reasonable to impose that
requirement on patch authors.

Question - do we plan to do this in master only, or was the plan to
backpatch the change? I'm not sure if these labels are used outside the
core code, that might be an issue for backpatching.

I think that we should bite the bullet and backpatch. I count only 17
instances of pg_attribute_always_inline on the master branch.

Some extensions will no longer build against the backbranches if we go
this way. However, extension authors should find it easy to work
around this on an ad-hoc basis. They're going to have to work around
it sooner or later, so we might as well favor the new spelling.

--
Peter Geoghegan

On 6/12/26 18:10, Peter Geoghegan wrote:

On Fri, Jun 12, 2026 at 11:23 AM Tomas Vondra <tomas@vondra.me> wrote:

I'm not sure about pg_mustinline. It seems weird to me, and I'm not sure
saving the 3 characters is worth it, pg_always_inline seems better.

I'm not going to make a fuss about it.

Yeah, we should not do such weird stuff just because of unnecessarily
long attribute names.

Right. Basically, I don't want to be told that I must completely
change the order of function definitions because I used
pg_[attribute]_always_inline. It's just not reasonable to impose that
requirement on patch authors.

Question - do we plan to do this in master only, or was the plan to
backpatch the change? I'm not sure if these labels are used outside the
core code, that might be an issue for backpatching.

I think that we should bite the bullet and backpatch. I count only 17
instances of pg_attribute_always_inline on the master branch.

Some extensions will no longer build against the backbranches if we go
this way. However, extension authors should find it easy to work
around this on an ad-hoc basis. They're going to have to work around
it sooner or later, so we might as well favor the new spelling.

So, what shall we do about this? I've been looking at the v29 of the
index prefetching patch series, which is using this attribute in a bunch
of places. And that reminded me of this thread.

I guess we should just commit that, so unless someone objects soon I'll
just do that. ISTM the agreement is to backpatch this too.

regards

--
Tomas Vondra

On Thu, 9 Jul 2026 at 09:42, Tomas Vondra <tomas@vondra.me> wrote:

So, what shall we do about this? I've been looking at the v29 of the
index prefetching patch series, which is using this attribute in a bunch
of places. And that reminded me of this thread.

I guess we should just commit that, so unless someone objects soon I'll
just do that. ISTM the agreement is to backpatch this too.

How about just backpatching the new macro and leaving the old one in
place back branches? There should be slightly less backpatching pain
that way, for us, and fewer merge issues for people maintaining forks
on existing versions.

David

David Rowley <dgrowleyml@gmail.com> writes:

On Thu, 9 Jul 2026 at 09:42, Tomas Vondra <tomas@vondra.me> wrote:

I guess we should just commit that, so unless someone objects soon I'll
just do that. ISTM the agreement is to backpatch this too.

How about just backpatching the new macro and leaving the old one in
place back branches?

+1. Breaking valid code in released branches is unfriendly.

regards, tom lane

On Wed, Jul 8, 2026 at 5:41 PM Tomas Vondra <tomas@vondra.me> wrote:

So, what shall we do about this? I've been looking at the v29 of the
index prefetching patch series, which is using this attribute in a bunch
of places. And that reminded me of this thread.

I guess we should just commit that, so unless someone objects soon I'll
just do that. ISTM the agreement is to backpatch this too.

+1 to backpatching, along with a compatibility macro that supports the
original spelling on backbranches only.

--
Peter Geoghegan

On 7/9/26 02:14, Tom Lane wrote:

David Rowley <dgrowleyml@gmail.com> writes:

On Thu, 9 Jul 2026 at 09:42, Tomas Vondra <tomas@vondra.me> wrote:

I guess we should just commit that, so unless someone objects soon I'll
just do that. ISTM the agreement is to backpatch this too.

How about just backpatching the new macro and leaving the old one in
place back branches?

+1. Breaking valid code in released branches is unfriendly.

OK, that sounds reasonable. So in the backbranches we'd have both
pg_always_inline and the original pg_attriute_always_inline.

Does "backpatching the new macro" mean we'd also adjust all existing
places to use the new macro or not? I mean, we could backpatch just the
bit in c.h. I plan to adjust the places, to minimize possible conflicts
when backpatching other stuff.

regards

--
Tomas Vondra

Tomas Vondra <tomas@vondra.me> writes:

Does "backpatching the new macro" mean we'd also adjust all existing
places to use the new macro or not? I mean, we could backpatch just the
bit in c.h. I plan to adjust the places, to minimize possible conflicts
when backpatching other stuff.

I think it'd be sufficient to add the new macro to the old branches.
The point of back-patching at all, I think, is to save ourselves
work if we have to back-patch some new code that uses the new style.

regards, tom lane

On 7/10/26 02:14, Tom Lane wrote:

Tomas Vondra <tomas@vondra.me> writes:

Does "backpatching the new macro" mean we'd also adjust all existing
places to use the new macro or not? I mean, we could backpatch just the
bit in c.h. I plan to adjust the places, to minimize possible conflicts
when backpatching other stuff.

I think it'd be sufficient to add the new macro to the old branches.
The point of back-patching at all, I think, is to save ourselves
work if we have to back-patch some new code that uses the new style.

I'm not very worried about new code using the new macro name. We only
use it in ~50 places, and I don't think fixes will use it very often.
It's more likely a fix will touching some code nearby, and the
cherry-pick will fail because of the difference.

I intend to backpatch "everything", with backbranches having both macros
(as in the -19 patch).

regards

--
Tomas Vondra

On 7/10/26 14:34, Tomas Vondra wrote:

On 7/10/26 02:14, Tom Lane wrote:

Tomas Vondra <tomas@vondra.me> writes:

Does "backpatching the new macro" mean we'd also adjust all existing
places to use the new macro or not? I mean, we could backpatch just the
bit in c.h. I plan to adjust the places, to minimize possible conflicts
when backpatching other stuff.

I think it'd be sufficient to add the new macro to the old branches.
The point of back-patching at all, I think, is to save ourselves
work if we have to back-patch some new code that uses the new style.

I'm not very worried about new code using the new macro name. We only
use it in ~50 places, and I don't think fixes will use it very often.
It's more likely a fix will touching some code nearby, and the
cherry-pick will fail because of the difference.

I intend to backpatch "everything", with backbranches having both macros
(as in the -19 patch).

FWIW here are the per-version patches what I plan to commit + backpatch.

At some point I was worried we might end up with an awful mix of code
using the two variants of the macro in the backbranches (old removed
code with old naming, and new name in code updated by the backpatch).

But that really does not seem to happen. Essentially the only code stuck
with the old name (pg_atttribute_...) is three lines in tuplesort.c
(which disappeared in 19).

So I think it's fine. I still want to double-check the backpatches.

regards

--
Tomas Vondra

On Fri, Jul 10, 2026 at 12:37 PM Tomas Vondra <tomas@vondra.me> wrote:

FWIW here are the per-version patches what I plan to commit + backpatch.

LGTM.

--
Peter Geoghegan

Hi,

On 2026-07-10 12:46:01 -0400, Peter Geoghegan wrote:

On Fri, Jul 10, 2026 at 12:37 PM Tomas Vondra <tomas@vondra.me> wrote:

FWIW here are the per-version patches what I plan to commit + backpatch.

LGTM.

Same. Personally I'd not have adjusted the uses of the macro in the
backbranches, but I don't have a problem with you doing that either.

Greetings,

Andres Freund

On Fri, Jul 10, 2026 at 1:32 PM Andres Freund <andres@anarazel.de> wrote:

Same. Personally I'd not have adjusted the uses of the macro in the
backbranches, but I don't have a problem with you doing that either.

FWIW a quick Debian code search shows the following Debian packages
depend on the current pg_attribute_always_inline spelling (excluding
Postgres itself):

pg-stat-plans, documentdb, timescaledb, mobilitydb, libpg-query, postgresql-rum

libpg-query and mobilitydb don't really count, because they seem to
vendor src/postgres/include/c.h directly. postgresql-rum and
pg-stat-plans definitely use pg_attribute_always_inline as
straightforward extensions. I think that the rest are also simple
extensions that rely on the authoritative pg_attribute_always_inline,
but I didn't check.

--
Peter Geoghegan