Add Postgres module info

Hi,

I would like to propose the module_info structure, which aims to let
extension maintainers sew some data into the binary file. Being included
in the module code, this information remains unchanged and is available
for reading by a backend.
As I see, this question was already debated during the introduction of
PG_MODULE_MAGIC [1], and developers didn't add such data because the
practical case wasn't obvious. Right now, when multiple extensions are
maintained and used in installations constantly, we have enough
information to continue this discussion.
The idea was initially born with the support of the extension, which had
a stable UI and frequently changing library code. To answer customer
requests, it was necessary to know the specific version of the code to
reproduce the situation in a test environment. That required introducing
a file naming rule and a specific exported variable into the module
code. However, this is not a sufficient guarantee of version
determination and complicates the technical process when supporting many
extensions obtained from different sources.
Another example is a library without an installation script at all - see
auto_explain. Without a 'CREATE EXTENSION' call, we don't have an option
to find out the library's version.
It would be much easier if the Postgres catalogue contained a function,
for example, module_info(module_name), which would allow you to
determine the file's full path and name containing the desired module in
the psql console and its version.
On the other hand, the Omnigres project (author Yurii Rashkovski) also
came up with the idea of ​​​​module versioning, although it does this
externally out of the Postgres core. When designing this code, I also
adopted ideas from this repository.
So, let me propose a patch that introduces this tiny feature: the
maintainer can add the PG_MODULE_INFO macro to the library code, and
Postgres reads it on the module's load.

There is a question of how much information makes sense to add to the
module. For now, each time I prepare extensions to release, I have to
add the extension name (to avoid issues with file naming/renaming) and
the version. Format of the version storage? Do we need a separate minor
version number? It is a subject to debate.

[1]
https://www.postgresql.org/message-id/flat/20060507211705.GB3808%40svana.org

--
regards, Andrei Lepikhov

Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
> I would like to propose the module_info structure, which aims to let
> extension maintainers sew some data into the binary file. Being included
> in the module code, this information remains unchanged and is available
> for reading by a backend.

I don't have much of an opinion one way or the other about the
usefulness of these additional info fields. But I would like to
object to the way you've gone about it, namely to copy-and-paste
the magic-block mechanism. That doesn't scale: the next time
somebody else wants some more fields, will we have three such
structs?

The approach we foresaw using was that we could simply add more
fields to Pg_magic_struct (obviously, only in a major version).
That's happened at least once already - abi_extra was not there
to begin with.

There are a couple of ways that we could deal with the API
seen by module authors:

1. The PG_MODULE_MAGIC macro keeps the same API and leaves the
additional field(s) empty. Authors who want to fill the
extra field(s) use a new macro, say PG_MODULE_MAGIC_V2.

2. PG_MODULE_MAGIC gains some arguments, forcing everybody
to change their code. While this would be annoying, it'd be
within our compatibility rules for a major version update.
I wouldn't do it though unless there were a compelling reason
why everybody should fill these fields.

3. Maybe we could do something with making PG_MODULE_MAGIC
variadic, but I've not thought hard about what that could
look like. In any case it'd only be a cosmetic improvement
over the above ways.

4. The extra fields are filled indirectly by macros that
extension authors can optionally provide (a variant on the
FMGR_ABI_EXTRA mechanism). This would be code-order-sensitive
so I'm not sure it's really a great idea.

5. Something I didn't think of?

With any of these except #4, authors who want their source code to
support multiple PG major versions would be forced into using #if
tests on CATALOG_VERSION_NO to decide what to write. That's a
bit annoying but hardly unusual.

regards, tom lane

Hi,

On 2024-12-11 13:21:03 -0500, Tom Lane wrote:
> Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
> > I would like to propose the module_info structure, which aims to let
> > extension maintainers sew some data into the binary file. Being included
> > in the module code, this information remains unchanged and is available
> > for reading by a backend.
>
> I don't have much of an opinion one way or the other about the
> usefulness of these additional info fields.

FWIW, Id like to have some more information in there, without commenting on
the specifics.

> But I would like to object to the way you've gone about it, namely to
> copy-and-paste the magic-block mechanism. That doesn't scale: the next time
> somebody else wants some more fields, will we have three such structs?

I agree with that.

> The approach we foresaw using was that we could simply add more
> fields to Pg_magic_struct (obviously, only in a major version).
> That's happened at least once already - abi_extra was not there
> to begin with.
>
> There are a couple of ways that we could deal with the API
> seen by module authors:
>
> 1. The PG_MODULE_MAGIC macro keeps the same API and leaves the
> additional field(s) empty. Authors who want to fill the
> extra field(s) use a new macro, say PG_MODULE_MAGIC_V2.
>
> 2. PG_MODULE_MAGIC gains some arguments, forcing everybody
> to change their code. While this would be annoying, it'd be
> within our compatibility rules for a major version update.
> I wouldn't do it though unless there were a compelling reason
> why everybody should fill these fields.

I'd like to avoid needing to do this again if / when we invent the next set of
optional arguments. So just having a different macro with a hardcoded set of
arguments or changing PG_MODULE_MAGIC to have a hardcoded set of arguments
doesn't seem great.

To be future proof, I think it'd be good to declare the arguments as
designated initializers. E.g. like

PG_MODULE_MAGIC_EXT(
.version = 10000,
.threadsafe = 1
);

where the macro would turn the arguments into a struct initializer inside
Pg_magic_struct.

That way we can add/remove arguments and only extensions that use
removed arguments need to change.

> 3. Maybe we could do something with making PG_MODULE_MAGIC
> variadic, but I've not thought hard about what that could
> look like. In any case it'd only be a cosmetic improvement
> over the above ways.

Yea, it'd be nice to avoid needing an _EXT or _V2. But I can't immediately
think of a way that allows a macro with no arguments and and an argument.

> 4. The extra fields are filled indirectly by macros that
> extension authors can optionally provide (a variant on the
> FMGR_ABI_EXTRA mechanism). This would be code-order-sensitive
> so I'm not sure it's really a great idea.

Agreed.

> With any of these except #4, authors who want their source code to
> support multiple PG major versions would be forced into using #if
> tests on CATALOG_VERSION_NO to decide what to write. That's a
> bit annoying but hardly unusual.

#2 would be bit more annoying than #1, I'd say, because it'd affect every
single extension, even ones not interested in any of this.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2024-12-11 13:21:03 -0500, Tom Lane wrote:
>> There are a couple of ways that we could deal with the API
>> seen by module authors:

> To be future proof, I think it'd be good to declare the arguments as
> designated initializers. E.g. like

> PG_MODULE_MAGIC_EXT(
> .version = 10000,
> .threadsafe = 1
> );

Yeah, I'd come to pretty much the same conclusion after sending
my email. That looks like it should work and be convenient
to extend further.

The other possibly-non-obvious bit is that we should probably
invent a sub-structure holding the ABI-related fields, so as to
minimize the amount of rewriting needed in dfmgr.c.

regards, tom lane

On Wed, Dec 11, 2024, at 4:26 PM, Andres Freund wrote:
> On 2024-12-11 13:21:03 -0500, Tom Lane wrote:
> > Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
> > > I would like to propose the module_info structure, which aims to let
> > > extension maintainers sew some data into the binary file. Being included
> > > in the module code, this information remains unchanged and is available
> > > for reading by a backend.
> >
> > I don't have much of an opinion one way or the other about the
> > usefulness of these additional info fields.
>
> FWIW, Id like to have some more information in there, without commenting on
> the specifics.

+1 for the general idea. I received some reports like [1] related to wal2json
that people wants to obtain the output plugin version. Since it is not installed
via CREATE EXTENSION, it is not possible to detect what version is installed,
hence, some tools cannot have some logic to probe the module version. In a
managed environment, it is hard to figure out the exact version for
non-CREATE-EXTENSION modules, unless it is explicitly informed by the vendor.

[1] https://github.com/eulerto/wal2json/issues/181

--
Euler Taveira
EDB https://www.enterprisedb.com/

On 12/12/2024 01:21, Tom Lane wrote:
> Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
>> I would like to propose the module_info structure, which aims to let
>> extension maintainers sew some data into the binary file. Being included
>> in the module code, this information remains unchanged and is available
>> for reading by a backend.
>
> I don't have much of an opinion one way or the other about the
> usefulness of these additional info fields. But I would like to
> object to the way you've gone about it, namely to copy-and-paste
> the magic-block mechanism. That doesn't scale: the next time
> somebody else wants some more fields, will we have three such
> structs?
It makes sense. But I want to clarify that I avoided changing
PG_MODULE_MAGIC because the newly introduced structure has a totally
different purpose and usage logic: the struct is designed to check
compatibility, but module info isn't connected to the core version at
all: a single version of the code may be built for multiple PG versions.
At the same time, various versions of the same library may be usable
with the same core.

From the coding point of view, I agree that your approach is more
laconic and reasonable. I will rewrite the code using this approach.

--
regards, Andrei Lepikhov

"Euler Taveira" <euler(at)eulerto(dot)com> writes:
> +1 for the general idea. I received some reports like [1] related to wal2json
> that people wants to obtain the output plugin version. Since it is not installed
> via CREATE EXTENSION, it is not possible to detect what version is installed,
> hence, some tools cannot have some logic to probe the module version. In a
> managed environment, it is hard to figure out the exact version for
> non-CREATE-EXTENSION modules, unless it is explicitly informed by the vendor.

What would you foresee as the SQL API for inspecting a module that's
not tied to an extension?

regards, tom lane

Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
> It makes sense. But I want to clarify that I avoided changing
> PG_MODULE_MAGIC because the newly introduced structure has a totally
> different purpose and usage logic: the struct is designed to check
> compatibility, but module info isn't connected to the core version at
> all: a single version of the code may be built for multiple PG versions.
> At the same time, various versions of the same library may be usable
> with the same core.

Surely. But I don't see a need for two separately-looked-up
physical structures. Seems to me it's sufficient to put the
ABI-checking fields into a sub-struct within the magic block.

regards, tom lane

On Wed, Dec 11, 2024 at 08:34:28PM -0500, Tom Lane wrote:
> "Euler Taveira" <euler(at)eulerto(dot)com> writes:
>> +1 for the general idea. I received some reports like [1] related to wal2json
>> that people wants to obtain the output plugin version. Since it is not installed
>> via CREATE EXTENSION, it is not possible to detect what version is installed,
>> hence, some tools cannot have some logic to probe the module version. In a
>> managed environment, it is hard to figure out the exact version for
>> non-CREATE-EXTENSION modules, unless it is explicitly informed by the vendor.
>
> What would you foresee as the SQL API for inspecting a module that's
> not tied to an extension?

Rather than a function that can be called with a specific module name
in input, invent a new system SRF function that would report back for
a process all the libraries that have been loaded in it? Presumably,
the extra tracking can be done in dfmgr.c with more fields added to
DynamicFileList to track the information involved.

Being able to print the information of DynamicFileList can be argued
as useful on its own as long as its execution can be granted, with
superuser right by default.
--
Michael

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> On Wed, Dec 11, 2024 at 08:34:28PM -0500, Tom Lane wrote:
>> What would you foresee as the SQL API for inspecting a module that's
>> not tied to an extension?

> Rather than a function that can be called with a specific module name
> in input, invent a new system SRF function that would report back for
> a process all the libraries that have been loaded in it?

Yeah, that could work.

> Presumably,
> the extra tracking can be done in dfmgr.c with more fields added to
> DynamicFileList to track the information involved.

I wouldn't add any overhead to the normal case for this. Couldn't
we walk the list and re-fetch each module's magic block inside
this new function?

regards, tom lane

On Wed, Dec 11, 2024 at 10:39:38PM -0500, Tom Lane wrote:
> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>> Presumably,
>> the extra tracking can be done in dfmgr.c with more fields added to
>> DynamicFileList to track the information involved.
>
> I wouldn't add any overhead to the normal case for this. Couldn't
> we walk the list and re-fetch each module's magic block inside
> this new function?

Depends on how much we should try to cache to make that less expensive
on repeated calls because we cannot unload libraries, but sure, I
don't see why we could not that for each SQL function call to simplify
the logic and the structures in place.
--
Michael

On 12/12/24 10:44, Michael Paquier wrote:
> On Wed, Dec 11, 2024 at 10:39:38PM -0500, Tom Lane wrote:
>> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>>> Presumably,
>>> the extra tracking can be done in dfmgr.c with more fields added to
>>> DynamicFileList to track the information involved.
>>
>> I wouldn't add any overhead to the normal case for this. Couldn't
>> we walk the list and re-fetch each module's magic block inside
>> this new function?
>
> Depends on how much we should try to cache to make that less expensive
> on repeated calls because we cannot unload libraries, but sure, I
> don't see why we could not that for each SQL function call to simplify
> the logic and the structures in place.
I want to say that 'cannot unload libraries' is a negative outcome of
the architecture. It would be better to invent something like
PG_unregister, allowing libraries to at least return a hook routine call
back to the system.
So, maybe it makes sense to design this feature with re-fetching
libraries, supposing it is already implemented somehow and elements of
the DynamicFileList may be removed.

--
regards, Andrei Lepikhov

On 12/12/24 08:36, Tom Lane wrote:
> Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
>> It makes sense. But I want to clarify that I avoided changing
>> PG_MODULE_MAGIC because the newly introduced structure has a totally
>> different purpose and usage logic: the struct is designed to check
>> compatibility, but module info isn't connected to the core version at
>> all: a single version of the code may be built for multiple PG versions.
>> At the same time, various versions of the same library may be usable
>> with the same core.
>
> Surely. But I don't see a need for two separately-looked-up
> physical structures. Seems to me it's sufficient to put the
> ABI-checking fields into a sub-struct within the magic block.
Okay, I've rewritten the patch to understand how it works. It seems to
work pretty well. I added separate fields for minor and major versions.

--
regards, Andrei Lepikhov

On Thu, Dec 12, 2024 at 3:41 PM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:

> On 12/12/24 08:36, Tom Lane wrote:
> > Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
> >> It makes sense. But I want to clarify that I avoided changing
> >> PG_MODULE_MAGIC because the newly introduced structure has a totally
> >> different purpose and usage logic: the struct is designed to check
> >> compatibility, but module info isn't connected to the core version at
> >> all: a single version of the code may be built for multiple PG versions.
> >> At the same time, various versions of the same library may be usable
> >> with the same core.
> >
> > Surely. But I don't see a need for two separately-looked-up
> > physical structures. Seems to me it's sufficient to put the
> > ABI-checking fields into a sub-struct within the magic block.
> Okay, I've rewritten the patch to understand how it works. It seems to
> work pretty well. I added separate fields for minor and major versions.
>
>
I am keenly interested in helping in this area; as you have mentioned, I've
done similar work using an extension.

Some thoughts/questions:

1. Do we need to latch onto the "magic" structure here? Have we considered
an opportunity to create a separate metadata slot that looks something like
`PG_MODULE_INFO(.version = ...)`. My impression of module magic was that it
should rather be populated during the build – to provide build-time
information. MODULE_INFO would be a rather informational section supplied
by the developer.

2. Any reasons to dictate MAJ.MIN format? With semantic versioning abound,
it's rather common to use MAJ.MIN.PATCH. There are also other extensions to
it (like pre-releases, builds, etc.). All of these indicate distinct
versions. The differences between them can be figured out using semver or
other parsers. Pure PL/pgSQL implementations of that exist [1].

3. In my work, I also introduced the concept of stable module identity – a
unique string (for example, UUID) that represents the identity of the
module even if its name is going to change. Admittedly, this is not _the
most common_ type of problem, but I anticipate it becoming more of an issue
with the growth of the extension ecosystem, potential name clashes, and
renamings. With this approach, developers assign this unique string to a
module once at the beginning and never change it. Have you considered this?

[1] https://github.com/bigsmoke/pg_text_semver

Hi,

On 2024-12-12 11:35:56 +0700, Andrei Lepikhov wrote:
> On 12/12/24 10:44, Michael Paquier wrote:
> > On Wed, Dec 11, 2024 at 10:39:38PM -0500, Tom Lane wrote:
> > > Michael Paquier <michael(at)paquier(dot)xyz> writes:
> > > > Presumably,
> > > > the extra tracking can be done in dfmgr.c with more fields added to
> > > > DynamicFileList to track the information involved.
> > >
> > > I wouldn't add any overhead to the normal case for this. Couldn't
> > > we walk the list and re-fetch each module's magic block inside
> > > this new function?
> >
> > Depends on how much we should try to cache to make that less expensive
> > on repeated calls because we cannot unload libraries, but sure, I
> > don't see why we could not that for each SQL function call to simplify
> > the logic and the structures in place.
> I want to say that 'cannot unload libraries' is a negative outcome of the
> architecture. It would be better to invent something like PG_unregister,
> allowing libraries to at least return a hook routine call back to the
> system.
> So, maybe it makes sense to design this feature with re-fetching libraries,
> supposing it is already implemented somehow and elements of the
> DynamicFileList may be removed.

I am quite certain we'll not support unloading libraries anytime soon. We used
to support it and it caused problems... Changing anything about how exactly
things are tracked in dfmgr.c will be the smallest part of supporting
unloading libraries again.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2024-12-12 11:35:56 +0700, Andrei Lepikhov wrote:
>> I want to say that 'cannot unload libraries' is a negative outcome of the
>> architecture. It would be better to invent something like PG_unregister,
>> allowing libraries to at least return a hook routine call back to the
>> system.

> I am quite certain we'll not support unloading libraries anytime soon. We used
> to support it and it caused problems... Changing anything about how exactly
> things are tracked in dfmgr.c will be the smallest part of supporting
> unloading libraries again.

Indeed. However, I don't see what that has to do with the current
discussion anyway. The proposed SRF would iterate through whatever
is in the DynamicFileList. What does it care whether there's a way
to add or remove entries?

regards, tom lane

On 12/12/24 21:02, Yurii Rashkovskii wrote:
> On Thu, Dec 12, 2024 at 3:41 PM Andrei Lepikhov <lepihov(at)gmail(dot)com
> <mailto:lepihov(at)gmail(dot)com>> wrote:
>
> On 12/12/24 08:36, Tom Lane wrote:
> > Andrei Lepikhov <lepihov(at)gmail(dot)com <mailto:lepihov(at)gmail(dot)com>>
> writes:
> >> It makes sense. But I want to clarify that I avoided changing
> >> PG_MODULE_MAGIC because the newly introduced structure has a totally
> >> different purpose and usage logic: the struct is designed to check
> >> compatibility, but module info isn't connected to the core
> version at
> >> all: a single version of the code may be built for multiple PG
> versions.
> >> At the same time, various versions of the same library may be usable
> >> with the same core.
> >
> > Surely.  But I don't see a need for two separately-looked-up
> > physical structures.  Seems to me it's sufficient to put the
> > ABI-checking fields into a sub-struct within the magic block.
> Okay, I've rewritten the patch to understand how it works. It seems to
> work pretty well. I added separate fields for minor and major versions.
>
>
> I am keenly interested in helping in this area; as you have mentioned,
> I've done similar work using an extension.
>
> Some thoughts/questions:
>
> 1. Do we need to latch onto the "magic" structure here? Have we
> considered an opportunity to create a separate metadata slot that looks
> something like `PG_MODULE_INFO(.version = ...)`. My impression of module
> magic was that it should rather be populated during the build – to
> provide build-time information. MODULE_INFO would be a rather
> informational section supplied by the developer.
It has already been debated above. I may agree with colleagues that
maintainer-provided information should be stored in the magic field to
reduce noise.
At the same time, we use a single code part to load all that data into
the DynamicFileList. That looks pretty well, isn't it?
>
> 2. Any reasons to dictate MAJ.MIN format? With semantic versioning
> abound, it's rather common to use MAJ.MIN.PATCH. There are also other
> extensions to it (like pre-releases, builds, etc.). All of these
> indicate distinct versions. The differences between them can be figured
> out using semver or other parsers. Pure PL/pgSQL implementations of that
> exist [1].
Okay, thanks; that's a good catch. I wonder how to follow these rules
with a static fixed-sized structure. I would like to read about any
suggestions and implementation examples.
>
> 3. In my work, I also introduced the concept of stable module identity –
> a unique string (for example, UUID) that represents the identity of the
> module even if its name is going to change. Admittedly, this is not _the
> most common_ type of problem, but I anticipate it becoming more of an
> issue with the growth of the extension ecosystem, potential name
> clashes, and renamings. With this approach, developers assign this
> unique string to a module once at the beginning and never change it.
> Have you considered this?
This option just needs some live examples. I think, if necessary, it
could be added later.

--
regards, Andrei Lepikhov

Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
> On 12/12/24 21:02, Yurii Rashkovskii wrote:
>> 2. Any reasons to dictate MAJ.MIN format? With semantic versioning
>> abound, it's rather common to use MAJ.MIN.PATCH.

> Okay, thanks; that's a good catch. I wonder how to follow these rules
> with a static fixed-sized structure. I would like to read about any
> suggestions and implementation examples.

There's nothing stopping a field of the magic block from being
a "const char *" pointer to a string literal.

regards, tom lane

On 12/13/24 10:17, Tom Lane wrote:
> Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
>> On 12/12/24 21:02, Yurii Rashkovskii wrote:
>>> 2. Any reasons to dictate MAJ.MIN format? With semantic versioning
>>> abound, it's rather common to use MAJ.MIN.PATCH.
>
>> Okay, thanks; that's a good catch. I wonder how to follow these rules
>> with a static fixed-sized structure. I would like to read about any
>> suggestions and implementation examples.
>
> There's nothing stopping a field of the magic block from being
> a "const char *" pointer to a string literal.
Ok, See v.2 in attachment.

--
regards, Andrei Lepikhov

On Dec 11, 2024, at 19:49, Euler Taveira <euler(at)eulerto(dot)com> wrote:

>> FWIW, Id like to have some more information in there, without commenting on
>> the specifics.
>
> +1 for the general idea.

Same.

> I received some reports like [1] related to wal2json
> that people wants to obtain the output plugin version. Since it is not installed
> via CREATE EXTENSION, it is not possible to detect what version is installed,
> hence, some tools cannot have some logic to probe the module version.

I’m all for additional metadata for native extensions, but I’d also like to draw attention to the “Future” section my proposal[1] to require that module-only extensions also include a control file and be loadable via CREATE EXTENSION (and proposed *_preload_extensions GUCs[2]). This would unify how all types of extensions are added to a database, and would include version information as for all other CREATE EXTENSION extensions.

Not a mutually-exclusive proposal, of course; I think it makes sense to have metadata included in the binary itself. Would be useful to compare against what CREATE EXTENSION thinks is the version and raising an error or warning when they diverge.

Best,

David

[1]: https://justatheory.com/2024/11/rfc-extension-packaging-lookup/#future-deprecate-load
[2]: https://justatheory.com/2024/11/rfc-extension-packaging-lookup/#extension-preloading

"David E. Wheeler" <david(at)justatheory(dot)com> writes:
> Not a mutually-exclusive proposal, of course; I think it makes sense to have metadata included in the binary itself. Would be useful to compare against what CREATE EXTENSION thinks is the version and raising an error or warning when they diverge.

How would that work for extensions where the C code is intentionally
supporting multiple versions of the SQL objects?

regards, tom lane

On Dec 23, 2024, at 15:17, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> How would that work for extensions where the C code is intentionally
> supporting multiple versions of the SQL objects?

I guess some people do that, eh? In that case it wouldn’t.

D

On 12/24/24 02:23, David E. Wheeler wrote:
> On Dec 11, 2024, at 19:49, Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
>>> FWIW, Id like to have some more information in there, without commenting on
>>> the specifics.
>>
>> +1 for the general idea.
>
> Same.
>
>> I received some reports like [1] related to wal2json
>> that people wants to obtain the output plugin version. Since it is not installed
>> via CREATE EXTENSION, it is not possible to detect what version is installed,
>> hence, some tools cannot have some logic to probe the module version.
>
> I’m all for additional metadata for native extensions, but I’d also like to draw attention to the “Future” section my proposal[1] to require that module-only extensions also include a control file and be loadable via CREATE EXTENSION (and proposed *_preload_extensions GUCs[2]). This would unify how all types of extensions are added to a database, and would include version information as for all other CREATE EXTENSION extensions.
Looking into the control file, I see that most parameters are
unnecessary for the library. Why do we have to maintain this file?
In my experience, extra features are usually designed as shared
libraries to 1) reduce complexity, 2) work across the overall cluster,
3) be dynamically loaded, 4) be hidden, and not waste the database with
any type of object. - remember, applications sometimes manage their data
through an API; databases and any objects inside may be created/moved
automatically, and we want to work in any database.
The 'CREATE EXTENSION' statement would have made sense if we had
register/unregister hook machinery. Without that, it seems it is just
about maintaining the library's version and comments locally in a
specific database.
It would be interesting to read about your real-life cases that caused
your proposal.

--
regards, Andrei Lepikhov

On Mon, Dec 16, 2024 at 12:02 PM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:

> On 12/13/24 10:17, Tom Lane wrote:
> > Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
> >> On 12/12/24 21:02, Yurii Rashkovskii wrote:
> >>> 2. Any reasons to dictate MAJ.MIN format? With semantic versioning
> >>> abound, it's rather common to use MAJ.MIN.PATCH.
> >
> >> Okay, thanks; that's a good catch. I wonder how to follow these rules
> >> with a static fixed-sized structure. I would like to read about any
> >> suggestions and implementation examples.
> >
> > There's nothing stopping a field of the magic block from being
> > a "const char *" pointer to a string literal.
> Ok, See v.2 in attachment.
>

I've reviewed the patch, and it is great that you support more flexible
versioning now. I am just wondering a bit about the case where
`minfo->name` can be `NULL` but `minfo->version` isn't, or where both are
`NULL` – should we skip any of these?

On 12/23/24 17:26, David E. Wheeler wrote:
> On Dec 23, 2024, at 15:17, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> How would that work for extensions where the C code is intentionally
>> supporting multiple versions of the SQL objects?
>
> I guess some people do that, eh? In that case it wouldn’t.

A function pointer rather than a version constant?

Or a function pointer, to be used if the version constant is null?

Regards,
-Chap

On Mon, Dec 23, 2024, at 8:49 PM, Andrei Lepikhov wrote:

> Looking into the control file, I see that most parameters are
> unnecessary for the library. Why do we have to maintain this file?

Most of the parameters apply to SQL extensions.

> The 'CREATE EXTENSION' statement would have made sense if we had
> register/unregister hook machinery. Without that, it seems it is just
> about maintaining the library's version and comments locally in a
> specific database.

Well, either way you have to load the extension, either CREATE EXTENSION to load an SQL extension (and any related shared modules), or LOAD or *_preload_libraries to load a shared module. I propose to add support for shared-module-only extensions to CREATE/UPDATE/DROP EXTENSION. It would then both insert the version info in the database (from the control file, at least), and load the shares module(s).

> It would be interesting to read about your real-life cases that caused
> your proposal.

They're in the first section of [1]. The desire to group all the files for an extension in a single directory led to a conflict with the exiting LOAD patterns, which in the final section of [1] I attempt to resolve by proposing a single way to manage *all* extensions, instead of the two separate patterns we have today.

Best,

David

[1]: https://justatheory.com/2024/11/rfc-extension-packaging-lookup/

On 12/27/24 01:26, David Wheeler wrote:
> On Mon, Dec 23, 2024, at 8:49 PM, Andrei Lepikhov wrote:
>
>> Looking into the control file, I see that most parameters are
>> unnecessary for the library. Why do we have to maintain this file?
> Well, either way you have to load the extension, either CREATE EXTENSION to load an SQL extension (and any related shared modules), or LOAD or *_preload_libraries to load a shared module. I propose to add support for shared-module-only extensions to CREATE/UPDATE/DROP EXTENSION. It would then both insert the version info in the database (from the control file, at least), and load the shares module(s).
I still can't get your point.
We intentionally wrote a library, not an extension. According to user
usage and upgrade patterns, it works across the whole instance and in
any database or locally in a single backend and ends its impact at the
end of its life.
Also, it doesn't maintain any object in the database and is managed by GUCs.
For example, my libraries add query tree transformations/path
recommendations to the planner. It doesn't depend on a database and
doesn't maintain DSM segments and users sometimes want to use it in
specific backends, not databases - in a backend dedicated to analytic
queries without extra overhead to backends, picked out for short
queries. For what reason do I need to add complexity and call 'CREATE
EXTENSION' here and add version info only in a specific database? Just
because of a formal one-directory structure?

--
regards, Andrei Lepikhov

On 12/24/24 10:42, Yurii Rashkovskii wrote:
> On Mon, Dec 16, 2024 at 12:02 PM Andrei Lepikhov <lepihov(at)gmail(dot)com
> I've reviewed the patch, and it is great that you support more flexible
> versioning now. I am just wondering a bit about the case where `minfo-
> >name` can be `NULL` but `minfo->version` isn't, or where both are
> `NULL` – should we skip any of these?
Depends. I wrote code that way so as not to restrict a maintainer by
initialising all the fields; remember, it may grow in the future.
But I am open to changing that logic. Do you have any specific rule on
which fields may be empty and that must be initialised? Do you think all
fields maintainer must fill with non-zero-length constants?

Also, I've added this patch to commitfest:
https://commitfest.postgresql.org/51/5465/

--
regards, Andrei Lepikhov

On Fri, Dec 27, 2024 at 8:34 AM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:

> On 12/24/24 10:42, Yurii Rashkovskii wrote:
> > On Mon, Dec 16, 2024 at 12:02 PM Andrei Lepikhov <lepihov(at)gmail(dot)com
> > I've reviewed the patch, and it is great that you support more flexible
> > versioning now. I am just wondering a bit about the case where `minfo-
> > >name` can be `NULL` but `minfo->version` isn't, or where both are
> > `NULL` – should we skip any of these?
> Depends. I wrote code that way so as not to restrict a maintainer by
> initialising all the fields; remember, it may grow in the future.
> But I am open to changing that logic. Do you have any specific rule on
> which fields may be empty and that must be initialised? Do you think all
> fields maintainer must fill with non-zero-length constants?

After more thinking, I'll concede that not doing anything about null
metadata is probably better – making the function always return the list of
modules, regardless of whether any metadata was supplied. It's beneficial
to be able to get the entire list of modules regardless of metadata.

The only other minor concern I have left is that some modules might have a
clashing name or may change the name during the extension's lifetime
(happened to some of my early work). Providing a permanent identifier and a
human-digestible identifier may be worth it.

On Dec 26, 2024, at 20:09, Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:

> We intentionally wrote a library, not an extension. According to user usage and upgrade patterns, it works across the whole instance and in any database or locally in a single backend and ends its impact at the end of its life.

The same is true for the shared libraries included in many extensions. A shared library is just an extension that’s available in all databases and has no associated SQL interface.

> Also, it doesn't maintain any object in the database and is managed by GUCs.

Sure, but this is just a semantic argument. The Postgres developers get to decide what terms mean. I’m I argue it can be worthwhile to merge the idea of a library into extensions.

> For example, my libraries add query tree transformations/path recommendations to the planner. It doesn't depend on a database and doesn't maintain DSM segments and users sometimes want to use it in specific backends, not databases - in a backend dedicated to analytic queries without extra overhead to backends, picked out for short queries. For what reason do I need to add complexity and call 'CREATE EXTENSION' here and add version info only in a specific database? Just because of a formal one-directory structure?

Perhaps shared-library only extensions are not limited to a single database.

Best,

David

Hi, Andrei!

On Mon, Dec 16, 2024 at 7:02 AM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:
>ly
> On 12/13/24 10:17, Tom Lane wrote:
> > Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
> >> On 12/12/24 21:02, Yurii Rashkovskii wrote:
> >>> 2. Any reasons to dictate MAJ.MIN format? With semantic versioning
> >>> abound, it's rather common to use MAJ.MIN.PATCH.
> >
> >> Okay, thanks; that's a good catch. I wonder how to follow these rules
> >> with a static fixed-sized structure. I would like to read about any
> >> suggestions and implementation examples.
> >
> > There's nothing stopping a field of the magic block from being
> > a "const char *" pointer to a string literal.
> Ok, See v.2 in attachment.

Generally, the patch looks good to me. I have couple of questions.

1) Is it intended to switch all in-core libraries to use PG_MODULE_MAGIC_EXT()?
2) Once we have module version information, it looks natural to
specify the required version for dependant objects, e.g. SQL-funcions
implemented in shared libraries. For instance,
CREATE FUNCTION ... AS 'MODULE_PATHNAME' LANGUAGE C module_version >= '1.0';
For this, and probably other purposes, it's desirable for version to
be something comparable at SQL level. Should we add some builtin
analogue of pg_text_semver?

------
Regards,
Alexander Korotkov
Supabase

On Mon, Feb 17, 2025 at 03:41:56AM +0200, Alexander Korotkov wrote:
> 1) Is it intended to switch all in-core libraries to use PG_MODULE_MAGIC_EXT()?
> 2) Once we have module version information, it looks natural to
> specify the required version for dependant objects, e.g. SQL-funcions
> implemented in shared libraries. For instance,
> CREATE FUNCTION ... AS 'MODULE_PATHNAME' LANGUAGE C module_version >= '1.0';
> For this, and probably other purposes, it's desirable for version to
> be something comparable at SQL level. Should we add some builtin
> analogue of pg_text_semver?

I see that this is just a way for extensions to map to some data
statically stored in the modules themselves based on what I can see at
[1]. Why not.

+ bool isnull[3] = {0,0,0};

Could be a simpler {0}.

-PG_MODULE_MAGIC;
+PG_MODULE_MAGIC_EXT(
+ .name = "auto_explain",
+ .version = "1.0.0"
+);

It does not make sense to me to stick that into into of the contrib
modules officially supported just for the sake of the API. I'd
suggest to switch in one of the modules of src/test/modules/ that are
loaded with shared_preload_libraries. A second thing I would suggest
to check is a SQL call with a library loaded via SQL with a LOAD.
test_oat_hooks is already LOAD'ed in a couple of scripts, for example.
For the shared_preload_libraries can, you could choose anything to
prove your point with tests.

+Datum
+module_info(PG_FUNCTION_ARGS)

This should use a "pg_" prefix, should use a plural term as it is a
SRF returning information about all the modules loaded. Perhaps just
name it to pg_get_modules() and also map it to a new system view?

Some problems with `git diff --check\` showing up here.

No documentation provided.

[1] https://www.postgresql.org/message-id/[email protected]
--
Michael

On 17/2/2025 02:41, Alexander Korotkov wrote:
> On Mon, Dec 16, 2024 at 7:02 AM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:
>> On 12/13/24 10:17, Tom Lane wrote:
>>> There's nothing stopping a field of the magic block from being
>>> a "const char *" pointer to a string literal.
>> Ok, See v.2 in attachment.
>
> Generally, the patch looks good to me. I have couple of questions.
>
> 1) Is it intended to switch all in-core libraries to use PG_MODULE_MAGIC_EXT()?
I haven't such intention. Just wanted to demonstrate how it might work.

> 2) Once we have module version information, it looks natural to
> specify the required version for dependant objects, e.g. SQL-funcions
> implemented in shared libraries. For instance,
> CREATE FUNCTION ... AS 'MODULE_PATHNAME' LANGUAGE C module_version >= '1.0';
Just to be clear. You want this stuff to let the core manage situations
of stale binaries and throw an error like the following:
"No function matches the given name, argument types and module version"
Do I understand you correctly?
It may make sense, but I can't figure out a use case. Could you describe
at least one example?

--
regards, Andrei Lepikhov

On 17/2/2025 04:00, Michael Paquier wrote:
> On Mon, Feb 17, 2025 at 03:41:56AM +0200, Alexander Korotkov wrote:
>> 1) Is it intended to switch all in-core libraries to use PG_MODULE_MAGIC_EXT()?
>> 2) Once we have module version information, it looks natural to
>> specify the required version for dependant objects, e.g. SQL-funcions
>> implemented in shared libraries. For instance,
>> CREATE FUNCTION ... AS 'MODULE_PATHNAME' LANGUAGE C module_version >= '1.0';
>> For this, and probably other purposes, it's desirable for version to
>> be something comparable at SQL level. Should we add some builtin
>> analogue of pg_text_semver?
>
> I see that this is just a way for extensions to map to some data
> statically stored in the modules themselves based on what I can see at
> [1]. Why not.
>
> + bool isnull[3] = {0,0,0};
>
> Could be a simpler {0}.
Done
>
> -PG_MODULE_MAGIC;
> +PG_MODULE_MAGIC_EXT(
> + .name = "auto_explain",
> + .version = "1.0.0"
> +);
>
> It does not make sense to me to stick that into into of the contrib
> modules officially supported just for the sake of the API. I'd
Done
> suggest to switch in one of the modules of src/test/modules/ that are
> loaded with shared_preload_libraries. A second thing I would suggest
> to check is a SQL call with a library loaded via SQL with a LOAD.
> test_oat_hooks is already LOAD'ed in a couple of scripts, for example.
> For the shared_preload_libraries can, you could choose anything to
> prove your point with tests.
Done
>
> +Datum
> +module_info(PG_FUNCTION_ARGS)
>
> This should use a "pg_" prefix, should use a plural term as it is a
> SRF returning information about all the modules loaded. Perhaps just
> name it to pg_get_modules() and also map it to a new system view?
Sure, done.
>
> Some problems with `git diff --check\` showing up here.
Done
>
> No documentation provided.
Ok, I haven't been sure this idea has a chance to be committed. I will
introduce the docs in the next version.

--
regards, Andrei Lepikhov

On 2/3/2025 20:35, Andrei Lepikhov wrote:
> On 17/2/2025 04:00, Michael Paquier wrote:
>> No documentation provided.
> Ok, I haven't been sure this idea has a chance to be committed. I will
> introduce the docs in the next version.
This is a new version with bug fixes. Also, use TAP tests instead of
regression tests. Still, no documentation is included.

--
regards, Andrei Lepikhov

On 7/3/2025 16:56, Andrei Lepikhov wrote:
> On 2/3/2025 20:35, Andrei Lepikhov wrote:
>> On 17/2/2025 04:00, Michael Paquier wrote:
>>> No documentation provided.
>> Ok, I haven't been sure this idea has a chance to be committed. I will
>> introduce the docs in the next version.
> This is a new version with bug fixes. Also, use TAP tests instead of
> regression tests. Still, no documentation is included.
>
v5 contains documentation entries for the pg_get_modules function and
the PG_MODULE_MAGIC_EXT macro. Also, commit comment is smoothed a little.

--
regards, Andrei Lepikhov

I spent awhile reviewing the v5 patch, and here's a proposed v6.
Some notes:

* I didn't like depending on offsetof(Pg_magic_struct, module_extra)
to determine which parts of the struct are checked for compatibility.
It just seems way too easy to break that with careless insertion
of new fields, and such breakage might not cause obvious failures.
I think the right thing is to break out the ABI-checking fields as
their own sub-struct, rather than breaking out the new fields as a
sub-struct.

* I renamed the inquiry function to pg_get_loaded_modules, since
it only works on loaded modules but that's hardly clear from the
previous name.

* It is not clear to me what permission restrictions we should put
on pg_get_loaded_modules, but it is clear that "none" is the wrong
answer. In particular, exposing the full file path of loaded modules
is against our rules: unprivileged users are not supposed to be able
to learn anything about the filesystem underneath the server. (This
is why for instance an unprivileged user can't read the data_directory
GUC.) In the attached I made the library path read as NULL unless the
user has pg_read_server_files, but I'm not attached to that specific
solution. One thing not to like is that it's very likely that you'd
just get a row of NULLs and no useful info about a module at all.
Another idea perhaps could be to strip off the directory path and
maybe the filename extension if the user doesn't have privilege.
Or we could remove the internal permission check and instead gate
access to the function altogether with grantable EXECUTE privilege.
(This might be the right answer, since it's not clear that Joe
Unprivileged User should be able to know what modules are loaded; some
of them might have security implications.) In any case, requiring
pg_read_server_files feels a little too strong, but I don't see an
alternative role I like better. The EXECUTE-privilege answer would at
least let installations adjust the function's availability to their
liking.

* I didn't like anything about the test setup. Making test_misc
dependent on other modules is a recipe for confusion, and perhaps for
failures in parallel builds. (Yes, I see somebody already made it
depend on injection_points. But doubling down on a bad idea doesn't
make it less bad.) Also, the test would fail completely in an
installation that came with any preloaded modules, which hardly seems
like an improbable future situation. I think we need to restrict what
modules we're looking at with a WHERE clause to prevent that. After
some thought I went back to the upthread idea of just having
auto_explain as a test case.

Still TBD:

* I'm not happy with putting pg_get_loaded_modules into dfmgr.c.
It feels like the wrong layer to have a SQL-callable function,
and the large expansion in its #include list is evidence that we're
adding functionality that doesn't belong there. But I'm not quite
sure where to put it instead. Also, the naive way to do that would
require exporting DynamicFileList which doesn't feel nice either.
Maybe we could make dfmgr.c export some sort of iterator function?

* Should we convert our existing modules to use PG_MODULE_MAGIC_EXT?
I'm mildly in favor of that, but I think we'd need some automated way
to manage their version strings, and I don't know what that ought to
look like. Maybe it'd be enough to make all the in-core modules use
PG_VERSION as their version string, but I think that might put a dent
in the idea of the version strings following semantic versioning
rules.

regards, tom lane

On 3/22/25 23:49, Tom Lane wrote:
> I spent awhile reviewing the v5 patch, and here's a proposed v6.
> Some notes:
>
> * I didn't like depending on offsetof(Pg_magic_struct, module_extra)
> to determine which parts of the struct are checked for compatibility.
> It just seems way too easy to break that with careless insertion
> of new fields, and such breakage might not cause obvious failures.
> I think the right thing is to break out the ABI-checking fields as
> their own sub-struct, rather than breaking out the new fields as a
> sub-struct.
Agree. It is a clear approach. I like it.
>
> * I renamed the inquiry function to pg_get_loaded_modules, since
> it only works on loaded modules but that's hardly clear from the
> previous name.
+1
>
> * It is not clear to me what permission restrictions we should put
> on pg_get_loaded_modules, ...
I vote for the idea of stripping the full path to just a filename. My
initial use cases were:
1. User reports the issue and need to provide me all loaded modules at
the moment of query execution. Higher privileges needs administrative
procedures that is a long way and not all the time possible.
2. A module needs to detect another loaded module - it is not a frequent
case so far, but concurrency on queryId with pg_stat_statements is at
least one of my examples happening sometimes.

Also, permissions here should be in agreement with permissions on
pg_available_extensions(), right?

>
> * I didn't like anything about the test setup. ...
Ok, thanks. I just played with alternatives.
>
> Still TBD:
>
> * I'm not happy with putting pg_get_loaded_modules into dfmgr.c.
> It feels like the wrong layer to have a SQL-callable function,
> and the large expansion in its #include list is evidence that we're
> adding functionality that doesn't belong there. But I'm not quite
> sure where to put it instead. Also, the naive way to do that would
> require exporting DynamicFileList which doesn't feel nice either.
> Maybe we could make dfmgr.c export some sort of iterator function?
I just attempted to reduce number of exported objects here. If it is ok
to introduce an iterator, the pg_get_loaded_modules() may live in
extension.c
>
> * Should we convert our existing modules to use PG_MODULE_MAGIC_EXT?
> I'm mildly in favor of that, but I think we'd need some automated way
> to manage their version strings, and I don't know what that ought to
> look like. Maybe it'd be enough to make all the in-core modules use
> PG_VERSION as their version string, but I think that might put a dent
> in the idea of the version strings following semantic versioning
> rules.
Yes, additional burden to bump version string was a stopper for me to
propose such a brave idea.

--
regards, Andrei Lepikhov

Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
> On 3/22/25 23:49, Tom Lane wrote:
>> * It is not clear to me what permission restrictions we should put
>> on pg_get_loaded_modules, ...

> I vote for the idea of stripping the full path to just a filename.

Works for me. v7 attached does it that way.

>> * I'm not happy with putting pg_get_loaded_modules into dfmgr.c.

> I just attempted to reduce number of exported objects here. If it is ok
> to introduce an iterator, the pg_get_loaded_modules() may live in
> extension.c

Yeah, I like that better than leaving it in dfmgr.c, so done that way.
The iterator functions also provide some cover for dealing with
on-the-fly changes of the file list, if we ever need that.

I converted pg_get_loaded_modules to run just once and deliver its
results in a tuplestore. That's partly because the adjacent SRFs
in extension.c work like that, but mostly because it removes the
hazard of the file list changing mid-run.

>> * Should we convert our existing modules to use PG_MODULE_MAGIC_EXT?
>> I'm mildly in favor of that, but I think we'd need some automated way
>> to manage their version strings, and I don't know what that ought to
>> look like. Maybe it'd be enough to make all the in-core modules use
>> PG_VERSION as their version string, but I think that might put a dent
>> in the idea of the version strings following semantic versioning
>> rules.

> Yes, additional burden to bump version string was a stopper for me to
> propose such a brave idea.

After sleeping on it, I think we really ought to do that, so 0002
attached does so.

I think this version is ready to commit, if there are not objections
to the decisions mentioned above.

regards, tom lane

On 3/23/25 20:10, Tom Lane wrote:
> Andrei Lepikhov <lepihov(at)gmail(dot)com> writes:
>> On 3/22/25 23:49, Tom Lane wrote:
>>> * It is not clear to me what permission restrictions we should put
>>> on pg_get_loaded_modules, ...
>
>> I vote for the idea of stripping the full path to just a filename.
>
> Works for me. v7 attached does it that way.
Thanks, you've done almost all the job.
>
>>> * I'm not happy with putting pg_get_loaded_modules into dfmgr.c.
>
>> I just attempted to reduce number of exported objects here. If it is ok
>> to introduce an iterator, the pg_get_loaded_modules() may live in
>> extension.c
>
> Yeah, I like that better than leaving it in dfmgr.c, so done that way.
> The iterator functions also provide some cover for dealing with
> on-the-fly changes of the file list, if we ever need that.
It also gives extension developers a tool to detect conflicting modules
any time we need it. More elegant than the SerializeLibraryState().
>
> I converted pg_get_loaded_modules to run just once and deliver its
> results in a tuplestore. That's partly because the adjacent SRFs
> in extension.c work like that, but mostly because it removes the
> hazard of the file list changing mid-run.
Ok.
>> Yes, additional burden to bump version string was a stopper for me to
>> propose such a brave idea.
>
> After sleeping on it, I think we really ought to do that, so 0002
> attached does so.
With the concept of the PG_VERSION string as a version, it looks more
meaningful than I've thought before.

Patch 0001 is ready to commit for me.
Patch 0002 I just checked on the errors in module names. That's more I
can do here? ;) Seems good, no errors found.

--
regards, Andrei Lepikhov

On Sun, Mar 23, 2025 at 3:10 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I think this version is ready to commit, if there are not objections
> to the decisions mentioned above.

It looks reasonable to me. I am a bit worried that using PG_VERSION as
the version string is going to feel like the wrong thing at some
stage, but I can't really say why, and I think it's better to do
something now and maybe have to revise it later than to do nothing now
and hope that we come up with a brilliant idea at some point in the
future.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> It looks reasonable to me. I am a bit worried that using PG_VERSION as
> the version string is going to feel like the wrong thing at some
> stage, but I can't really say why, and I think it's better to do
> something now and maybe have to revise it later than to do nothing now
> and hope that we come up with a brilliant idea at some point in the
> future.

Agreed. I think something is clearly better than nothing here, and
PG_VERSION has the huge advantage that we need no new mechanism to
maintain it. (A version identifier that isn't updated when it needs
to be is worse than no identifier, IMO.)

If somebody thinks of a better idea and is willing to do the legwork
to make it happen, we can surely change to something else later on.
Or invent another field with different semantics, or whatever.

regards, tom lane

On Mon, Mar 24, 2025 at 11:54 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> If somebody thinks of a better idea and is willing to do the legwork
> to make it happen, we can surely change to something else later on.
> Or invent another field with different semantics, or whatever.

Yeah, my thoughts exactly.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Mar 24, 2025, at 12:54 PM, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > It looks reasonable to me. I am a bit worried that using PG_VERSION as
> > the version string is going to feel like the wrong thing at some
> > stage, but I can't really say why, and I think it's better to do
> > something now and maybe have to revise it later than to do nothing now
> > and hope that we come up with a brilliant idea at some point in the
> > future.
>
> Agreed. I think something is clearly better than nothing here, and
> PG_VERSION has the huge advantage that we need no new mechanism to
> maintain it. (A version identifier that isn't updated when it needs
> to be is worse than no identifier, IMO.)

Agreed. My only concern is that people can confuse this version with the one
available in pg_extension or pg_available_extension* functions.

> If somebody thinks of a better idea and is willing to do the legwork
> to make it happen, we can surely change to something else later on.
> Or invent another field with different semantics, or whatever.

I think those modules without control file, it is natural to use PG_VERSION.
However, I'm concerned that users can confuse the version if we provide
PG_VERSION as version and the extension catalog says something different.

postgres=# select * from pg_available_extensions where name = 'plperl';
name | default_version | installed_version | comment
--------+-----------------+-------------------+-----------------------------
plperl | 1.0 | | PL/Perl procedural language
(1 row)

postgres=# load 'plperl';
LOAD
postgres=# select * from pg_get_loaded_modules();
module_name | version | file_name
-------------+---------+-----------
plperl | 18devel | plperl.so
(1 row)

Maybe a note into default_version [1] is sufficient to clarify or a mechanism
to grab the information from control file and expose it as a macro. (I attached
an idea to accomplish this goal although it lacks meson support.) Thoughts?

I played with it a bit and it seems good to go.

postgres=# select version();
version
----------------------------------------------------------------------------------------------
PostgreSQL 18devel on x86_64-pc-linux-gnu, compiled by gcc (Debian 12.2.0-14) 12.2.0, 64-bit
(1 row)

postgres=# select * from pg_get_loaded_modules();
module_name | version | file_name
-------------+---------+-----------
(0 rows)

postgres=# load 'wal2json';
LOAD
postgres=# select * from pg_get_loaded_modules();
module_name | version | file_name
-------------+---------+-------------
wal2json | 2.6 | wal2json.so
(1 row)

Code:

diff --git a/wal2json.c b/wal2json.c
index 0c6295d..1f439be 100644
--- a/wal2json.c
+++ b/wal2json.c
@@ -40,7 +40,14 @@
#define WAL2JSON_FORMAT_VERSION 2
#define WAL2JSON_FORMAT_MIN_VERSION 1

+#if PG_VERSION_NUM >= 180000
+PG_MODULE_MAGIC_EXT(
+ .name = "wal2json",
+ .version = WAL2JSON_VERSION
+);
+#else
PG_MODULE_MAGIC;
+#endif

[1] https://www.postgresql.org/docs/current/extend-extensions.html

--
Euler Taveira
EDB https://www.enterprisedb.com/

"Euler Taveira" <euler(at)eulerto(dot)com> writes:
> I think those modules without control file, it is natural to use PG_VERSION.
> However, I'm concerned that users can confuse the version if we provide
> PG_VERSION as version and the extension catalog says something different.

Maybe, but the values will be sufficiently different that I don't
think the confusion will last long. Anyway I don't want the version
in an extension's module to mean something totally different than
the version in a non-extension module. I could possibly get behind
setting version = PG_VERSION and having another field "ext_version"
or such that shows the expected current extension version if the
module belongs to an extension. I'm not really convinced it's worth
the trouble, though.

regards, tom lane

Hearing no further discussion, I've pushed this.

regards, tom lane

Hi Tom,

This recent patch is great but causes a small problem. It mixes designated
and non-designated initializers, specifically in `PG_MODULE_MAGIC_DATA(0)`.

While this is permissible in C, when imported in C++ code (in extern "C"),
it causes GCC to emit an error: `either all initializer clauses should be
designated or none of them should be`.
In Clang, this is a warning: `mixture of designated and non-designated
initializers in the same initializer list is a C99 extension`

I understand that this won't affect C extensions, it causes a need for an
unnecessary workaround for C++ extensions. C++ extensions are, of course,
not first-class-supported, but they are documented as essentially feasible
(and I am exercising this successfully)

Can we amend `PG_MODULE_MAGIC_DATA` to use designated initializers
exclusively? This way there will be no special-casing for C++, yet it will
provide relief for its users.

On Wed, Mar 26, 2025 at 8:15 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Hearing no further discussion, I've pushed this.
>
> regards, tom lane
>

Yurii Rashkovskii <yrashk(at)omnigres(dot)com> writes:
> This recent patch is great but causes a small problem. It mixes designated
> and non-designated initializers, specifically in `PG_MODULE_MAGIC_DATA(0)`.

Ugh. I felt a bit itchy about that, but my compiler wasn't
complaining...

Can you propose a specific change to clean it up? I wanted to write
just "PG_MODULE_MAGIC_DATA()", but I'm not sure that's valid C either.

regards, tom lane

On Wed, Mar 26, 2025 at 7:45 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> > This recent patch is great but causes a small problem. It mixes
> designated
> > and non-designated initializers, specifically in
> `PG_MODULE_MAGIC_DATA(0)`.
>
> Ugh. I felt a bit itchy about that, but my compiler wasn't
> complaining...
>

That's because this is valid in C99/C11; it's just not valid in C++. That
said, I think it's confusing and error-prone.

>
> Can you propose a specific change to clean it up? I wanted to write
> just "PG_MODULE_MAGIC_DATA()", but I'm not sure that's valid C either.
>
> I was thinking about passing `.name = NULL, .version = NULL` instead of
`0`—do you have any reservations about this?

Yurii

Yurii Rashkovskii <yrashk(at)omnigres(dot)com> writes:
>> Can you propose a specific change to clean it up? I wanted to write
>> just "PG_MODULE_MAGIC_DATA()", but I'm not sure that's valid C either.

> I was thinking about passing `.name = NULL, .version = NULL` instead of
> `0`—do you have any reservations about this?

If we're going that way, I'd minimize it to just ".name = NULL".

regards, tom lane

On Wed, Mar 26, 2025 at 8:17 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Yurii Rashkovskii <yrashk(at)omnigres(dot)com> writes:
> >> Can you propose a specific change to clean it up? I wanted to write
> >> just "PG_MODULE_MAGIC_DATA()", but I'm not sure that's valid C either.
>
> > I was thinking about passing `.name = NULL, .version = NULL` instead of
> > `0`—do you have any reservations about this?
>
> If we're going that way, I'd minimize it to just ".name = NULL".
>

Would something like this work?

Yurii Rashkovskii <yrashk(at)omnigres(dot)com> writes:
> Would something like this work?

Works for me; pushed.

regards, tom lane