Re: Our ABI diff infrastructure ignores enum SysCacheIdentifier

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