Subject: [PATCH] Support % wildcard in extension upgrade filenames

From: Sandro Santilli <[email-masked]>
Date: Fri, 11 Feb 2022 18:59:50 +0100

At PostGIS we've been thinking of ways to have better support, from
PostgreSQL proper, to our upgrade strategy, which has always consisted
in a SINGLE upgrade file good for upgrading from any older version.

The current lack of such support for EXTENSIONs forced us to install
a lot of files on disk and we'd like to stop doing that at some point
in the future.

The proposal is to support wildcards for versions encoded in the
filename so that (for our case) a single wildcard could match "any
version". I've been thinking about the '%' character for that, to
resemble the wildcard used for LIKE.

Here's the proposal:
https://lists.osgeo.org/pipermail/postgis-devel/2022-February/029500.html

A very very short (and untested) patch which might (or
might not) support our case is attached.

The only problem with my proposal/patch would be the presence, on the
wild, of PostgreSQL EXTENSION actually using the '%' character in
their version strings, which is currently considered legit by
PostgreSQL.

How do you feel about the proposal (which is wider than the patch) ?

--strk; 

  Libre GIS consultant/developer
  https://strk.kbt.io/services.html
>From e9d060b19c655924c4edb6679169261d605f735d Mon Sep 17 00:00:00 2001
From: Sandro Santilli <[email-masked]>
Date: Fri, 11 Feb 2022 18:49:45 +0100
Subject: [PATCH] Support % wildcard in extension upgrade scripts

---
 src/backend/commands/extension.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index a2e77c418a..aafd9c17ee 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -1072,6 +1072,8 @@ get_ext_ver_info(const char *versionname, List **evi_list)
 	foreach(lc, *evi_list)
 	{
 		evi = (ExtensionVersionInfo *) lfirst(lc);
+		if (strcmp(evi->name, "%") == 0)
+			return evi;
 		if (strcmp(evi->name, versionname) == 0)
 			return evi;
 	}
-- 
2.30.2

---

From: "Regina Obe" <[email-masked]>
Date: Fri, 27 May 2022 17:37:12 -0400

> PostgreSQL proper, to our upgrade strategy, which has always consisted in
a
> SINGLE upgrade file good for upgrading from any older version.
> 
> The current lack of such support for EXTENSIONs forced us to install a lot
of
> files on disk and we'd like to stop doing that at some point in the
future.
> 
> The proposal is to support wildcards for versions encoded in the filename
so
> that (for our case) a single wildcard could match "any version". I've been
> thinking about the '%' character for that, to resemble the wildcard used
for
> LIKE.
> A very very short (and untested) patch which might (or might not) support
our
> case is attached.
wild,
> of PostgreSQL EXTENSION actually using the '%' character in their version
> strings, which is currently considered legit by PostgreSQL.
> 
[Regina Obe] 

Just a heads up about the above, Sandro has added it as a commitfest item
which hopefully we can polish in time for PG16.
https://commitfest.postgresql.org/38/3654/

Does anyone think this is such a horrible idea that we should abandon all
hope?

The main impetus is that many extensions (postgis, pgRouting, and I'm sure
others) have over 300 extensions script files that are exactly the same.
We'd like to reduce this footprint significantly.

strk said the patch is crappy so don't look at it just yet.  We'll work on
polishing it.  I'll review and provide docs for it.

Thanks,
Regina

---

From: Laurenz Albe <[email-masked]>
Date: Sat, 28 May 2022 16:50:20 +0200

On Fri, 2022-05-27 at 17:37 -0400, Regina Obe wrote:
> > PostgreSQL proper, to our upgrade strategy, which has always consisted in a
> > The current lack of such support for EXTENSIONs forced us to install a lot of
> > files on disk and we'd like to stop doing that at some point in the future.
> > 
> > The proposal is to support wildcards for versions encoded in the filename so
> > thinking about the '%' character for that, to resemble the wildcard used for
> > LIKE.
> > 
> > Here's the proposal:
> > A very very short (and untested) patch which might (or might not) support our
> > case is attached.
> > 
> > The only problem with my proposal/patch would be the presence, on the wild,

I don't think this idea is fundamentally wrong, but I have two worries:

1. It would be a good idea good to make sure that there is not both
   "extension--%--2.0.sql" and "extension--1.0--2.0.sql" present.
   Otherwise the behavior might be indeterministic.

2. What if you have a "postgis--%--3.3.sql", and somebody tries to upgrade
   their PostGIS 1.1 installation with it?  Would that work?
   Having a lower bound for a matching version might be a good idea,
   although I have no idea how to do that.

Yours,
Laurenz Albe

---

From: Daniel Gustafsson <[email-masked]>
Date: Sat, 28 May 2022 17:26:05 +0200

> On 28 May 2022, at 16:50, Laurenz Albe <[email-masked]> wrote:

> although I have no idea how to do that.

Following that reasoning, couldn't a rogue actor inject a fake file (perhaps
bundled with another innocent looking extension) which takes precedence in
wildcard matching?

--
Daniel Gustafsson		https://vmware.com/

---

From: Tom Lane <[email-masked]>
Date: Sat, 28 May 2022 11:37:30 -0400

>    Otherwise the behavior might be indeterministic.

The existing logic to find multi-step upgrade paths is going to make
this a very pressing problem.  For example, if you provide both
extension--%--2.0.sql and extension--%--2.1.sql, it's not at all
clear whether the code would try to use both of those or just one
to get from 1.x to 2.1.

>    although I have no idea how to do that.

The lack of upper bound is a problem too: what stops the system from
trying to use this to get from (say) 4.2 to 3.3, and if it does try that,
will the script produce a sane result?  (Seems unlikely, as it couldn't
know what 4.x objects it ought to alter or remove.)  But it's going to be
very hard to provide bounds, because we intentionally designed the
extension system to not have an explicit concept of some versions being
less than others.

I'm frankly skeptical that this is a good idea at all.  It seems
to have come out of someone's willful refusal to use the system as
designed, ie as a series of small upgrade scripts that each do just
one step.  I don't feel an urgent need to cater to the
one-monster-script-that-handles-all-cases approach, because no one
has offered any evidence that that's really a better way.  How would
you even write the conditional logic needed ... plpgsql DO blocks?
Testing what?  IIRC we don't expose any explicit knowledge of the
old extension version number to the script.

			regards, tom lane

---

From: Sandro Santilli <[email-masked]>
Date: Sat, 4 Jun 2022 11:20:55 +0200

>    Otherwise the behavior might be indeterministic.

I'd make sure to use extension--1.0--2.0.sql in that case (more
specific first).

>    their PostGIS 1.1 installation with it?  Would that work?

For PostGIS in particular it will NOT work as the PostGIS upgrade
script checks for the older version and decides if the upgrade is
valid or not. This is the same upgrade code used for non-extension
installs.

>    although I have no idea how to do that.

I was thinking of a broader pattern matching support, like:

  postgis--3.%--3.3.sql

But it would be better to start simple and eventually if needed
increase the complexity ?

Another option could be specifying something in the control file,
which would also probably be a good idea to still allow some
extensions to use '%' in the version string (for example).

--strk;

---

From: Sandro Santilli <[email-masked]>
Date: Sat, 4 Jun 2022 11:21:53 +0200

> bundled with another innocent looking extension) which takes precedence in

I think whoever can write into the PostgreSQL extension folder will
be able to inject anything anyway....

--strk;

---

From: Sandro Santilli <[email-masked]>
Date: Sat, 4 Jun 2022 11:26:19 +0200

> will the script produce a sane result?

This is a very old problem we had before EXTENSION was even available
in PostgreSQL, and so we solved this internally. The upgrade script
for PostGIS checks the version of the existing code and refuses to
downgrade (and refuses to upgrade if a dump/restore is required).

> old extension version number to the script.

We (PostGIS) do expose explicit knowledge of the old extension, and
for this reason I think the pattern-based logic should be
enabled explicitly in the postgis.control file. It could be even less
generic and more specific to a given extension need, if done
completely inside the control file. For PostGIS all we need at the
moment is something like (in the control file):

  one_monster_upgrade_script = postgis--ANY--3.3.0.sql

--strk;

---

From: Sandro Santilli <[email-masked]>
Date: Thu, 15 Sep 2022 00:01:00 +0200

I'm attaching an updated version of the patch. This time the patch
is tested. Nothing changes unless the .control file for the subject
extension doesn't have a "wildcard_upgrades = true" statement.

When wildcard upgrades are enabled, a file with a "%" symbol as
the "source" part of the upgrade path will match any version and
will be used if a specific version upgrade does not exist.
This means that in presence of the following files:

    postgis--3.0.0--3.2.0.sql
    postgis--%--3.2.0.sql

The first one will be used for going from 3.0.0 to 3.2.0.

This is the intention. The patch lacks automated tests and can
probably be improved.

For more context, a previous (non-working) version of this patch was
submitted to commitfest: https://commitfest.postgresql.org/38/3654/

--strk;

---

From: Sandro Santilli <[email-masked]>
Date: Thu, 15 Sep 2022 00:02:49 +0200

And now with the actual patch attached ... (sorry)

--strk;

---

From: "Regina Obe" <[email-masked]>
Date: Mon, 31 Oct 2022 01:55:05 -0400

Just to reiterate the main impetus for this patch is to save PostGIS from
shipping 100s of duplicate extension files for each release. 

> And now with the actual patch attached ... (sorry)

Sandro, can you submit an updated version of this patch.
I was testing it out and looked good first time.

But I retried just now testing against master, and it fails with 

commands/extension.o: In function `file_exists':
postgresql-git\src\backend\commands/extension.c:3430: undefined reference to
`AssertArg'

It seems 2 days ago AssertArg and AssertState were removed.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b1099eca8f38f
f5cfaf0901bb91cb6a22f909bc6

So your use of AssertArg needs to be replaced with Assert I guess. 
I did that and was able to test again with a sample extension I made

Called:

wildtest

1) The wildcard patch in its current state only does anything if 

wildcard_upgrades = true 

is in the control file. If it's false or missing, then the behavior of
extension upgrades doesn't change.

2) It only understands % as a complete wildcard for a version number

So this is legal
wildtest--%--2.0.sql

This does nothing
wildtest--2%--2.0.sql

3) I confirmed that if you have a path such as

wildtest--2.0--2.2.sql
wildtest--%--2.2.sql

then the exact match trumps the wildcard. In the above case if I am on 2.0
and going to 2.2, the wildtest--2.0--2.2.sql script is used instead of the
wildtest--% one.

4) It is not possible to downgrade with the wildcard.  For example I had
paths
wildtest--%--2.1.sql  

and I was unable to go from a version 2.2 down to a version 2.1.  I didn't
check why that was so, but probably a good thing.

If everyone is okay with this patch, we'll go ahead and add tests and
documentation to go with it.

Thanks,
Regina

---

From: Sandro Santilli <[email-masked]>
Date: Fri, 4 Nov 2022 18:31:58 +0100

> I was testing it out and looked good first time.

Attached updated version of the patch.

> documentation to go with it.

Yes please let us know if there's any chance of getting this
mechanism approved or if we should keep advertising works around
this limitation (it's not just installing 100s of files but also
still missing needed upgrade paths when doing so).

--strk;

---

From: Sandro Santilli <[email-masked]>
Date: Wed, 9 Nov 2022 18:53:52 +0100

And now a version of the patch including documentation and regression tests.
Anything you see I should improve ?

--strk;

---

From: LEO HSU <[email-masked]>
Date: Thu, 10 Nov 2022 01:10:52 +0000

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

PURPOSE:  

This feature is intended to allow projects with many micro versions that do the same thing, be able to ship much fewer files by specifying less granular stopping.
One of the projects that will benefit from this is the PostGIS project.  Currently we ship over 300 extension scripts per version which are currently just symlinks to the same file.
Part of the reason for that is our changes are dependent on both PostgreSQL version and PostGIS version, so a simple upgrade that only considers say PostGIS 3.2.0--3.3.1 is not sufficient.
Also much of the time, our function definitions don't change in micros, but yet we need to ship them to allow users to properly upgrade.

This feature will allow us to get rid of all these symlinks or 0-byte files.

I've tested this feature against the PostgreSQL master branch on mingw64 gcc 8.1.

BASIC FEATURES

1) As stated, this feature only works if in the .control file, has a line:

wildcard_upgrades = true

2) It does nothing different if the line is missing or set to false.

3) If there is such a line and there is no other path that takes an extension from it's current version to the requested version, then it will use the wildcard script file.

TESTING

AUTOMATED TESTS
I have confirmed tests are in place.
However the tests do not run when I do 
make check (from the root source folder)

or when I do

make installcheck-world

To run these tests, I had to

cd src/test/modules/test_extensions
make check

and see the output showing:

============== running regression test queries        ==============
test test_extensions              ... ok          186 ms
test test_extdepend               ... ok           97 ms

I confirmed the tests are in test_extensions, which has always existed.
So I assume why it's not being run in installcheck-world is something off with my configuration 
or it's intentionally not run.

MANUAL TESTS
 I also ran some adhoc tests of my own to confirm the behavior. and checking with
SET client_min_messages='DEBUG1';

To confirm that the wildcard script I have only gets called when there is no other path that will take the user to the new version.
I created my own extension
wildtest

1) I confirmed  It only understands % as a complete wildcard for a version number

So this is legal
wildtest--%--2.0.sql

This does nothing
wildtest--2%--2.0.sql

2) I confirmed that if you have a path such as

wildtest--2.0--2.2.sql
wildtest--%--2.2.sql

3) It is not possible to downgrade with the wildcard.  For example I had
paths
wildtest--%--2.1.sql  

and I was unable to go from a version 2.2 down to a version 2.1.

DOCUMENTATION

I built the html docs and confirmed that in the section of the docs where it defines valid properties of extension files it now includes this line:

wildcard_upgrades (boolean)

    This parameter, if set to true (which is not the default), allows ALTER EXTENSION to consider a wildcard character % as matching any version of the extension. Such wildcard match will only be used when no perfect match is found for a version.

The new status of this patch is: Ready for Committer

---

From: Regina Obe <[email-masked]>
Date: Thu, 10 Nov 2022 15:23:31 +0000

Apologies.  I made a mistake on my downgrade test.
So 3 should be

3) It is possible to downgrade with the wildcard.  For example I had
paths
wildtest--%--2.1.sql 

and confirmed it used the downgrade path when going from 2.2 to 2.1

---

From: Christoph Berg <[email-masked]>
Date: Fri, 11 Nov 2022 16:59:06 +0100

Re: Tom Lane
> clear whether the code would try to use both of those or just one

find_update_path already includes Dijkstra to avoid that problem.

> old extension version number to the script.

I was just talking to strk about that on #postgis and where's what I
took away from the discussion:

The main objection seems to be that 500 identical (linked) files
aren't how the extension system is supposed to be used, and that this
proposal merely changes that to a single foo--%--1.2.3.sql file which
is again not how the system is supposed to be used. Instead, we should
try to find a small set of files that just change what needs to be
changed between the versions.

First, it's 580 files because the same problem exists for 7 extensions
shipped in postgis, so we are effectively talking about 80 files per
extension.

So the question is, can we reduce that 80 to some small number. It
turns out the answer is "no" for several reasons:

* parallel branches: postgis maintains several major branches, and
  people can upgrade at any time. If we currently have 3.3.4 and
  3.4.0, we will likely have a 3.3.4--3.4.0.sql file. Now when 3.3.5
  and 3.4.1 are released, we add 3.3.4--3.3.5 in the 3.3 branch, and a
  3.4.0--3.4.1 in the 3.4 branch.

  But we will also have to provide a new 3.3.5--3.4.0 (or
  3.3.5--3.4.1) file *in the 3.4 branch*. That means even if we decide
  that upgrades always need to go through a 3.4.0 pivot version, we
  have to update the 3.4 branch each time 3.3 is updated, because
  history isn't single-branch linear. Lots of extra files, which need
  to be stored in a non-natural location.

  If we wanted to use the "proper" update files here, we'd be left
  with something like (number of major versions)*(number of all minor
  versions in total) number of files, spread over several major
  versions, all interconnected.

* Even if we disregard topology, we still have to provide *some*
  start-updates-here file for each minor version released ever before.
  Hence we are at 80. (The number might be lower for smaller
  extensions like address_standardizer, but postgis itself would
  change in each version, plus many of the other extensions in there.)

Christoph

---

From: Christoph Berg <[email-masked]>
Date: Fri, 11 Nov 2022 17:07:13 +0100

Re: Sandro Santilli
> the "source" part of the upgrade path will match any version and

Fwiw I believe wildcard_upgrades isn't necessary in the .control file.
If there are no % files, none would be used anyway, and if there are,
it's clear it's meant as wildcard since % won't appear in any remotely
sane version number.

Christoph

---

From: "Regina Obe" <[email-masked]>
Date: Sun, 13 Nov 2022 23:46:50 -0500

> Re: Sandro Santilli
> > I'm attaching an updated version of the patch. This time the patch is
> > tested. Nothing changes unless the .control file for the subject
> > When wildcard upgrades are enabled, a file with a "%" symbol as the
> > "source" part of the upgrade path will match any version and
> 
> If there are no % files, none would be used anyway, and if there are, it's
clear
> it's meant as wildcard since % won't appear in any remotely sane version
> number.
> 
> Christoph

I also like the idea of skipping the wildcard_upgrades syntax.
Then there is no need to have a conditional control file for PG 16 vs. older
versions.

Thanks,
Regina

---

From: 'Sandro Santilli' <[email-masked]>
Date: Thu, 17 Nov 2022 00:09:33 +0100

> Then there is no need to have a conditional control file for PG 16 vs. older

Here we go. Attached a version of the patch with no
"wildcard_upgrades" controlling it.

--strk;

---

From: "Regina Obe" <[email-masked]>
Date: Wed, 16 Nov 2022 18:16:11 -0500

> > > > is tested. Nothing changes unless the .control file for the
> > > > subject extension doesn't have a "wildcard_upgrades = true"
statement.
> > > it's meant as wildcard since % won't appear in any remotely sane
> > > version number.
> >
> > Then there is no need to have a conditional control file for PG 16 vs.
> > older versions.
> 
> Here we go. Attached a version of the patch with no "wildcard_upgrades"
> controlling it.
> 
> --strk;

I think you should increment the version number on the file name of this
patch.
You had one earlier called 0001-...
The one before that was missing a version number entirely.

Maybe call this 0003-...

Thanks,
Regina

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 16 Nov 2022 17:25:07 -0600

Note that the patch is passing tests when using autoconf build but
failing for meson builds.

http://cfbot.cputube.org/sandro-santilli.html

I imagine you need to make the corresponding change to ./meson.build
that you made to ./Makefile.

https://wiki.postgresql.org/wiki/Meson_for_patch_authors
https://wiki.postgresql.org/wiki/Meson

You can test under cirrusci from your github account, with instructions
at: ./src/tools/ci/README

Cheers,
-- 
Justin

---

From: Sandro Santilli <[email-masked]>
Date: Thu, 17 Nov 2022 10:57:34 +0100

> failing for meson builds.

Thanks for pointing me to the right direction.
I'm attaching an updated patch, will keep an eye on cirrusCI

--strk;

---

From: Regina Obe <[email-masked]>
Date: Tue, 22 Nov 2022 19:03:14 +0000

make installcheck-world:  tested, passed

I reviewed this patch https://www.postgresql.org/message-id/20221117095734.igldlk6kngr6ogim%40c19

Most look good to me.  The only things that have changed since last I reviewed this, with the new patch is

1) wildcard_upgrades=true is no longer needed in the control file and will break if present.  This is an expected change, since we are now going strictly by presence of % extension upgrade scripts per suggestions
2) The meson build issue has been fixed.  Cirrus is passing on that now.  I'm still fiddling with my meson setup, so didn't personally test this.

3) The documentation has been updated to no longer include wildcard_upgrades as a variable for control file.

and has this text now describing it's use:

 The literal value % can be used as the old_version component in an extension update script for it to match any version. Such wildcard update scripts will only be used when no explicit path is found from old to target version.

Given that a suitable update script is available, the command ALTER EXTENSION UPDATE will update an installed extension to the specified new version. The update script is run in the same environment that CREATE EXTENSION provides for installation scripts: in particular, search_path is set up in the same way, and any new objects created by the script are automatically added to the extension. Also, if the script chooses to drop extension member objects, they are automatically dissociated from the extension. 

I built the html docs but ran into a lot of warnings I don't recall getting last time.  I think this is just my doc local setup is a bit messed up or something else changed in the doc setup.

My main gripe with this patch is Sandro did not increment the version number of it, so it's the same name as an old patch he had submitted before.