public inbox for [email protected]
help / color / mirror / Atom feedRe: Our ABI diff infrastructure ignores enum SysCacheIdentifier
6+ messages / 2 participants
[nested] [flat]
* Re: Our ABI diff infrastructure ignores enum SysCacheIdentifier
@ 2026-02-18 02:34 Michael Paquier <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2026-02-18 02:34 UTC (permalink / raw)
To: Andreas Karlsson <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Our ABI diff infrastructure ignores enum SysCacheIdentifier
@ 2026-03-27 21:17 Andres Freund <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andres Freund @ 2026-03-27 21:17 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andreas Karlsson <[email protected]>; Tom Lane <[email protected]>; [email protected]
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Our ABI diff infrastructure ignores enum SysCacheIdentifier
@ 2026-04-06 00:01 Michael Paquier <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2026-04-06 00:01 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Andreas Karlsson <[email protected]>; Tom Lane <[email protected]>; [email protected]
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
From d5eef6cc0b96ba4bdbab1b388ca24f7a35b1e7a2 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Mon, 6 Apr 2026 08:13:46 +0900
Subject: [PATCH] Remove some syscache includes
---
src/include/catalog/objectaddress.h | 2 +-
src/include/utils/inval.h | 2 +-
src/backend/catalog/genbki.pl | 6 +++++-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/include/catalog/objectaddress.h b/src/include/catalog/objectaddress.h
index b549be2d523e..1f965e1faef4 100644
--- a/src/include/catalog/objectaddress.h
+++ b/src/include/catalog/objectaddress.h
@@ -14,10 +14,10 @@
#define OBJECTADDRESS_H
#include "access/htup.h"
+#include "catalog/syscache_ids.h"
#include "nodes/parsenodes.h"
#include "storage/lockdefs.h"
#include "utils/relcache.h"
-#include "utils/syscache.h"
/*
* An ObjectAddress represents a database object of any type.
diff --git a/src/include/utils/inval.h b/src/include/utils/inval.h
index 5f64fb204776..735e42f73108 100644
--- a/src/include/utils/inval.h
+++ b/src/include/utils/inval.h
@@ -15,9 +15,9 @@
#define INVAL_H
#include "access/htup.h"
+#include "catalog/syscache_ids.h"
#include "storage/relfilelocator.h"
#include "utils/relcache.h"
-#include "utils/syscache.h"
extern PGDLLIMPORT int debug_discard_caches;
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 48c6805f7527..80ef3bcd1685 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -795,7 +795,10 @@ print $fk_info "};\n\n#endif\t\t\t\t\t\t\t/* SYSTEM_FK_INFO_H */\n";
# Now generate syscache info
print_boilerplate($syscache_ids_fh, "syscache_ids.h", "SysCache identifiers");
-print $syscache_ids_fh "typedef enum SysCacheIdentifier
+print $syscache_ids_fh "#ifndef SYSCACHE_IDS_H
+#define SYSCACHE_IDS_H
+
+typedef enum SysCacheIdentifier
{
\tSYSCACHEID_INVALID = -1,\n";
@@ -834,6 +837,7 @@ foreach my $syscache (sort keys %syscaches)
print $syscache_ids_fh "} SysCacheIdentifier;\n";
print $syscache_ids_fh "#define SysCacheSize ($last_syscache + 1)\n";
+print $syscache_ids_fh "#endif\t\t/* SYSCACHE_IDS_H */";
print $syscache_info_fh "};\n";
--
2.53.0
Attachments:
[text/plain] 0001-Remove-some-syscache-includes.patch (2.1K, 2-0001-Remove-some-syscache-includes.patch)
download | inline diff:
From d5eef6cc0b96ba4bdbab1b388ca24f7a35b1e7a2 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Mon, 6 Apr 2026 08:13:46 +0900
Subject: [PATCH] Remove some syscache includes
---
src/include/catalog/objectaddress.h | 2 +-
src/include/utils/inval.h | 2 +-
src/backend/catalog/genbki.pl | 6 +++++-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/include/catalog/objectaddress.h b/src/include/catalog/objectaddress.h
index b549be2d523e..1f965e1faef4 100644
--- a/src/include/catalog/objectaddress.h
+++ b/src/include/catalog/objectaddress.h
@@ -14,10 +14,10 @@
#define OBJECTADDRESS_H
#include "access/htup.h"
+#include "catalog/syscache_ids.h"
#include "nodes/parsenodes.h"
#include "storage/lockdefs.h"
#include "utils/relcache.h"
-#include "utils/syscache.h"
/*
* An ObjectAddress represents a database object of any type.
diff --git a/src/include/utils/inval.h b/src/include/utils/inval.h
index 5f64fb204776..735e42f73108 100644
--- a/src/include/utils/inval.h
+++ b/src/include/utils/inval.h
@@ -15,9 +15,9 @@
#define INVAL_H
#include "access/htup.h"
+#include "catalog/syscache_ids.h"
#include "storage/relfilelocator.h"
#include "utils/relcache.h"
-#include "utils/syscache.h"
extern PGDLLIMPORT int debug_discard_caches;
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 48c6805f7527..80ef3bcd1685 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -795,7 +795,10 @@ print $fk_info "};\n\n#endif\t\t\t\t\t\t\t/* SYSTEM_FK_INFO_H */\n";
# Now generate syscache info
print_boilerplate($syscache_ids_fh, "syscache_ids.h", "SysCache identifiers");
-print $syscache_ids_fh "typedef enum SysCacheIdentifier
+print $syscache_ids_fh "#ifndef SYSCACHE_IDS_H
+#define SYSCACHE_IDS_H
+
+typedef enum SysCacheIdentifier
{
\tSYSCACHEID_INVALID = -1,\n";
@@ -834,6 +837,7 @@ foreach my $syscache (sort keys %syscaches)
print $syscache_ids_fh "} SysCacheIdentifier;\n";
print $syscache_ids_fh "#define SysCacheSize ($last_syscache + 1)\n";
+print $syscache_ids_fh "#endif\t\t/* SYSCACHE_IDS_H */";
print $syscache_info_fh "};\n";
--
2.53.0
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Our ABI diff infrastructure ignores enum SysCacheIdentifier
@ 2026-04-06 02:09 Andres Freund <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andres Freund @ 2026-04-06 02:09 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andreas Karlsson <[email protected]>; Tom Lane <[email protected]>; [email protected]
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Our ABI diff infrastructure ignores enum SysCacheIdentifier
@ 2026-04-08 02:28 Michael Paquier <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2026-04-08 02:28 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Andreas Karlsson <[email protected]>; Tom Lane <[email protected]>; [email protected]
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Our ABI diff infrastructure ignores enum SysCacheIdentifier
@ 2026-04-08 23:54 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Michael Paquier @ 2026-04-08 23:54 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Andreas Karlsson <[email protected]>; Tom Lane <[email protected]>; [email protected]
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-04-08 23:54 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-02-18 02:34 Re: Our ABI diff infrastructure ignores enum SysCacheIdentifier Michael Paquier <[email protected]>
2026-03-27 21:17 ` Andres Freund <[email protected]>
2026-04-06 00:01 ` Michael Paquier <[email protected]>
2026-04-06 02:09 ` Andres Freund <[email protected]>
2026-04-08 02:28 ` Michael Paquier <[email protected]>
2026-04-08 23:54 ` Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox