proposal - get_extension_version function
Hi
I try to write a safeguard check that ensures the expected extension
version for an extension library.
Some like
const char *expected_extversion = "2.5";
...
extoid = getExtensionOfObject(ProcedureRelationId, fcinfo->flinfo->fn_oid));
extversion = get_extension_version(extoid);
if (strcmp(expected_extversion, extversion) != 0)
elog(ERROR, "extension \"%s\" needs \"ALTER EXTENSION %s UPDATE\",
get_extension_name(extversion),
get_extension_name(extversion)))
Currently the extension version is not simply readable - I need to read
directly from the table.
Notes, comments?
Regards
Pavel
Pavel Stehule <pavel.stehule@gmail.com> writes:
I try to write a safeguard check that ensures the expected extension
version for an extension library.
This is a bad idea. How will you do extension upgrades, if the new .so
won't run till you apply the extension upgrade script but the old .so
malfunctions as soon as you do? You need to make the C code as forgiving
as possible, not as unforgiving as possible.
If you have C-level ABI changes you need to make, the usual fix is to
include some sort of version number in the C name of each individual
function you've changed, so that calls made with the old or the new SQL
definition will be routed to the right place. There are multiple
examples of this in contrib/.
regards, tom lane
On Wed, Mar 8, 2023 at 10:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
This is a bad idea. How will you do extension upgrades, if the new .so
won't run till you apply the extension upgrade script but the old .so
malfunctions as soon as you do?
Which upgrade paths allow you to have an old .so with a new version
number? I didn't realize that was an issue.
--Jacob
st 8. 3. 2023 v 20:04 odesílatel Jacob Champion <jchampion@timescale.com>
napsal:
On Wed, Mar 8, 2023 at 10:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
This is a bad idea. How will you do extension upgrades, if the new .so
won't run till you apply the extension upgrade script but the old .so
malfunctions as soon as you do?Which upgrade paths allow you to have an old .so with a new version
number? I didn't realize that was an issue.
installation from rpm or deb packages
Show quoted text
--Jacob
Jacob Champion <jchampion@timescale.com> writes:
On Wed, Mar 8, 2023 at 10:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
This is a bad idea. How will you do extension upgrades, if the new .so
won't run till you apply the extension upgrade script but the old .so
malfunctions as soon as you do?
Which upgrade paths allow you to have an old .so with a new version
number? I didn't realize that was an issue.
More usually, it's the other way around: new .so but SQL objects not
upgraded yet. That's typical in a pg_upgrade to a new major version,
where the new installation may have a newer extension .so than the
old one did. You can't run ALTER EXTENSION UPGRADE if the new .so
refuses to load with the old SQL objects ... which AFAICS is exactly
what Pavel's sketch would do.
If you have old .so and new SQL objects, it's likely that at least
some of those new objects won't work --- but it's good to not break
any more functionality than you have to. That's why I suggest
managing the compatibility checks on a per-function level rather
than trying to have an overall version check.
regards, tom lane
st 8. 3. 2023 v 19:49 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
Pavel Stehule <pavel.stehule@gmail.com> writes:
I try to write a safeguard check that ensures the expected extension
version for an extension library.This is a bad idea. How will you do extension upgrades, if the new .so
won't run till you apply the extension upgrade script but the old .so
malfunctions as soon as you do? You need to make the C code as forgiving
as possible, not as unforgiving as possible.
This method doesn't break updates. It allows any registration, just
doesn't allow execution with unsynced SQL API.
If you have C-level ABI changes you need to make, the usual fix is to
include some sort of version number in the C name of each individual
function you've changed, so that calls made with the old or the new SQL
definition will be routed to the right place. There are multiple
examples of this in contrib/.
In my extensions like plpgsql_check I don't want to promise compatible ABI.
I support PostgreSQL 10 .. 16, and I really don't try to multiply code for
any historic input/output.
Show quoted text
regards, tom lane
st 8. 3. 2023 v 20:17 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
Jacob Champion <jchampion@timescale.com> writes:
On Wed, Mar 8, 2023 at 10:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
This is a bad idea. How will you do extension upgrades, if the new .so
won't run till you apply the extension upgrade script but the old .so
malfunctions as soon as you do?Which upgrade paths allow you to have an old .so with a new version
number? I didn't realize that was an issue.More usually, it's the other way around: new .so but SQL objects not
upgraded yet. That's typical in a pg_upgrade to a new major version,
where the new installation may have a newer extension .so than the
old one did. You can't run ALTER EXTENSION UPGRADE if the new .so
refuses to load with the old SQL objects ... which AFAICS is exactly
what Pavel's sketch would do.If you have old .so and new SQL objects, it's likely that at least
some of those new objects won't work --- but it's good to not break
any more functionality than you have to. That's why I suggest
managing the compatibility checks on a per-function level rather
than trying to have an overall version check.
There is agreement - I call this check from functions.
https://github.com/okbob/plpgsql_check/commit/b0970ff319256207ffe5ba5f18b2a7476c7136f9
Regards
Pavel
Show quoted text
regards, tom lane
On Wed, Mar 8, 2023 at 11:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jacob Champion <jchampion@timescale.com> writes:
On Wed, Mar 8, 2023 at 10:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
This is a bad idea. How will you do extension upgrades, if the new .so
won't run till you apply the extension upgrade script but the old .so
malfunctions as soon as you do?Which upgrade paths allow you to have an old .so with a new version
number? I didn't realize that was an issue.More usually, it's the other way around: new .so but SQL objects not
upgraded yet. That's typical in a pg_upgrade to a new major version,
where the new installation may have a newer extension .so than the
old one did.
That's the opposite case though; I think the expectation of backwards
compatibility from C to SQL is very different from (infinite?)
forwards compatibility from C to SQL.
If you have old .so and new SQL objects, it's likely that at least
some of those new objects won't work --- but it's good to not break
any more functionality than you have to.
To me it doesn't seem like a partial break is safer than refusing to
execute in the face of old-C-and-new-SQL -- assuming it's safe at all?
A bailout seems pretty reasonable in that case.
Thanks,
--Jacob
On Wed, Mar 8, 2023 at 11:11 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
installation from rpm or deb packages
Right, but I thought the safe order for a downgrade was to issue the
SQL downgrade first (thus putting the system back into the
post-upgrade state), and only then replacing the packages with prior
versions.
--Jacob
On Wed, Mar 8, 2023 at 11:22 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
There is agreement - I call this check from functions.
I think pg_auto_failover does this too, or at least used to.
Timescale does strict compatibility checks as well. It's not entirely
comparable to your implementation, though.
--Jacob
Jacob Champion <jchampion@timescale.com> writes:
On Wed, Mar 8, 2023 at 11:11 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
installation from rpm or deb packages
Right, but I thought the safe order for a downgrade was to issue the
SQL downgrade first (thus putting the system back into the
post-upgrade state), and only then replacing the packages with prior
versions.
Pavel's proposed check would break that too. There's going to be some
interval where the SQL definitions are not in sync with the .so version,
so you really want the .so to support at least two versions' worth of
SQL objects.
regards, tom lane
On Wed, Mar 8, 2023 at 1:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Pavel's proposed check would break that too. There's going to be some
interval where the SQL definitions are not in sync with the .so version,
so you really want the .so to support at least two versions' worth of
SQL objects.
I think we're in agreement that the extension must be able to load
with SQL version X and binary version X+1. (Pavel too, if I'm reading
the argument correctly -- the proposal is to gate execution paths, not
init time. And Pavel's not the only one implementing that today.)
What I'm trying to pin down is the project's position on the reverse
-- binary version X and SQL version X+1 -- because that seems
generally unmaintainable, and I don't understand why an author would
pay that tax if they could just avoid it by bailing out entirely. (If
an author wants to allow that, great, but does everyone have to?)
--Jacob
Jacob Champion <jchampion@timescale.com> writes:
What I'm trying to pin down is the project's position on the reverse
-- binary version X and SQL version X+1 -- because that seems
generally unmaintainable, and I don't understand why an author would
pay that tax if they could just avoid it by bailing out entirely. (If
an author wants to allow that, great, but does everyone have to?)
Hard to say. Our experience with the standard contrib modules is that
it really isn't much additional trouble; but perhaps more-complex modules
would have more interdependencies between functions. In any case,
I fail to see the need for basing things on a catalog lookup rather
than embedding API version numbers in relevant C symbols.
regards, tom lane
st 8. 3. 2023 v 23:43 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
Jacob Champion <jchampion@timescale.com> writes:
What I'm trying to pin down is the project's position on the reverse
-- binary version X and SQL version X+1 -- because that seems
generally unmaintainable, and I don't understand why an author would
pay that tax if they could just avoid it by bailing out entirely. (If
an author wants to allow that, great, but does everyone have to?)Hard to say. Our experience with the standard contrib modules is that
it really isn't much additional trouble; but perhaps more-complex modules
would have more interdependencies between functions. In any case,
I fail to see the need for basing things on a catalog lookup rather
than embedding API version numbers in relevant C symbols.
How can you check it? There is not any callback now.
Regards
Pavel
Show quoted text
regards, tom lane
Hi
st 8. 3. 2023 v 17:58 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:
Hi
I try to write a safeguard check that ensures the expected extension
version for an extension library.Some like
const char *expected_extversion = "2.5";
...
extoid = getExtensionOfObject(ProcedureRelationId,
fcinfo->flinfo->fn_oid));
extversion = get_extension_version(extoid);
if (strcmp(expected_extversion, extversion) != 0)
elog(ERROR, "extension \"%s\" needs \"ALTER EXTENSION %s UPDATE\",
get_extension_name(extversion),
get_extension_name(extversion)))Currently the extension version is not simply readable - I need to read
directly from the table.Notes, comments?
attached patch
Regards
Pavel
Show quoted text
Regards
Pavel
Attachments:
0001-get_extension_version-given-an-extension-OID-look-up.patchtext/x-patch; charset=US-ASCII; name=0001-get_extension_version-given-an-extension-OID-look-up.patchDownload
From b12a43d885af4311dbb61684d65b6f5fc41b60d2 Mon Sep 17 00:00:00 2001
From: "okbob@github.com" <pavel.stehule@gmail.com>
Date: Sat, 11 Mar 2023 05:04:28 +0100
Subject: [PATCH] get_extension_version - given an extension OID, look up the
version
Returns a palloc'd string, or NULL if no such extension.
---
src/backend/commands/extension.c | 50 ++++++++++++++++++++++++++++++++
src/include/commands/extension.h | 1 +
2 files changed, 51 insertions(+)
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 02ff4a9a7f..ea347750fe 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -256,6 +256,56 @@ get_extension_schema(Oid ext_oid)
return result;
}
+/*
+ * get_extension_version - given an extension OID, look up the version
+ *
+ * Returns a palloc'd string, or NULL if no such extension.
+ */
+char *
+get_extension_version(Oid ext_oid)
+{
+ char *result;
+ Relation rel;
+ SysScanDesc scandesc;
+ HeapTuple tuple;
+ ScanKeyData entry[1];
+
+ rel = table_open(ExtensionRelationId, AccessShareLock);
+
+ ScanKeyInit(&entry[0],
+ Anum_pg_extension_oid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(ext_oid));
+
+ scandesc = systable_beginscan(rel, ExtensionOidIndexId, true,
+ NULL, 1, entry);
+
+ tuple = systable_getnext(scandesc);
+
+ /* We assume that there can be at most one matching tuple */
+ if (HeapTupleIsValid(tuple))
+ {
+ Datum datum;
+ bool isnull;
+
+ datum = heap_getattr(tuple, Anum_pg_extension_extversion,
+ RelationGetDescr(rel), &isnull);
+
+ if (isnull)
+ elog(ERROR, "extversion is null");
+
+ result = text_to_cstring(DatumGetTextPP(datum));
+ }
+ else
+ result = NULL;
+
+ systable_endscan(scandesc);
+
+ table_close(rel, AccessShareLock);
+
+ return result;
+}
+
/*
* Utility functions to check validity of extension and version names
*/
diff --git a/src/include/commands/extension.h b/src/include/commands/extension.h
index 74ae391395..3563e07b9f 100644
--- a/src/include/commands/extension.h
+++ b/src/include/commands/extension.h
@@ -48,6 +48,7 @@ extern ObjectAddress ExecAlterExtensionContentsStmt(AlterExtensionContentsStmt *
extern Oid get_extension_oid(const char *extname, bool missing_ok);
extern char *get_extension_name(Oid ext_oid);
extern Oid get_extension_schema(Oid ext_oid);
+extern char *get_extension_version(Oid ext_oid);
extern bool extension_file_exists(const char *extensionName);
extern ObjectAddress AlterExtensionNamespace(const char *extensionName, const char *newschema,
--
2.39.2