Обсуждение: Our ABI diff infrastructure ignores enum SysCacheIdentifier

I discovered $SUBJECT while working on commits dbb09fd8e et al.
The point of those commits was to back-patch addition of a syscache
that previously existed only in v18+, so naturally I was concerned
about not breaking ABI by changing any existing syscache's ID.
I tested whether abidiff would bleat about it if I intentionally
changed an ID, and found that *it didn't*.

I don't think this is (quite) a bug in libabigail.  They are pretty
clear that what they regard as ABI is:

       ... the descriptions of the types reachable by the interfaces
       (functions and variables) that are visible outside of their
       translation unit.

We do not use enum SysCacheIdentifier as the declared type of any
global variable or any globally-visible function argument or result.
Therefore it's not ABI per their definition.

This is frankly pretty scary.  Now that we know the rules, we can
fix it for enum SysCacheIdentifier, but we have other things that are
similarly vulnerable.  The thing I am most concerned about right now
is enum GUCs.  The guc.c APIs mandate that those be declared as type
int, so I think (haven't actually checked) that most if not all of
the associated enum values will not be perceived as ABI-relevant by
abidiff.  What can we do about that?

As for SysCacheIdentifier, the root of the problem is that
SearchSysCache and friends are declared to take "int cacheId"
not "enum SysCacheIdentifier cacheId".  Likely we should change
that in HEAD, but that'd be an actual not theoretical ABI break
(the enum's not necessarily int-width).  In the back branches
I'm thinking about adding a dummy function just for this purpose,
more or less as in the under-commented patch attached.

Thoughts?

            regards, tom lane

commit 12d2a8cd596d63dd908149b9140f527b723585cc
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Sat Jan 10 18:28:39 2026 -0500

    Make SysCacheIdentifier be ABI.

diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index f7f4f56a4d2..1af3ab3afdf 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -671,6 +671,14 @@ GetSysCacheHashValue(int cacheId,
     return GetCatCacheHashValue(SysCache[cacheId], key1, key2, key3, key4);
 }

+/*
+ * Dummy function with an "enum SysCacheIdentifier" parameter.
+ */
+void
+SysCacheDummy(enum SysCacheIdentifier cacheId)
+{
+}
+
 /*
  * List-search interface
  */
diff --git a/src/include/utils/syscache.h b/src/include/utils/syscache.h
index b541911c8fc..052c4550d22 100644
--- a/src/include/utils/syscache.h
+++ b/src/include/utils/syscache.h
@@ -72,6 +72,8 @@ extern Datum SysCacheGetAttrNotNull(int cacheId, HeapTuple tup,
 extern uint32 GetSysCacheHashValue(int cacheId,
                                    Datum key1, Datum key2, Datum key3, Datum key4);

+extern void SysCacheDummy(enum SysCacheIdentifier cacheId);
+
 /* list-search interface.  Users of this must import catcache.h too */
 struct catclist;
 extern struct catclist *SearchSysCacheList(int cacheId, int nkeys,

On Thu, Feb 12, 2026 at 11:17:37AM -0500, Tom Lane wrote:
> I discovered $SUBJECT while working on commits dbb09fd8e et al.
> The point of those commits was to back-patch addition of a syscache
> that previously existed only in v18+, so naturally I was concerned
> about not breaking ABI by changing any existing syscache's ID.
> I tested whether abidiff would bleat about it if I intentionally
> changed an ID, and found that *it didn't*.

Well.  That's annoying.  This one is important to be careful about.

> As for SysCacheIdentifier, the root of the problem is that
> SearchSysCache and friends are declared to take "int cacheId"
> not "enum SysCacheIdentifier cacheId".  Likely we should change
> that in HEAD, but that'd be an actual not theoretical ABI break
> (the enum's not necessarily int-width).

Enforcing a type for this sounds like a good idea in the long term on
HEAD, yes, even putting the ABI argument aside for a moment.  I'd
suggest the addition of an extra typedef in the code generated for
syscache_ids.h to save from a couple of enum markers in these
declarations, once refactored to use the new enum type.

> In the back branches
> I'm thinking about adding a dummy function just for this purpose,
> more or less as in the under-commented patch attached.
>
> Thoughts?

Adding a comment explaining why this function needs to exist would be
good.
--
Michael

On 2/12/26 5:17 PM, Tom Lane wrote:
> As for SysCacheIdentifier, the root of the problem is that
> SearchSysCache and friends are declared to take "int cacheId"
> not "enum SysCacheIdentifier cacheId".  Likely we should change
> that in HEAD, but that'd be an actual not theoretical ABI break
> (the enum's not necessarily int-width).  In the back branches
> I'm thinking about adding a dummy function just for this purpose,
> more or less as in the under-commented patch attached.
> 
> Thoughts?

Attached a patch which changes that in HEAD and I think for HEAD the 
best solution is the just fix all cases where we use ints like this to 
actually use the enum.

As for back branches I agree with Michael, just add a comment explaining 
why this dummy function is necessary.

Andreas

On Fri, Feb 13, 2026 at 06:46:55AM +0100, Andreas Karlsson wrote:
> Attached a patch which changes that in HEAD and I think for HEAD the best
> solution is the just fix all cases where we use ints like this to actually
> use the enum.

I was expecting something a bit more complicated.  Nice at quick
glance, Andreas.
--
Michael

On 2/13/26 9:24 AM, Michael Paquier wrote:
> I was expecting something a bit more complicated.  Nice at quick
> glance, Andreas.

It is a bit more code churn if I also include inval.c, but I still do 
not think it would be too bad.

Andreas

While it isn't strictly ABI, this made me wonder about the static
inline functions in public headers. Wouldn't extensions using those,
compiled with older minor versions have similar issues as issues
caused by actual ABI incompatibility?

I did a quick test with scripts about this in the REL 17 branch, and
there are several changes that could cause issues in theory.

See ffd9b8134658 for example introduced in 17.3. While technically
this is ABI compatible (nbanks is the same size as bank_mask in the
public struct, no binary change), part of the modified calculation is
in an inline function in the header, but part of it is in the C part.
Extensions compiled against 17.2 or earlier and using this
functionality will misbehave in 17.3 or later, as only part of the
code gets updated.

Similar issues could be caused by macro changes, for example COPYCHAR
was changed from memcpy to ts_copychar_cstr. That doesn't seem to be
an actual issue, but it could have been with a different change, and I
think similarly could go unnoticed.

There are also cases where something was simply removed from public
headers (firstbyte64(v) define from hashfn_unstable.h for example).
This again wouldn't cause any issues as long as the functions called
by it are still there, but it seems to break source compatibility
between minor versions.

On Fri, Feb 13, 2026 at 10:36:41AM +0100, Andreas Karlsson wrote:
> It is a bit more code churn if I also include inval.c, but I still do not
> think it would be too bad.

The blast looks acceptable with inval.c in sight.  What's less
acceptable is the set of failures generated, like:
#3  0x00000000019e7ac4 in ExceptionalCondition
(conditionName=0x1e31ff0 "cacheId >= 0 && cacheId < SysCacheSize &&
SysCache[cacheId]", fileName=0x1e31e80 "syscache.c",
lineNumber=223) at assert.c:65
#4  0x00000000019d277e in SearchSysCache1 (cacheId=4294967295,
key1=16778) at syscache.c:223

I didn't look beyond that.
--
Michael

On Mon, Feb 16, 2026 at 04:47:24PM +0900, Michael Paquier wrote:
> The blast looks acceptable with inval.c in sight.  What's less
> acceptable is the set of failures generated, like:
> #3  0x00000000019e7ac4 in ExceptionalCondition
> (conditionName=0x1e31ff0 "cacheId >= 0 && cacheId < SysCacheSize &&
> SysCache[cacheId]", fileName=0x1e31e80 "syscache.c",
> lineNumber=223) at assert.c:65
> #4  0x00000000019d277e in SearchSysCache1 (cacheId=4294967295,
> key1=16778) at syscache.c:223

The issue here is that we have three code paths that are perfectly OK
with dealing in negative syscache ID values:
- DropObjectById()@dependency.c
- AlterObjectRename_internal()@alter.c
- AlterObjectNamespace_internal()@alter.c

The best path moving forward on this one that I can think of in
objectaddress.c would be to add an extra "invalid" value in the enum
of SysCacheIdentifier and attach that to the ObjectProperty that we
can use to mark when we don't have an ID assigned.  That would have
the advantage to self-document the behavior that we don't have a
syscache entry at all when using this invalid ID.

What do you think about the updated version attached?
--
Michael

On 2/17/26 7:58 AM, Michael Paquier wrote:
> On Mon, Feb 16, 2026 at 04:47:24PM +0900, Michael Paquier wrote:
>> The blast looks acceptable with inval.c in sight.  What's less
>> acceptable is the set of failures generated, like:
>> #3  0x00000000019e7ac4 in ExceptionalCondition
>> (conditionName=0x1e31ff0 "cacheId >= 0 && cacheId < SysCacheSize &&
>> SysCache[cacheId]", fileName=0x1e31e80 "syscache.c",
>> lineNumber=223) at assert.c:65
>> #4  0x00000000019d277e in SearchSysCache1 (cacheId=4294967295,
>> key1=16778) at syscache.c:223
> 
> The issue here is that we have three code paths that are perfectly OK
> with dealing in negative syscache ID values:
> - DropObjectById()@dependency.c
> - AlterObjectRename_internal()@alter.c
> - AlterObjectNamespace_internal()@alter.c
> 
> The best path moving forward on this one that I can think of in
> objectaddress.c would be to add an extra "invalid" value in the enum
> of SysCacheIdentifier and attach that to the ObjectProperty that we
> can use to mark when we don't have an ID assigned.  That would have
> the advantage to self-document the behavior that we don't have a
> syscache entry at all when using this invalid ID.
> 
> What do you think about the updated version attached?

Yeah, that looks like a quite nice improvement. My only comment is that 
if it was me I would have split it into two patches, one introducing the 
invalid and one replacing int. But you are much more familiar than me 
with what granularity of commits the project prefers

Andreas

On Tue, Feb 17, 2026 at 09:20:44AM +0100, Andreas Karlsson wrote:
> Yeah, that looks like a quite nice improvement. My only comment is that if
> it was me I would have split it into two patches, one introducing the
> invalid and one replacing int. But you are much more familiar than me with
> what granularity of commits the project prefers

Splitting that into two is probably better, yes.  Even if both changes
touch the same portions of perl script, it makes the introduction of
the two concepts cleaner.
--
Michael

On Tue, Feb 17, 2026 at 05:59:31PM +0900, Michael Paquier wrote:
> On Tue, Feb 17, 2026 at 09:20:44AM +0100, Andreas Karlsson wrote:
>> Yeah, that looks like a quite nice improvement. My only comment is that if
>> it was me I would have split it into two patches, one introducing the
>> invalid and one replacing int. But you are much more familiar than me with
>> what granularity of commits the project prefers
>
> Splitting that into two is probably better, yes.  Even if both changes
> touch the same portions of perl script, it makes the introduction of
> the two concepts cleaner.

The conclusion of this exercise is that I have spotted two more spots
that checked for an invalid syscache ID based on a hardcoded value of
-1, and I have replaced them with the new value assigned in the enum.
A few more of these checked for a positive value.  There was also one
spot that I've found was incorrect, fixed separately as of
f7df12a66cc9.

The buildfarm is not complaining after c06b5b99bbb0 and ee642cccc43c,
meaning that we are hopefully good for v19 and future versions.
--
Michael

Hi,

On 2026-02-18 11:34:19 +0900, Michael Paquier wrote:
> The buildfarm is not complaining after c06b5b99bbb0 and ee642cccc43c,
> meaning that we are hopefully good for v19 and future versions.

I'm not happy that this change exposed syscache.h much more widely. Before
ee642cccc43c syscache.h was not included by any header. Now it is very widely
included, via objectaddress.h, which in turn is included by a lot of other
headers.

With ee642cccc43c a change to syscache.h rebuilds 632 files. With ee642cccc43c
reverted, it's just 196.

Leaving build impact aside, I don't think it's good to expose a relatively low
level detail like syscache.h to most of the backend. It's imo something that
only .c, never .h files should need.

Greetings,

Andres Freund

On Fri, Mar 27, 2026 at 05:17:49PM -0400, Andres Freund wrote:
> With ee642cccc43c a change to syscache.h rebuilds 632 files. With ee642cccc43c
> reverted, it's just 196.

Point received.

> Leaving build impact aside, I don't think it's good to expose a relatively low
> level detail like syscache.h to most of the backend. It's imo something that
> only .c, never .h files should need.

And as we already define SysCacheIdentifier in its own header, this
can be answered with the attached, removing the need for syscache.h in
objectaddress.h and inval.h.  The trick in genbki.pl was needed to
avoid some noise due to -Wenum-compare in a couple of files.

Would you prefer a different option?  That would protect from large
rebuilds should syscache.h be touched in some way.  A different option
would be to move get_object_catcache_oid() and
get_object_catcache_name() out of objectaddress.h to a different
header, limiting the scope of what's pulled in objectaddress.h.
Anyway, the attached should take care of your main concern, I guess?
--
Michael

Hi,

On 2026-04-06 09:01:24 +0900, Michael Paquier wrote:
> On Fri, Mar 27, 2026 at 05:17:49PM -0400, Andres Freund wrote:
> > With ee642cccc43c a change to syscache.h rebuilds 632 files. With ee642cccc43c
> > reverted, it's just 196.
>
> Point received.
>
> > Leaving build impact aside, I don't think it's good to expose a relatively low
> > level detail like syscache.h to most of the backend. It's imo something that
> > only .c, never .h files should need.
>
> And as we already define SysCacheIdentifier in its own header, this
> can be answered with the attached, removing the need for syscache.h in
> objectaddress.h and inval.h.

It's somewhat gross to have to include syscache_ids.h, but unfortunately with
C++ not allowing forward declarations of C enums, I'm not sure we have
particularly good alternatives.



> The trick in genbki.pl was needed to avoid some noise due to -Wenum-compare
> in a couple of files.

You mean the include guards?  Seems they should be added regardless of
anything else.


> Would you prefer a different option?

Frankly, I'm a bit doubtful that ee642cccc43c is worth the cost.

All this trouble to switch to SysCacheIdentifier in a bunch of places, when
enums provide basically no typesafety. And sure, it maybe could help us to
detect some ABI change, but I'm a bit doubtful anybody would think that
renumbering syscaches in the back branches is sane.  What are we actually
gaining here?

I'm doubtful that numeric keys fo syscaches, and one global list of them, is
the right long term direction. What does this number actually gain us? C has
working symbol names for global objects, why do we want a numeric key?

Right now every syscache is allocated dynamically, in every backend. Every
syscache lookup has to get the address of the actual syscache via
  static CatCache *SysCache[SysCacheSize]

In our silliness we even exist to do this via different translation units
(syscache.c -> catcache.c).

ISTM a better direction would be to make MAKE_SYSCACHE(name,idxname,nbuckets)
declare something like
   extern SysCache name;

where SysCache is a forward declared struct type with the definition private
to a C file or an internals header.

And then have genbki emit definitions of those that gets included into a C
file. That struct can then have all the necessary spce to avoid having to
having to allocate as much and perhaps even get some of the metadata specified
at compile time, so it doesn't have to be redone in every backend.



> Would you prefer a different option?  That would protect from large
> rebuilds should syscache.h be touched in some way.  A different option
> would be to move get_object_catcache_oid() and
> get_object_catcache_name() out of objectaddress.h to a different
> header, limiting the scope of what's pulled in objectaddress.h.

I frankly would just make those return an integer.


> Anyway, the attached should take care of your main concern, I guess?

It'd be better than today.  I don't like the syscache ids being known
everywhere, but it's better than those being known as well as the rest of
syscache.h.

Greetings,

Andres Freund

On Sun, Apr 05, 2026 at 10:09:48PM -0400, Andres Freund wrote:
>> The trick in genbki.pl was needed to avoid some noise due to -Wenum-compare
>> in a couple of files.
>
> You mean the include guards?  Seems they should be added regardless of
> anything else.

They would be needed with this patch.  Now we don't need them as
syscache.h is the only location where syscache_ids.h is pulled in.

> ISTM a better direction would be to make MAKE_SYSCACHE(name,idxname,nbuckets)
> declare something like
>    extern SysCache name;
>
> where SysCache is a forward declared struct type with the definition private
> to a C file or an internals header.
>
> And then have genbki emit definitions of those that gets included into a C
> file. That struct can then have all the necessary spce to avoid having to
> having to allocate as much and perhaps even get some of the metadata specified
> at compile time, so it doesn't have to be redone in every backend.

Perhaps. not for v19 for sure.

>> Would you prefer a different option?  That would protect from large
>> rebuilds should syscache.h be touched in some way.  A different option
>> would be to move get_object_catcache_oid() and
>> get_object_catcache_name() out of objectaddress.h to a different
>> header, limiting the scope of what's pulled in objectaddress.h.
>
> I frankly would just make those return an integer.

Not sure about that.  We know what we are getting by calling this API
with the type defined, at least.

>> Anyway, the attached should take care of your main concern, I guess?
>
> It'd be better than today.  I don't like the syscache ids being known
> everywhere, but it's better than those being known as well as the rest of
> syscache.h.

Well, the main point being to be able to detect breakages more
carefully, I am still curious to see where this experiment will lead
us, so I'd be content to leave the code as-is on HEAD, adjusting
things based on what I have sent in my previous email.  If we are able
to detect one problem, at least, that would be a win for me, and the
solution of HEAD is much better than creating fake routines to tell
ABI detection libraries about the existence of the enum, at least
that's my take.

If others have any comments and/or opinions, feel free of course.
--
Michael

On Wed, Apr 08, 2026 at 11:28:13AM +0900, Michael Paquier wrote:
> Well, the main point being to be able to detect breakages more
> carefully, I am still curious to see where this experiment will lead
> us, so I'd be content to leave the code as-is on HEAD, adjusting
> things based on what I have sent in my previous email.  If we are able
> to detect one problem, at least, that would be a win for me, and the
> solution of HEAD is much better than creating fake routines to tell
> ABI detection libraries about the existence of the enum, at least
> that's my take.

The footprint of syscache.h has been reduced in src/include/ as of
e0fa5bd14656, for now, after more tweaks applied to the format of the
file generated due to the new ifndef.
--
Michael