Re: dfmgr additional ABI version fields
I have committed this patch as posted.
Re: dfmgr additional ABI version fields
Peter Eisentraut writes: >> On Wed, Oct 13, 2021 at 12:50:38PM -0400, Robert Haas wrote: >>> Would it be reasonable to consider something similar for the control >>> file, for the benefit of distributions that are not the same on disk? > The catalog version already serves this purpose. We already have fields in pg_control for that, and fields to check endianness, maxalign, etc, ie the things that matter for data storage. Perhaps there is a need for more such fields, but I don't see that extension ABI questions are directly related. regards, tom lane
Re: dfmgr additional ABI version fields
On 19.11.21 08:58, Michael Paquier wrote: On Wed, Oct 13, 2021 at 12:50:38PM -0400, Robert Haas wrote: I'm not a different vendor, but I do work on different code than you do, and I like this. Advanced Server accidentally dodges this problem at present by shipping with a different FUNC_MAX_ARGS value, but this is much cleaner. I am pretty sure that Greenplum could benefit from something like that. As a whole, using a string looks like a good idea for that. Would it be reasonable to consider something similar for the control file, for the benefit of distributions that are not the same on disk? Hmm. Wouldn't that cause more harm than actual benefits? The catalog version already serves this purpose.
Re: dfmgr additional ABI version fields
On Wed, Oct 13, 2021 at 12:50:38PM -0400, Robert Haas wrote: > I'm not a different vendor, but I do work on different code than you > do, and I like this. Advanced Server accidentally dodges this problem > at present by shipping with a different FUNC_MAX_ARGS value, but this > is much cleaner. I am pretty sure that Greenplum could benefit from something like that. As a whole, using a string looks like a good idea for that. > Would it be reasonable to consider something similar for the control > file, for the benefit of distributions that are not the same on disk? Hmm. Wouldn't that cause more harm than actual benefits? -- Michael signature.asc Description: PGP signature
Re: dfmgr additional ABI version fields
On Tue, Oct 12, 2021 at 8:13 AM Peter Eisentraut wrote: > So here is a patch. This does what I had in mind as a use case. > Obviously, the naming and wording can be tuned. Input from other > vendors is welcome. I'm not a different vendor, but I do work on different code than you do, and I like this. Advanced Server accidentally dodges this problem at present by shipping with a different FUNC_MAX_ARGS value, but this is much cleaner. Would it be reasonable to consider something similar for the control file, for the benefit of distributions that are not the same on disk? -- Robert Haas EDB: http://www.enterprisedb.com
Re: dfmgr additional ABI version fields
So here is a patch. This does what I had in mind as a use case.
Obviously, the naming and wording can be tuned. Input from other
vendors is welcome.
From 837680c2195a04bf1f1ecf567fadc3a3d69087fa Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 12 Oct 2021 14:08:10 +0200
Subject: [PATCH v1] Add ABI extra field to fmgr magic block
This allows derived products to intentionally make their fmgr ABI
incompatible, with a clean error message.
Discussion:
https://www.postgresql.org/message-id/flat/55215fda-db31-a045-d6b7-d6f2d2dc9920%40enterprisedb.com
---
src/backend/utils/fmgr/dfmgr.c | 15 +++
src/include/fmgr.h | 7 ++-
src/include/pg_config_manual.h | 17 +
3 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 96fd9d2268..cc1a520070 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -330,6 +330,21 @@ incompatible_module_error(const char *libname,
magic_data.version / 100,
library_version)));
}
+ /*
+* Similarly, if the ABI extra field doesn't match, error out. Other
+* fields below might also mismatch, but that isn't useful information
if
+* you're using the wrong product altogether.
+*/
+ if (strcmp(module_magic_data->abi_extra, magic_data.abi_extra) != 0)
+ {
+ ereport(ERROR,
+ (errmsg("incompatible library \"%s\": ABI
mismatch",
+ libname),
+errdetail("Server has ABI \"%s\", library has
\"%s\".",
+ magic_data.abi_extra,
+
module_magic_data->abi_extra)));
+ }
+
/*
* Otherwise, spell out which fields don't agree.
*
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index ab7b85c86e..cec663bdff 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -458,6 +458,7 @@ typedef struct
int indexmaxkeys; /* INDEX_MAX_KEYS */
int namedatalen;/* NAMEDATALEN */
int float8byval;/* FLOAT8PASSBYVAL */
+ charabi_extra[32]; /* see pg_config_manual.h */
} Pg_magic_struct;
/* The actual data block contents */
@@ -468,9 +469,13 @@ typedef struct
FUNC_MAX_ARGS, \
INDEX_MAX_KEYS, \
NAMEDATALEN, \
- FLOAT8PASSBYVAL \
+ FLOAT8PASSBYVAL, \
+ FMGR_ABI_EXTRA, \
}
+StaticAssertDecl(sizeof(FMGR_ABI_EXTRA) <=
sizeof(((Pg_magic_struct*)0)->abi_extra),
+"FMGR_ABI_EXTRA too long");
+
/*
* Declare the module magic function. It needs to be a function as the dlsym
* in the backend is only guaranteed to work on functions, not data
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 614035e215..225c5b87cc 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -42,6 +42,23 @@
*/
#define FUNC_MAX_ARGS 100
+/*
+ * When creating a product derived from PostgreSQL with changes that cause
+ * incompatibilities for loadable modules, it is recommended to change this
+ * string so that dfmgr.c can refuse to load incompatible modules with a clean
+ * error message. Typical examples that cause incompatibilities are any
+ * changes to node tags or node structures. (Note that dfmgr.c already
+ * detects common sources of incompatibilities due to major version
+ * differences and due to some changed compile-time constants. This setting
+ * is for catching anything that cannot be detected in a straightforward way.)
+ *
+ * There is no prescribed format for the string. The suggestion is to include
+ * product or company name, and optionally any internally-relevant ABI
+ * version. Example: "ACME Postgres/1.2". Note that the string will appear
+ * in a user-facing error message if an ABI mismatch is detected.
+ */
+#define FMGR_ABI_EXTRA "PostgreSQL"
+
/*
* Maximum number of columns in an index. There is little point in making
* this anything but a multiple of 32, because the main cost is associated
--
2.33.0
Re: dfmgr additional ABI version fields
On 07.10.21 21:15, Tom Lane wrote: Chapman Flack writes: On 10/07/21 12:42, Tom Lane wrote: Can we make the addition be a string not a number, so that we could include something more useful than "1234" in the error message? Just using a string like "EDB v" + something would probably rule out collisions in practice. To be more formal about it, something like the tag URI scheme [0] could be recommended. Hmm. Personally I'm more interested in the string being comprehensible to end users than in whether there's any formal rule guaranteeing uniqueness. I really doubt that we will have any practical problem with collisions, so I'd rather go with something like "EnterpriseDB v1.2.3" than with something like "tag:enterprisedb.com,2021:1.2.3". Yeah, just a string should be fine.
Re: dfmgr additional ABI version fields
Chapman Flack writes: > On 10/07/21 12:42, Tom Lane wrote: >> Can we make the addition be a string not a number, so that we >> could include something more useful than "1234" in the error >> message? > Just using a string like "EDB v" + something would probably rule out > collisions in practice. To be more formal about it, something like > the tag URI scheme [0] could be recommended. Hmm. Personally I'm more interested in the string being comprehensible to end users than in whether there's any formal rule guaranteeing uniqueness. I really doubt that we will have any practical problem with collisions, so I'd rather go with something like "EnterpriseDB v1.2.3" than with something like "tag:enterprisedb.com,2021:1.2.3". Conceivably we could have two strings, or a printable string and a chosen-at-random unique number (the latter not meant to be shown to users). Not sure it's worth the trouble though. regards, tom lane
Re: dfmgr additional ABI version fields
On 10/07/21 12:42, Tom Lane wrote: > Can we make the addition be a string not a number, so that we > could include something more useful than "1234" in the error > message? I was wondering the same thing, just to sidestep the "who hands out IDs" question. Just using a string like "EDB v" + something would probably rule out collisions in practice. To be more formal about it, something like the tag URI scheme [0] could be recommended. Nothing at runtime would have to know or care about tag URI syntax; it would just match a string with a fixed opaque prefix and some suffix. The scheme gives the developer an easy way to construct a meaningful and reliably non-colliding string. Surely loading libraries isn't a hot enough operation to begrudge a strcmp. Regards, -Chap [0] https://datatracker.ietf.org/doc/html/rfc4151
Re: dfmgr additional ABI version fields
Andres Freund writes: > On October 7, 2021 8:49:57 AM PDT, Tom Lane >> I'm also kind of unclear on why we need to do anything about this >> in the community version. If someone has forked PG and changed >> APIs to the extent that extensions are unlikely to work, there's >> not much stopping them from also making the two-line change >> to fmgr.h that would be needed to guarantee that different magic >> struct contents are needed. > I can see two reasons. First, it'd probably allow stock pg to generate a > better error message when confronted with such a module. Second, there's some > value in signaling forks that they should change (or think about changing), > that field. Hmm, ok, I can buy the first of those arguments. Less sure about the second, but the first is reason enough. Can we make the addition be a string not a number, so that we could include something more useful than "1234" in the error message? Something like "Module is built for EDB v1234.56" seems like it'd be a lot more on-point to the average user, and it gets us out of having to design the ABI versioning scheme that a fork should use. regards, tom lane
Re: dfmgr additional ABI version fields
Hi, On October 7, 2021 8:49:57 AM PDT, Tom Lane >I'm also kind of unclear on why we need to do anything about this >in the community version. If someone has forked PG and changed >APIs to the extent that extensions are unlikely to work, there's >not much stopping them from also making the two-line change >to fmgr.h that would be needed to guarantee that different magic >struct contents are needed. I can see two reasons. First, it'd probably allow stock pg to generate a better error message when confronted with such a module. Second, there's some value in signaling forks that they should change (or think about changing), that field. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: dfmgr additional ABI version fields
Peter Eisentraut writes: > I'm thinking about adding two more int fields to Pg_magic_struct: a > product or vendor magic number, and an ABI version that can be used > freely within a product/vendor. Who would hand out these magic numbers? If the answer is "choose a random one, it probably won't collide" then I'm not sure why we need two fields. You can choose a new random number for each ABI version, if you're changing it faster than once per PG major version. I'm also kind of unclear on why we need to do anything about this in the community version. If someone has forked PG and changed APIs to the extent that extensions are unlikely to work, there's not much stopping them from also making the two-line change to fmgr.h that would be needed to guarantee that different magic struct contents are needed. regards, tom lane
Re: dfmgr additional ABI version fields
čt 7. 10. 2021 v 11:28 odesílatel Peter Eisentraut < peter.eisentr...@enterprisedb.com> napsal: > When producing a forked version of PostgreSQL, there is no > straightforward way to enforce that users don't accidentally load > modules built for the non-forked (standard, community) version. You can > only distinguish by PostgreSQL major version and a few compile-time > settings. (see internal_load_library(), Pg_magic_struct) Depending on > the details, mixing and matching might even work, until it doesn't, so > this is a bad experience. > > I'm thinking about adding two more int fields to Pg_magic_struct: a > product or vendor magic number, and an ABI version that can be used > freely within a product/vendor. > > Would anyone else have use for this? Any thoughts? > +1 Pavel
