Horton

Thread

Commits

  1. Add ABI extra field to fmgr magic block

  1. dfmgr additional ABI version fields

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-10-07T09:27:48Z

    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?

  2. Re: dfmgr additional ABI version fields

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-10-07T10:02:23Z

    čt 7. 10. 2021 v 11:28 odesílatel Peter Eisentraut <
peter.eisentraut@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

  3. Re: dfmgr additional ABI version fields

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-07T15:49:57Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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

  4. Re: dfmgr additional ABI version fields

    Andres Freund <andres@anarazel.de> — 2021-10-07T16:32:10Z

    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.

  5. Re: dfmgr additional ABI version fields

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-07T16:42:23Z

    Andres Freund <andres@anarazel.de> 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

  6. Re: dfmgr additional ABI version fields

    Chapman Flack <chap@anastigmatix.net> — 2021-10-07T16:56:22Z

    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

  7. Re: dfmgr additional ABI version fields

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-07T19:15:22Z

    Chapman Flack <chap@anastigmatix.net> 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

  8. Re: dfmgr additional ABI version fields

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-10-08T14:54:45Z

    On 07.10.21 21:15, Tom Lane wrote:
> Chapman Flack <chap@anastigmatix.net> 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.

  9. Re: dfmgr additional ABI version fields

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-10-12T12:13:40Z

    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.

  10. Re: dfmgr additional ABI version fields

    Robert Haas <robertmhaas@gmail.com> — 2021-10-13T16:50:38Z

    On Tue, Oct 12, 2021 at 8:13 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> 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

  11. Re: dfmgr additional ABI version fields

    Michael Paquier <michael@paquier.xyz> — 2021-11-19T07:58:52Z

    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

  12. Re: dfmgr additional ABI version fields

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-11-19T10:01:11Z

    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.

  13. Re: dfmgr additional ABI version fields

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-19T14:59:17Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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

  14. Re: dfmgr additional ABI version fields

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-11-22T07:23:08Z

    I have committed this patch as posted.