RE: Can't compile PG 17 (master) from git under Msys2 autoconf
> > I think it ends up doing a copy thus the copy error in my log failures > > which don't exist anywhere in the Makefil > > > > cp -pR ../../backend/storage/lmgr/lwlocknames.h > > > > Sorry for not checking on a linux system. I was thinking I should have done > that first. > > Ah yeah, that's per configure: > > if ln -s conf$$.file conf$$ 2>/dev/null; then > as_ln_s='ln -s' > # ... but there are two gotchas: > # 1) On MSYS, both `ln -s file dir' and `ln file dir' fail. > # 2) DJGPP < 2.04 has no symlinks; `ln -s' creates a wrapper executable. > # In both cases, we have to default to `cp -pR'. > ln -s conf$$.file conf$$.dir 2>/dev/null && test ! -f conf$$.exe || > as_ln_s='cp -pR' > > I guess we need to patch the rule so that the LN_S is called so that it'd > resolve > correctly in both cases. I guess the easy option is to go back to the > original > recipe and update the comment to indicate that we do it to placate Msys2. > Here it is with the old comment: > > -# The point of the prereqdir incantation in some of the rules below is to > -# force the symlink to use an absolute path rather than a relative path. > -# For headers which are generated by make distprep, the actual header > within > -# src/backend will be in the source tree, while the symlink in src/include > -# will be in the build tree, so a simple ../.. reference won't work. > -# For headers generated during regular builds, we prefer a relative > symlink. > >$(top_builddir)/src/include/storage/lwlocknames.h: > storage/lmgr/lwlocknames.h > - prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \ > -cd '$(dir $@)' && rm -f $(notdir $@) && \ > -$(LN_S) "$$prereqdir/$(notdir $<)" . > > > Maybe it's possible to make this simpler, as it looks overly baroque, and we > don't really need absolute paths anyway -- we just need the path resolved at > the right time. > > -- > Álvaro HerreraBreisgau, Deutschland — > https://www.EnterpriseDB.com/ Yah I was thinking it was removed cause no one could figure out why it was so complicated and decided to make it more readable.
RE: Can't compile PG 17 (master) from git under Msys2 autoconf
> > I think I got something not too far off from what's there now that works > under my msys2 setup again. This is partly using your idea of using > $(top_builddir) to qualify the path but in the LN_S section that is causing me > grief. > > This seems to work okay building in tree and out of tree. > > By changing these lines in src/backend/Makefile > > > > $(top_builddir)/src/include/storage/lwlocknames.h: > storage/lmgr/lwlocknames.h > > rm -f '$@' > > - $(LN_S) ../../backend/$< '$@' > > + $(LN_S) $(top_builddir)/src/backend/$< '$@' > > > > $(top_builddir)/src/include/utils/wait_event_types.h: > utils/activity/wait_event_types.h > > rm -f '$@' > > - $(LN_S) ../../backend/$< '$@' > > + $(LN_S) $(top_builddir)/src/backend/$< '$@' > > > > I've also attached as a patch. > > Hmm, that's quite strange ... it certainly doesn't work for me. Maybe the > LN_S utility is resolving the symlink at creation time, rather than letting > it be a > reference to be resolved later. Apparently, the only Msys2 buildfarm animal > is > now using Meson, so we don't have any representative animal for your > situation. > > What does LN_S do for you anyway? Looking at > https://stackoverflow.com/questions/61594025/symlink-in-msys2-copy-or- > hard-link > it sounds like this would work if the MSYS environment variable was set to > winsymlinks (or maybe not. I don't know if a "windows shortcut" would be > usable in this case.) > > -- > Álvaro Herrera PostgreSQL Developer — > https://www.EnterpriseDB.com/ > "Nadie está tan esclavizado como el que se cree libre no siéndolo" (Goethe) I think it ends up doing a copy thus the copy error in my log failures which don't exist anywhere in the Makefil cp -pR ../../backend/storage/lmgr/lwlocknames.h Sorry for not checking on a linux system. I was thinking I should have done that first. Thanks, Regina
RE: Can't compile PG 17 (master) from git under Msys2 autoconf
> > Hi Regina, > > > > On 2024-Mar-27, Regina Obe wrote: > > > > > The error is > > > > > > rm -f '../../src/include/storage/lwlocknames.h' > > > cp -pR ../../backend/storage/lmgr/lwlocknames.h > > > '../../src/include/storage/lwlocknames.h' > > > cp: cannot stat '../../backend/storage/lmgr/lwlocknames.h': No such > > > file or directory > > > make[1]: *** [Makefile:143: ../../src/include/storage/lwlocknames.h] > > > Error 1 > > > make[1]: Leaving directory '/projects/postgresql/postgresql- > > git/src/backend' > > > make: *** [../../src/Makefile.global:382: submake-generated-headers] > > > Error 2 > > > > Hmm, I changed these rules again in commit da952b415f44, maybe your > > problem is with that one? I wonder if changing the references to > > ../include/storage/lwlocklist.h to > > $(topdir)/src/include/storage/lwlocklist.h > > (and similar things in > > src/backend/storage/lmgr/Makefile) would fix it. > > > > Do you run configure in the source directory or a separate build one? > > > > -- > > Álvaro HerreraBreisgau, Deutschland — > > https://www.EnterpriseDB.com/ > > "If it is not right, do not do it. > > If it is not true, do not say it." (Marcus Aurelius, Meditations) > > I tried the change > > ../include/storage/lwlocklist.h to > > $(top_builddir)/src/include/storage/lwlocklist.h > > I assume you meant that instead of $(topdir) > > But nah that made no difference. Your change was already in my patched > version so isn't causing any issues. I think I got something not too far off from what's there now that works under my msys2 setup again. This is partly using your idea of using $(top_builddir) to qualify the path but in the LN_S section that is causing me grief. This seems to work okay building in tree and out of tree. By changing these lines in src/backend/Makefile $(top_builddir)/src/include/storage/lwlocknames.h: storage/lmgr/lwlocknames.h rm -f '$@' - $(LN_S) ../../backend/$< '$@' + $(LN_S) $(top_builddir)/src/backend/$< '$@' $(top_builddir)/src/include/utils/wait_event_types.h: utils/activity/wait_event_types.h rm -f '$@' - $(LN_S) ../../backend/$< '$@' + $(LN_S) $(top_builddir)/src/backend/$< '$@' I've also attached as a patch. Thanks, Regina v1-0001-FIX-autoconf-build-under-Msys2-Mingw64.patch Description: Binary data
RE: Can't compile PG 17 (master) from git under Msys2 autoconf
> Hi Regina, > > On 2024-Mar-27, Regina Obe wrote: > > > The error is > > > > rm -f '../../src/include/storage/lwlocknames.h' > > cp -pR ../../backend/storage/lmgr/lwlocknames.h > > '../../src/include/storage/lwlocknames.h' > > cp: cannot stat '../../backend/storage/lmgr/lwlocknames.h': No such > > file or directory > > make[1]: *** [Makefile:143: ../../src/include/storage/lwlocknames.h] > > Error 1 > > make[1]: Leaving directory '/projects/postgresql/postgresql- > git/src/backend' > > make: *** [../../src/Makefile.global:382: submake-generated-headers] > > Error 2 > > Hmm, I changed these rules again in commit da952b415f44, maybe your > problem is with that one? I wonder if changing the references to > ../include/storage/lwlocklist.h to $(topdir)/src/include/storage/lwlocklist.h > (and similar things in > src/backend/storage/lmgr/Makefile) would fix it. > > Do you run configure in the source directory or a separate build one? > > -- > Álvaro HerreraBreisgau, Deutschland — > https://www.EnterpriseDB.com/ > "If it is not right, do not do it. > If it is not true, do not say it." (Marcus Aurelius, Meditations) I tried the change > ../include/storage/lwlocklist.h to > $(top_builddir)/src/include/storage/lwlocklist.h I assume you meant that instead of $(topdir) But nah that made no difference. Your change was already in my patched version so isn't causing any issues. But as stated, rolling back this change in src/backend/Makefile in 0a475f01a4a (November 6th commit) makes it work for me. $(top_builddir)/src/include/storage/lwlocknames.h: storage/lmgr/lwlocknames.h - prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \ - cd '$(dir $@)' && rm -f $(notdir $@) && \ - $(LN_S) "$$prereqdir/$(notdir $<)" . + rm -f '$@' + $(LN_S) ../../backend/$< '$@' $(top_builddir)/src/include/utils/wait_event_types.h: utils/activity/wait_event_types.h - prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \ - cd '$(dir $@)' && rm -f $(notdir $@) && \ - $(LN_S) "$$prereqdir/$(notdir $<)" . + rm -f '$@' + $(LN_S) ../../backend/$< '$@'
RE: Can't compile PG 17 (master) from git under Msys2 autoconf
> Hi Regina, > > On 2024-Mar-27, Regina Obe wrote: > > > The error is > > > > rm -f '../../src/include/storage/lwlocknames.h' > > cp -pR ../../backend/storage/lmgr/lwlocknames.h > > '../../src/include/storage/lwlocknames.h' > > cp: cannot stat '../../backend/storage/lmgr/lwlocknames.h': No such > > file or directory > > make[1]: *** [Makefile:143: ../../src/include/storage/lwlocknames.h] > > Error 1 > > make[1]: Leaving directory '/projects/postgresql/postgresql- > git/src/backend' > > make: *** [../../src/Makefile.global:382: submake-generated-headers] > > Error 2 > > Hmm, I changed these rules again in commit da952b415f44, maybe your > problem is with that one? I wonder if changing the references to > ../include/storage/lwlocklist.h to $(topdir)/src/include/storage/lwlocklist.h > (and similar things in > src/backend/storage/lmgr/Makefile) would fix it. > > Do you run configure in the source directory or a separate build one? > > -- > Álvaro HerreraBreisgau, Deutschland — > https://www.EnterpriseDB.com/ > "If it is not right, do not do it. > If it is not true, do not say it." (Marcus Aurelius, Meditations) I run in the source directory, but tried doing in a separate build directory and ran into the same issue. Let me look at that commit and get back to you if that makes a difference. Thanks, Regina
RE: Crash on UNION with PG 17
> On Thu, 28 Mar 2024 at 04:34, Regina Obe wrote: > > CREATE TABLE edge_data AS > > SELECT i AS edge_id, i + 1 AS start_node, i + 2 As end_node FROM > > generate_series(1,10) AS i; > > > > WITH edge AS ( > > SELECT start_node, end_node > > FROM edge_data > > WHERE edge_id = 1 > > ) > > SELECT start_node id FROM edge UNION > > SELECT end_node FROM edge; > > As of d5d2205c8, this query should work as expected. > > Thank you for letting us know about this. > > David Thanks for the fix. I confirm it works now and our bots are green again. Regina
Can't compile PG 17 (master) from git under Msys2 autoconf
I've been having trouble compiling PG 17 using msys2 / mingw64 (sorry my meson setup is a bit broken at moment, so couldn't test that.). Both my msys2 envs (Rev2, Built by MSYS2 project) 13.2.0 and my older setup (x86_64-posix-seh-rev0, Built by MinGW-W64 project) 8.1.0 Have the same issue: The error is rm -f '../../src/include/storage/lwlocknames.h' cp -pR ../../backend/storage/lmgr/lwlocknames.h '../../src/include/storage/lwlocknames.h' cp: cannot stat '../../backend/storage/lmgr/lwlocknames.h': No such file or directory make[1]: *** [Makefile:143: ../../src/include/storage/lwlocknames.h] Error 1 make[1]: Leaving directory '/projects/postgresql/postgresql-git/src/backend' make: *** [../../src/Makefile.global:382: submake-generated-headers] Error 2 I traced the issue down to this change in https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=721856ff24b $(top_builddir)/src/include/storage/lwlocknames.h: storage/lmgr/lwlocknames.h - prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \ - cd '$(dir $@)' && rm -f $(notdir $@) && \ - $(LN_S) "$$prereqdir/$(notdir $<)" . + rm -f '$@' + $(LN_S) ../../backend/$< '$@' $(top_builddir)/src/include/utils/wait_event_types.h: utils/activity/wait_event_types.h - prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \ - cd '$(dir $@)' && rm -f $(notdir $@) && \ - $(LN_S) "$$prereqdir/$(notdir $<)" . + rm -f '$@' + $(LN_S) ../../backend/$< '$@' Reverting just the above change fixes the issue. I'm not sure what all that does to be honest, so not sure the best way to move forward. My linux autoconf build systems do not have this issue. Thanks, Regina
Crash on UNION with PG 17
Our PostGIS bot that follows master branch has been crashing for past couple of days on one of our tests https://trac.osgeo.org/postgis/ticket/5701 I traced the issue down to this commit: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=66c0185a3d14b bbf51d0fc9d267093ffec735231 The issue can be exercised without postgis installed as follows: CREATE TABLE edge_data AS SELECT i AS edge_id, i + 1 AS start_node, i + 2 As end_node FROM generate_series(1,10) AS i; WITH edge AS ( SELECT start_node, end_node FROM edge_data WHERE edge_id = 1 ) SELECT start_node id FROM edge UNION SELECT end_node FROM edge; If I run using UNION ALL, this works fine: WITH edge AS ( SELECT start_node, end_node FROM edge_data WHERE edge_id = 1 ) SELECT start_node id FROM edge UNION ALL SELECT end_node FROM edge; Thanks, Regina
RE: [PATCH] Support % wildcard in extension upgrade filenames
> > On 1 Aug 2023, at 20:45, Robert Haas wrote: > > > > On Tue, Aug 1, 2023 at 2:24 PM Daniel Gustafsson > wrote: > >> returned with feedback. Please feel free to resubmit to a future CF > >> when there is a new version of the patch. > > > > Isn't the real problem here that there's no consensus on what to do? > > I don't disagree with that, but there is nothing preventing that discussion to > continue here or on other threads. The fact that consensus is that far away > and no patch that applies exist seems to me to indicate that a new CF entry is > a better option than pushing forward. > > -- > Daniel Gustafsson We are still interested. We'll clean up in the next week or so. Been tied up with PostGIS release cycle. To Robert's point, we are losing faith in coming up with a solution that everyone agrees is workable. What I thought Sandro had so far in his last patch seemed like a good compromise to me. If we get it back to the point of passing the CF Bot, can we continue on this thread or are we just wasting our time? Thanks, Regina
RE: [PATCH] Support % wildcard in extension upgrade filenames
> It isn't. ZDB, and I think (at least) PostGIS, have their own "version()" function. > Keeping everything the same version keeps me "sane" and eliminates a class > of round-trip questions with users. > Yes we have several version numbers and yes we too like to keep the extension version the same, cause it's the only thing that is universally consistent across all PostgreSQL extensions. Yes we have our own internal version functions. One for PostGIS (has the true lib version file) and a script version and we have logic to make sure they are aligned. We even have versions for our dependencies (PROJ, GEOS, GDAL) cause behavior of PostGIS changes based on versions of those. This goes for each extension we package that has a lib file (postgis, postgis_raster, postgis_topology, postgis_sfcgal) In addition to that we also have a version for PostgreSQL (that the scripts were installed on). To catch cases when a pg_upgrade is needed to go from 3.3.0 to 3.3.0. Yes we need same version upgrades (particularly because of pg_upgrade). Sandro and I were talking about this. This is something we do in our postgis_extensions_upgrade() (basically forcing an upgrade to a version that does nothing, so we can force an upgrade to 3.3.0 again) to make up for this limitation in extension model. The reason for that is features get exposed based on version of PostgreSQL you are running. So in 3.3.0 we leveraged the new gist fast indexing build, which is only enabled for users running PostgreSQL 15 and above. What usually happens is someone has PostGIS 3.3.0 say on PG 14, they pg_upgrade to PG 15 but they are running with PG 14 scripts So they are not taking advantage of the new PG 15 features until they do a SELECT postgis_extensions_upgrade(); So this is why we need the DO commands for scenarios like this. > One of my desires is that the on-disk .so's filename be associated with the > pg_extension entry and not Each. Individual. Function. There's a few > extensions that like to version the on-disk .so's filename which means a > CREATE OR REPLACE for every function on every extension version bump. > That forces an upgrade script even if the schema didn't technically change and > also creates the need for bespoke tooling around extension.sql and > upgrade.sql scripts. > > But I don't want to derail this thread. > > eric= This is more or less the reason why we had to do CREATE OR REPLACE for all our functions. In the past we minor versioned our lib files so we had postgis-2.4, postgis-2.5 At 3.0 after much in-fighting (battle between convenience of developers vs. regular old users just wanting to use PostGIS and my frustration trying to hold peoples hands thru pg_upgrade), we settled on major version for the lib file, with option for developers to still keep the minor. So default install will be postgis-3 for life of 3 series and become postgis-4 when 4 comes along (hopefully not for another 10 years). Completely stripping the version we decided not to do cause with the change we have a whole baggage of legacy functions we needed to stub as we removed them so pg_upgrade will work more or less seamlessly. So come postgis-4 these stubs will be removed. Our CI bots however many of them do use the minor versionings 3.1, 3.2, 3.3 etc, cause it's easier to test upgrades and do regressions. And many PostGIS developers do the same. So a replace of all functions is still largely needed. This is one of the reasons the whole chained upgrade path never worked for us and why we settled on one script to handle everything.
RE: [PATCH] Support % wildcard in extension upgrade filenames
> > > As for Tom's concern about downgrades, I think it's valid but it's a > > case that is easy to test for in Plpgsql and either handle or error. > > For example, we use semver so testing for a downgrade at the top of > > the upgrade script is trivial. > I was thinking about this more. The extension model as it stands doesn't allow an extension author to define version hierarchy. We handle this internally in our scripts to detect a downgrade attempt and stop it similar to what Mat is saying. Most of that is basically converting our version string to a numeric number which we largely do with a regex pattern. I was thinking if there were some way to codify that regex pattern in our control file, then wild cards can only be applied if such a pattern is defined and the %-- The % has to be numerically before the target version. Thanks, Regina
RE: [PATCH] Support % wildcard in extension upgrade filenames
> This change also makes it easier for extensions that use versioned .so files > (by that I mean uses extension-.so rather than extension.so). > Because such extensions can't really use the chaining property of the > existing upgrade system and so need to write a direct X--Y.sql migration file > for > every prior version X. I know the system wasn't designed for this, but in > reality a lot of extensions do this. Especially the more complex ones. > Hopefully this is helpful, > Mat Thanks for the feedback. Yes this idea of versioned .so is also one of the reasons why we always have to run a replace on all functions. Like for example on PostGIS side, we have option of full minor (so the lib could be postgis-3.3.so or postgis-3.so) For development I always include the minor version and the windows interim builds I build against our master branch has the minor. But the idea is someone can upgrade to stable version, so the script needs to always have the .so version in it to account for this shifting of options. Thanks, Regina
RE: Order changes in PG16 since ICU introduction
> > My opinion is that the switch to using ICU by default is ill-advised > > and should be reverted. > > Most of the complaints seem to be complaints about v15 as well, and while > those complaints may be a reason to not make ICU the default, they are also > an argument that we should continue to learn and try to fix those issues > because they exist in an already-released version. > Leaving it the default for now will help us fix those issues rather than hide > them. > > It's still early, so we have plenty of time to revert the initdb default if > we need > to. > > Regards, > Jeff Davis I'm fine with that. Sounds like it wouldn't be too hard to just pull it out at the end. Before this, I didn't even know ICU existed in PG15. My first realization that ICU was even a thing was when my PG16 refused to compile without adding my ICU path to my pkg-config or putting in --without-icu. So yah I suspect leaving it in a little bit longer will uncover some more issues and won't harm too much. Thanks, Regina
RE: Order changes in PG16 since ICU introduction
> Yeah. My recommendation is just LOCALE: > > regression=# CREATE DATABASE test1 TEMPLATE=template0 ENCODING = > 'UTF8' LOCALE = 'C'; CREATE DATABASE regression=# CREATE DATABASE test2 > TEMPLATE=template0 ENCODING = 'UTF8' ICU_LOCALE = 'C'; > NOTICE: using standard form "en-US-u-va-posix" for locale "C" > CREATE DATABASE > > I think it's probably intentional that ICU_LOCALE is stricter about being given > a real ICU locale name, but I didn't write any of that code. > > regards, tom lane CREATE DATABASE test1 TEMPLATE=template0 ENCODING = 'UTF8' LOCALE = 'C'; Doesn't seem to work at least not under mingw64 anyway. SELECT '+' < '-' ; Returns false
RE: Order changes in PG16 since ICU introduction
> > CREATE DATABASE test TEMPLATE=template0 ENCODING = 'UTF8' > LC_COLLATE = 'C' > > LC_CTYPE = 'C'; > > As has been pointed out already, setting LC_COLLATE/LC_CTYPE is > meaningless when the locale provider is ICU. You need to look at what ICU > locale is being chosen, or force it with LOCALE = 'C'. > > regards, tom lane Okay got it was on IRC with RhodiumToad and he suggested: CREATE DATABASE test2 TEMPLATE=template0 ENCODING = 'UTF8' LC_COLLATE = 'C' LC_CTYPE = 'C' ICU_LOCALE='C'; Which gives expected result: SELECT '+' < '-' ; -- true but gives me a notice: NOTICE: using standard form "en-US-u-va-posix" for locale "C"
RE: Order changes in PG16 since ICU introduction
> Peter Eisentraut writes: > > If the database is created with locale provider ICU, then lc_collate > > does not apply here, so the result might be correct (depending on what > > locale you have set). > > FWIW, an installation created under LANG=C defaults to ICU locale en-US-u- > va-posix for me (see psql \l), and that still sorts as expected on my RHEL8 box. > We've not seen buildfarm problems either. > > I am wondering however whether this doesn't mean that all our carefully > coded fast paths for C locale just went down the drain. Does the ICU code > have any of that? Has any performance testing been done to see what impact > this change had on C-locale installations? (The current code coverage report > for pg_locale.c is not encouraging.) > > regards, tom lane Just another metric. On my mingw64 setup, I built a test database on PG16 (built with icu support) and PG15 (no icu support) CREATE DATABASE test TEMPLATE=template0 ENCODING = 'UTF8' LC_COLLATE = 'C' LC_CTYPE = 'C'; I think the above is the similar setup we have when testing. On PG15 SELECT '+' < '-' ; returns true On PG 16 returns false For PG 16, to strk's point, you have to do: to get a true SELECT '+' COLLATE "C" < '-' COLLATE "C"; I would expect since I'm initializing my db in collate C they would both behave the same
Order changes in PG16 since ICU introduction
A couple of days ago, our PostGIS PG16 bots started failing with order changes in text. We have our tests set to locale=c It seems since April 20th, our tests that rely on sorting characters changed. As noted in this ticket: https://trac.osgeo.org/postgis/ticket/5375 I'm assuming it's result of icu change: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=fcb21b3ac dcb9a60313325618fd7080aa36f1626 I suspect all our bots are compiling with icu enabled. But I haven't confirmed. I'm assuming this is an expected change in behavior, but just want to confirm. Thanks, Regina
RE: [PATCH] Support % wildcard in extension upgrade filenames
> Here are my thoughts of how this can work to satisfy our specific needs and > that of others who have many micro versions. > > 1) We define an additional file. I'll call this a paths file > > So for example postgis would have a > > postgis.paths file > > The format of the path file would be of the form > > , => 3.3.0--3.4.0 > > It will also allow a wildcard option > % => ANY--3.4.0.sql > > So a postgis.paths with multiple lines might look like > > 3.2.0,3.2.1 => 3.2.2--3.3.0 > 3.3.% => 3.3--3.4.0 > % => ANY--3.4.0 > > 2) The order of precedence would be: > > a) physical files are always used first > b) If no physical path is present on disk, then it looks at a .paths > file to formulate virtual paths > c) Longer mappings are preferred over shorter mappings > > So that means the % => ANY--3.4.0 would be the path of last resort > > Let's say our current installed version of postgis is postgis VERSION 3.2.0 > > The above path formulated would be > > 3.2.0 -> 3.3.0 -> 3.4.0 > The simulated scripts used to get there would be > > postgis--3.2.2--3.3.0.sql > postgis--3.3.0--3.4.0.sql > > > This however does not solve the issue of downgrades, which admittedly > postgis is not concerned about since we have accounted for that in our > internal scripts. > > So we might have issue with having a bear: %. If we don't allow a bear % > > Then our postgis patterns might look something like: > > 3.%, 2.% => ANY --3.4.0 > > Which would mean 3.0.1, 3.0.2, 3.2.etc would all use the same script. > > Which would still be a major improvement from what we have today and > minimizes likeliness of downgrade footguns. > > Thoughts anyone? > Minor correction scripts to get from 3.2.0 to 3.4.0 would be: postgis--3.2.2--3.3.0.sql postgis--3.3--3.4.0.sql
RE: [PATCH] Support % wildcard in extension upgrade filenames
Here are my thoughts of how this can work to satisfy our specific needs and that of others who have many micro versions. 1) We define an additional file. I'll call this a paths file So for example postgis would have a postgis.paths file The format of the path file would be of the form , => 3.3.0--3.4.0 It will also allow a wildcard option % => ANY--3.4.0.sql So a postgis.paths with multiple lines might look like 3.2.0,3.2.1 => 3.2.2--3.3.0 3.3.% => 3.3--3.4.0 % => ANY--3.4.0 2) The order of precedence would be: a) physical files are always used first b) If no physical path is present on disk, then it looks at a .paths file to formulate virtual paths c) Longer mappings are preferred over shorter mappings So that means the % => ANY--3.4.0 would be the path of last resort Let's say our current installed version of postgis is postgis VERSION 3.2.0 The above path formulated would be 3.2.0 -> 3.3.0 -> 3.4.0 The simulated scripts used to get there would be postgis--3.2.2--3.3.0.sql postgis--3.3.0--3.4.0.sql This however does not solve the issue of downgrades, which admittedly postgis is not concerned about since we have accounted for that in our internal scripts. So we might have issue with having a bear: %. If we don't allow a bear % Then our postgis patterns might look something like: 3.%, 2.% => ANY --3.4.0 Which would mean 3.0.1, 3.0.2, 3.2.etc would all use the same script. Which would still be a major improvement from what we have today and minimizes likeliness of downgrade footguns. Thoughts anyone?
RE: [PATCH] Support % wildcard in extension upgrade filenames
> Personally I don't see the benefit of 1 big file vs. many 0-length files to justify > the cost (time and complexity) of a PostgreSQL change, with the > corresponding cost of making use of this new functionality based on > PostgreSQL version. > >From a packaging stand-point 1 big file is better than tons of 0-length files. Fewer files to uninstall and to double check when testing. As to the added complexity agree it's more but in my mind worth it if we could get rid of this mountain of files. But my vote would be the wild-card solution as I think it would serve more than just postgis need. Any project that rarely does anything but add, remove, or modify functions doesn't really need multiple upgrade scripts and I think quite a few extensions fall in that boat. > We'd still have the problem of missing upgrade paths unless we release a new > version of PostGIS even if it's ONLY for the sake of updating that 1 big file (or > adding a new file, in the current situation). > > --strk; I think we always have more releases with newer stable versions than older stable versions. I can't remember a case when we had this ONLY issue. If there is one fix in an older stable, version we usually wait for several more before bothering with a release and then all the stables are released around the same time. So I don't see the ONLY being a real problem.
RE: [PATCH] Support % wildcard in extension upgrade filenames
> Packager might actually know better in that they could ONLY consider the > packages ever packaged by them. > I'm a special case packager cause I'm on the PostGIS project and I only package postgis related extensions, but even I find this painful. But for most packagers, I think they are juggling too many packages and too many OS versions to micro manage the business of each package. In my case my job is simple. I deal just with Windows and that doesn't change from Windows version to Windows version (just PG specific). Think of upgrading from Debian 10 to Debian 12 - what would you as a PG packager expect people to be running and upgrading from? They could be switching from say the main distro to the pgdg distro. > Hey, best would be having support for wildcard wouldn't it ? > For PostGIS yes and any other extension that does nothing but add new functions or replaces existing ones. For others some minor handling would be ideal, though I guess some other projects would be happy with a wildcard (e.g. pgRouting would prefer a wildcard) since most of the changes are just additions of new functions or replacements of existing functions. For something like h3-pg I think a simpler micro handling would be ideal, though not sure. They ship two extensions (one that is a bridge to postgis and their newest takes advantage of postgis_raster too) https://github.com/zachasme/h3-pg/tree/main/h3_postgis/sql/updates https://github.com/zachasme/h3-pg/tree/main/h3/sql/updates Many of their upgrades are No-ops cause they really are just lib upgrades. I'm thinking maybe we should discuss these ideas with projects who would benefit most from this: (many of which I'm familiar with because they are postgis offsprings and I package or plan to package them - pgRouting, h3-pg, pgPointCloud, mobilityDb, Not PostGIS offspring: ZomboDB - https://github.com/zombodb/zombodb/tree/master/sql - (most of those are just changing versioning on the function call, so yah wildcard would be cleaner there) TimescaleDB - https://github.com/timescale/timescaledb/tree/main/sql/updates ( I don't think a wildcard would work here especially since they have some downgrade paths, but is a useful example of a micro-level extension upgrade pattern we should think about if we could make easier) https://github.com/timescale/timescaledb/tree/main/sql/updates > > I much preferred the idea of just listing all our upgrade targets in the > control file. > > Again: how would we know all upgrade targets ? > We already do, remember you wrote it :) https://git.osgeo.org/gitea/postgis/postgis/src/branch/master/extensions/upg radeable_versions.mk Yes it does require manual updating each release cycle (and putting in versions from just released stable branches). I can live with continuing with that exercise. It was a nice to have but is not nearly as annoying as 1000 scripts or as a packager trying to catalog what versions of packages have I released that I need to worry about. > > We need to come up with a convention of how to describe a micro > > update, as it's really a problem with extensions that follow the > > pattern > > I think it's a problem with extensions maintaining stable branches, as if the > history was linear we would possibly need less files (although at this stage > any number bigger than 1 would be too much for me) > > --strk; I'm a woman of compromise. Sure 1 file would be ideal, but I'd rather live with a big file listing all version upgrades than 1000 files with the same information. It's cleaner to read a single file than make sense of a pile of files.
RE: [PATCH] Support % wildcard in extension upgrade filenames
> On Mon, Apr 03, 2023 at 09:26:25AM +0700, Yurii Rashkovskii wrote: > > I want to chime in on the issue of lower-number releases that are > > released after higher-number releases. The way I see this particular > > problem is that we always put upgrade SQL files in release "packages," > > and they obviously become static resources. > > > > While I [intentionally] overlook some details here, what if (as a > > convention, for projects where it matters) we shipped extensions with > > non-upgrade SQL files only, and upgrades were available as separate > > downloads? This way, we're not tying releases themselves to upgrade paths. > > This also requires no changes to Postgres. > > This is actually something that's on the plate, and we recently added a -- > disable-extension-upgrades-install configure switch and a `install-extension- > upgrades-from-known-versions` make target in PostGIS to help going in that > direction. I guess the ball would now be in the hands of packagers. > > > I know this may be a big delivery layout departure for > > well-established projects; I also understand that this won't solve the > > problem of having to have these files in the first place (though in > > many cases, they can be automatically generated once, I suppose, if they are > trivial). > > We will now also be providing a `postgis` script for administration that among > other things will support a `install-extension-upgrades` command to install > upgrade paths from specific old versions, but will not hard-code any list of > "known" old versions as such a list will easily become outdated. > > Note that all upgrade scripts installed by the Makefile target or by the > `postgis` scripts will only be empty upgrade paths from the source version to > the fake "ANY" version, as the ANY-- upgrade path will still be the > "catch-all" upgrade script. > > --strk; > Sounds like a user and packaging nightmare to me. How is a packager to know which versions a user might have installed? and leaving that to users to run an extra command sounds painful. They barely know how to run ALTER EXTENSION postgis UPDATE; Or pg_upgrade I much preferred the idea of just listing all our upgrade targets in the control file. The many annoyance I have left is the 1000 of files that seem to grow forever. This isn't just postgis. It's pgRouting, it's h3_pg, it's mobilitydb. I don't want to have to set this up for every single extension that does micro-updates. I just want a single file that can list all the target paths worst case. We need to come up with a convention of how to describe a micro update, as it's really a problem with extensions that follow the pattern major.minor.micro In almost all cases the minor upgrade script works for all micros of that minor and in postgis yes we have a single script that works for all cases. Thanks, Regina
RE: Ability to reference other extensions by schema in extension scripts
> Pushed with some mostly-cosmetic adjustments (in particular I tried to make > the docs and tests neater). > > I did not commit the changes in get_available_versions_for_extension > to add no_relocate as an output column. Those were dead code because you > hadn't done anything to connect them up to an actual output parameter of > pg_available_extension_versions(). While I'm not necessarily averse to > making the no_relocate values visible somehow, I'm not convinced that > pg_available_extension_versions should be the place to do it. ISTM what's > relevant is the no_relocate values of *installed* extensions, not those of > potentially-installable extensions. If we had a view over pg_extension then > that might be a place to add this, but we don't. On the whole it didn't seem > important enough to pursue, so I just left it out. > > regards, tom lane Thanks. Agree with get_available_versions_for_extension, not necessary. Thanks, Regina
RE: Ability to reference other extensions by schema in extension scripts
> On Sat, Mar 11, 2023 at 03:18:18AM -0500, Regina Obe wrote: > > Attached is a revised patch with these changes in place. > > I've given a try to this patch. It builds and regresses fine. > > My own tests also worked fine. As long as ext1 was found in the ext2's > no_relocate list it could not be relocated, and proper error message is given > to user trying it. > > Nitpicking, there are a few things that are weird to me: > > 1) I don't get any error/warning if I put an arbitrary string into no_relocate > (there's no check to verify the no_relocate is a subset of the requires). > > 2) An extension can still reference extensions it depends on without putting > them in no_relocate. This may be intentional, as some substitutions may not > require blocking relocation, but felt inconsistent with the normal > @extschema@ which is never replaced unless an extension is marked as non- > relocatable. > > --strk; > > Libre GIS consultant/developer > https://strk.kbt.io/services.html Attached is a slightly revised patch to fix the extra whitespace in the extend.gml document that Sandro noted to me. Thanks, Regina 0007-Allow-use-of-extschema-reqextname-to-reference.patch Description: Binary data
RE: [PATCH] Support % wildcard in extension upgrade filenames
> > I wonder if a solution to this problem might be to provide some kind > > of a version map file. Let's suppose that the map file is a bunch of > > lines of the form X Y Z, where X, Y, and Z are version numbers. The > > semantics could be: we (the extension authors) promise that if you > > want to upgrade from X to Z, it suffices to run the script that knows > > how to upgrade from Y to Z. > > This would address Tom's concern, because if you write a master > > upgrade script, you have to explicitly declare the versions to which > > it applies. > > This would just move the problem from having 1968 files to having to write > 1968 lines in control files, 1968 lines in one control file, is still much nicer than 1968 files in my book. >From a packaging standpoint also much cleaner, as that single file gets replaced with each upgrade and no need to uninstall more junk from prior install. > We maintain multiple stable branches (5, at the moment: 2.5, 3.0, 3.1, 3.2, > 3.3) and would like to allow anyone running an old patched version to still > upgrade to a newer version released earlier. > > --strk; > > Libre GIS consultant/developer > https://strk.kbt.io/services.html That said, I really would like a wildcard option for issues strk mentioned. I still see the main use-case as for those that micro version and for this use case, they would need a way, not necessarily to have a single upgrade script, but a script for each minor. So something like 3.2.%--3.4.0 = 3.2--3.4.0 In many cases, they don't even need anything done in an upgrade step, aside from moving the dial button on extension up a notch to coincide with the lib version. In our case, yes ours would be below because we already block downgrades internally in our scripts %--3.4.0 = ANY--3.4.0 Or we could move to a 2.%--3.4.0 = 2--3.4.0 3.%--3.4.0 = 3--3.4.0 Thanks, Regina
RE: Ability to reference other extensions by schema in extension scripts
> I've given a try to this patch. It builds and regresses fine. > > My own tests also worked fine. As long as ext1 was found in the ext2's > no_relocate list it could not be relocated, and proper error message is given > to user trying it. > > Nitpicking, there are a few things that are weird to me: > > 1) I don't get any error/warning if I put an arbitrary string into no_relocate > (there's no check to verify the no_relocate is a subset of the requires). > I thought about that and decided it wasn't worth checking for. If an extension author puts in an extension not in requires it's on them as the docs say it should be in requires. It will just pretend that extension is not listed in no_relocate. > 2) An extension can still reference extensions it depends on without putting > them in no_relocate. This may be intentional, as some substitutions may not > require blocking relocation, but felt inconsistent with the normal > @extschema@ which is never replaced unless an extension is marked as non- > relocatable. > > --strk; > > Libre GIS consultant/developer > https://strk.kbt.io/services.html Yes this is intentional. As Tom mentioned, if for example an extension author decides To schema qualify @extschema:foo@ in their table definition, and they marked as requiring foo since such a reference is captured by a schema move, there is no need for them to prevent relocate of the foo extension (assuming foo was relocatable to begin with)
RE: Ability to reference other extensions by schema in extension scripts
> Subject: Re: Ability to reference other extensions by schema in extension > scripts > > "Regina Obe" writes: > >> requires = 'extfoo, extbar' > >> no_relocate = 'extfoo' > > > So when no_relocate is specified, where would that live? > > In the control file. > > > Would I mark the extfoo as not relocatable on CREATE / ALTER of said > > extension? > > Or add an extra field to pg_extension > > We don't record dependent extensions in pg_extension now, so that doesn't > seem like it would fit well. I was envisioning that ALTER EXTENSION SET > SCHEMA would do something along the lines of > > (1) scrape the list of dependent extensions out of pg_depend > (2) open and parse each of their control files > (3) fail if any of their control files mentions the target one in > no_relocate. > > Admittedly, this'd be a bit slow, but I doubt that ALTER EXTENSION SET > SCHEMA is a performance bottleneck for anybody. > > > I had tried to do that originally, e.g. instead of even bothering with > > such an extra arg, just mark it as not relocatable if the extension's > > script contains references to the required extension's schema. > > I don't think that's a great approach, because those references might appear > in places that can track a rename (ie, in an object name that's resolved to a > stored OID). Short of fully parsing the script file you aren't going to get a > reliable answer. I'm content to lay that problem off on the extension authors. > > > But then what if extfoo is upgraded? > > We already have mechanisms for version-dependent control files, so I don't > see where there's a problem. > > regards, tom lane Attached is a revised patch with these changes in place. 0006-Allow-use-of-extschema-reqextname-to-reference.patch Description: Binary data
RE: Ability to reference other extensions by schema in extension scripts
> "Regina Obe" writes: > >> requires = 'extfoo, extbar' > >> no_relocate = 'extfoo' > > > So when no_relocate is specified, where would that live? > > In the control file. > > > Would I mark the extfoo as not relocatable on CREATE / ALTER of said > > extension? > > Or add an extra field to pg_extension > > We don't record dependent extensions in pg_extension now, so that doesn't > seem like it would fit well. I was envisioning that ALTER EXTENSION SET > SCHEMA would do something along the lines of > > (1) scrape the list of dependent extensions out of pg_depend > (2) open and parse each of their control files > (3) fail if any of their control files mentions the target one in > no_relocate. > > Admittedly, this'd be a bit slow, but I doubt that ALTER EXTENSION SET > SCHEMA is a performance bottleneck for anybody. > Okay I'll move ahead with this approach. Thanks, Regina
RE: Ability to reference other extensions by schema in extension scripts
> No, pg_depend is not the thing to use for this. I was thinking of a new field in > the extension's control file, right beside where it says it's dependent on such- > and-such extensions in the first place. Say like > > requires = 'extfoo, extbar' > no_relocate = 'extfoo' > So when no_relocate is specified, where would that live? Would I mark the extfoo as not relocatable on CREATE / ALTER of said extension? Or add an extra field to pg_extension I had tried to do that originally, e.g. instead of even bothering with such an extra arg, just mark it as not relocatable if the extension's script contains references to the required extension's schema. But then what if extfoo is upgraded? ALTER EXTENSION extfoo UPDATE; Wipes out the not relocatable of extfoo set. So in order to prevent that, I have to a) check the control files of all extensions that depend on foo to see if they made such a request. or b) "Seeing if the extension is marked as not relocatable, prevent ALTER EXTENSION from marking it as relocatable" problem with b is what if the extension author changed their mind and wanted it to be relocatable? Given the default is (not relocatable), it's possible the author didn't know this and later decided to put in an explicit relocate=false. c) define a new column in pg_extension to hold this bit of info. I was hoping I could reuse pg_extension.extconfig, but it seems that's hardwired to be only used for backup. Am I missing something or is this really as complicated as I think it is? If we go with b) I'm not sure why I need to bother defining a no_relocate, as it's obvious looking at the extension install/upgrade script that it should not be relocatable. > > So you are proposing I change the execute_extension_scripts input args > > to take more args? > > Why not? It's local to that file, so you won't break anything. > Okay, I wasn't absolutely sure if it was. If it is then I'll change. > regards, tom lane Thanks, Regina
RE: Ability to reference other extensions by schema in extension scripts
> "Regina Obe" writes: > > [ 0005-Allow-use-of-extschema-reqextname-to-reference.patch ] > > I took a look at this. I'm on board with the feature design, but not so much > with this undocumented restriction you added to ALTER EXTENSION SET > SCHEMA: > > + /* If an extension requires this extension > + * do not allow relocation */ > + if (pg_depend->deptype == DEPENDENCY_NORMAL && > pg_depend->classid == ExtensionRelationId){ > + dep.classId = pg_depend->classid; > + dep.objectId = pg_depend->objid; > + dep.objectSubId = pg_depend->objsubid; > + ereport(ERROR, > + > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + errmsg("cannot SET SCHEMA of > extension %s because other extensions require it", > + NameStr(extForm- > >extname)), > + errdetail("%s requires extension %s", > + > getObjectDescription(&dep, false), > +NameStr(extForm->extname; > > That seems quite disastrous for usability, and it's making an assumption > unsupported by any evidence: that it will be a majority use-case for > dependent extensions to have used @extschema:myextension@ in a way that > would be broken by ALTER EXTENSION SET SCHEMA. > > I think we should just drop this. It might be worth putting in some > documentation notes about the hazard, instead. > That was my thought originally too and also given the rarity of people changing schemas I wasn't that bothered with not forcing this. Sandro was a bit more bothered by not forcing it and given the default for extensions is not relocatable, we didn't see that much of an issue with it. > If you want to work harder, perhaps a reasonable way to deal with the issue > would be to allow dependent extensions to declare that they don't want your > extension relocated. But I do not think it's okay to make that the default > behavior, much less the only behavior. I had done that in one iteration of the patch. We discussed this here https://www.postgresql.org/message-id/01d949ad%241159adc0%24340d0940%24% 40pcorp.us and here https://www.postgresql.org/message-id/20230223183906.6rhtybwdpe37sri7%40c19 - the main issue I ran into is I have to introduce another dependency type or go with Sandro's idea of using refsubobjid for this purpose. I think defining a new dependency type is less likely to cause unforeseen complications elsewhere, but did require me to expand the scope (to make changes to pg_depend). Which I am fine with doing, but didn't want to over extend my reach too much. One of my revisions tried to use DEPENDENCY_AUTO which did not work (as Sandro discovered) and I had some other annoyances with lack of helper functions https://www.postgresql.org/message-id/000401d93a14%248647f540%2492d7dfc0%24% 40pcorp.us key point: "Why isn't there a variant getAutoExtensionsOfObject take a DEPENDENCY type as an option so it would be more useful or is there functionality for that I missed?" > And really, since we've gotten along without it so far, I'm not sure that it's > necessary to have it. > > Another thing that's bothering me a bit is the use of get_required_extension > in execute_extension_script. That does way more than you really need, and > passing a bunch of bogus parameter values to it makes me uncomfortable. > The callers already have the required extensions' OIDs at hand; it'd be better > to add that list to execute_extension_script's API instead of redoing the > lookups. > > regards, tom lane So you are proposing I change the execute_extension_scripts input args to take more args?
RE: Ability to reference other extensions by schema in extension scripts
> -Original Message- > From: Tom Lane [mailto:t...@sss.pgh.pa.us] > Sent: Friday, March 10, 2023 3:07 PM > To: Regina Obe > Cc: 'Gregory Stark (as CFM)' ; 'Sandro Santilli' > ; pgsql-hackers@lists.postgresql.org; 'Regina Obe' > > Subject: Re: Ability to reference other extensions by schema in extension > scripts > > "Regina Obe" writes: > > [ 0005-Allow-use-of-extschema-reqextname-to-reference.patch ] > > I took a look at this. I'm on board with the feature design, but not so much > with this undocumented restriction you added to ALTER EXTENSION SET > SCHEMA: > > + /* If an extension requires this extension > + * do not allow relocation */ > + if (pg_depend->deptype == DEPENDENCY_NORMAL && > pg_depend->classid == ExtensionRelationId){ > + dep.classId = pg_depend->classid; > + dep.objectId = pg_depend->objid; > + dep.objectSubId = pg_depend->objsubid; > + ereport(ERROR, > + > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + errmsg("cannot SET SCHEMA of > extension %s because other extensions require it", > + NameStr(extForm- > >extname)), > + errdetail("%s requires extension %s", > + > getObjectDescription(&dep, false), > +NameStr(extForm->extname; > > That seems quite disastrous for usability, and it's making an assumption > unsupported by any evidence: that it will be a majority use-case for > dependent extensions to have used @extschema:myextension@ in a way that > would be broken by ALTER EXTENSION SET SCHEMA. > > I think we should just drop this. It might be worth putting in some > documentation notes about the hazard, instead. > > If you want to work harder, perhaps a reasonable way to deal with the issue > would be to allow dependent extensions to declare that they don't want your > extension relocated. But I do not think it's okay to make that the default > behavior, much less the only behavior. > And really, since we've gotten along without it so far, I'm not sure that it's > necessary to have it. > > Another thing that's bothering me a bit is the use of get_required_extension > in execute_extension_script. That does way more than you really need, and > passing a bunch of bogus parameter values to it makes me uncomfortable. > The callers already have the required extensions' OIDs at hand; it'd be better > to add that list to execute_extension_script's API instead of redoing the > lookups. > > regards, tom lane
RE: [PATCH] Support % wildcard in extension upgrade filenames
> I wonder if a solution to this problem might be to provide some kind of a > version map file. Let's suppose that the map file is a bunch of lines of the > form > X Y Z, where X, Y, and Z are version numbers. The semantics could be: we (the > extension authors) promise that if you want to upgrade from X to Z, it > suffices > to run the script that knows how to upgrade from Y to Z. This would address > Tom's concern, because if you write a master upgrade script, you have to > explicitly declare the versions to which it applies. But it gives you a way > to do > that which does not require creating a bazillion empty files. Instead you can > just declare that if you're running the upgrade script from > 14.36.279 to 14.36.280, that also suffices for an upgrade from > 14.36.278 or 14.36.277 or 14.36.276 or > > -- > Robert Haas > EDB: http://www.enterprisedb.com That's what I was proposing as a compromise. Just not sure what the appropriate name would be for the map file. Originally I thought maybe we could put it in the .control, but decided it's better to have as a separate file that way we don't need to change the control and just add this extra file for PostgreSQL 16+. Then question arises if you have such a map file and you have files as well (the old way). Would we meld the two, so the map file would be used to simulate the file paths and these get added on as extra target paths or should we just assume if there is a map file, then that takes precedence over any paths inferred by files in the system. Thanks, Regina
RE: [PATCH] Support % wildcard in extension upgrade filenames
> I'm not unsympathetic to the idea of trying to support multiple upgrade paths > in one script. I just don't like this particular design for that, because it > requires the extension author to make promises that nobody is actually going > to deliver on. > > regards, tom lane How about the idea I mentioned, of we revise the patch to read versioned upgrades from the control file rather than relying on said file to exist. https://www.postgresql.org/message-id/000201d92572%247dcd8260%2479688720%24%40pcorp.us Even better, we have an additional control file, something like postgis--paths.control That has separate lines to itemize those paths. It would be nice if we could allow wild-cards in that, but I could live without that if we can stop shipping 300 empty files. Thanks, Regina
RE: [PATCH] Support % wildcard in extension upgrade filenames
> The thing that confuses me here is why the PostGIS folks are ending up with > so many files. > We certainly don't have that problem with the extension that > are being maintained in contrib, and I guess there is some difference in > versioning practice that is making it an issue for them but not for us. I > wish I > understood what was going on there. The contrib files are minor versioned. PostGIS is micro versioned. So we have for example postgis--3.3.0--3.3.1.sql Also we have 5 extensions we ship all micro versions, so multiply that issue by 5 postgis postgis_raster postgis_topology postgis_tiger_geocoder postgis_sfcgal The other wrinkle we have that I don't think postgresql's contrib have is that we support Each version across multiple PostgreSQL versions. So for example our 3.3 series supports PostgreSQL 12-15 (with plans to also support 16).
RE: Ability to reference other extensions by schema in extension scripts
> It looks like this patch needs a quick rebase, there's a conflict in the > meson.build. > > I'll leave the state since presumably this would be easy to resolve but it > would > be more likely to get attention if it's actually building cleanly. > > http://cfbot.cputube.org/patch_42_4023.log > -- > Gregory Stark > As Commitfest Manager Attach is the patch rebased against master. 0005-Allow-use-of-extschema-reqextname-to-reference.patch Description: Binary data
RE: Ability to reference other extensions by schema in extension scripts
> It looks like this patch needs a quick rebase, there's a conflict in the > meson.build. > > I'll leave the state since presumably this would be easy to resolve but it > would > be more likely to get attention if it's actually building cleanly. > > http://cfbot.cputube.org/patch_42_4023.log > > On Tue, 28 Feb 2023 at 18:44, Sandro Santilli wrote: > > > > 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 > > > > I've applied the patch attached to message > > https://www.postgresql.org/message- > id/000401d94bc8%2448dff700%24da9fe5 > > 00%24%40pcorp.us (md5sum a7d45a32c054919d94cd4a26d7d07c20) onto > > current tip of the master branch being > > 128dd9f9eca0b633b51ffcd5b0f798fbc48ec4c0 > > > > The review written in https://www.postgresql.org/message- > id/20230228224608.ak7br5shev4wic5a%40c19 all still applies. > > > > The `make installcheck-world` test fails for me but the failures seem > unrelated to the patch (many occurrences of "+ERROR: function > pg_input_error_info(unknown, unknown) does not exist" in the > regression.diff). > > > > Documentation exists for the new feature > > > > The new status of this patch is: Ready for Committer > > > > -- > Gregory Stark > As Commitfest Manager Just sent a note about the wildcard one. Was this conflicting with the wildcard one or some other. I can rebase if it was conflicting with another one, if it was the wildcard one, then maybe we should commit this one and we'll rebase the wildcard one. We would like to submit the wildcard one too, but I think Tom had some reservations on that one. Thanks, Regina
RE: [PATCH] Support % wildcard in extension upgrade filenames
> This patch too is conflicting on meson.build. Are these two patches > interdependent? > > This one looks a bit more controversial. I'm not sure if Tom has been > convinced and I haven't seen anyone else speak up. > > Perhaps this needs a bit more discussion of other options to solve this issue. > Maybe it can just be solved with multiple one-line scripts that call to a > master > script? > > -- > Gregory Stark > As Commitfest Manager They edit the same file yes so yes conflicts are expected. The wildcard one, Tom was not convinced, so I assume we'll need to go a couple more rounds on it and hopefully we can get something that will not be so controversial. I don't think the schema qualifying required extension feature is controversial though so I think that should be able to go and we'll rebase our other patch after that. Thanks, Regina
RE: Ability to reference other extensions by schema in extension scripts
> On Sun, Feb 26, 2023 at 01:39:24AM -0500, Regina Obe wrote: > > > > 1) Just don't allow any extensions referenced by other > > >extensions to be relocatable. > > > > Attached is my revision 3 patch, which follows the proposed #1. > > Don't allow schema relocation of an extension if another extension > > requires it. > > I've built a version of PostgreSQL with this patch applied and I confirm it > works as expected. > > The "ext1" is relocatable and creates a function ext1log(): > > =# create extension ext1 schema n1; > CREATE EXTENSION > > The "ext2" is relocatable and creates a function ext2log() relying on the > ext1log() function from "ext1" extension, referencing it via > @extschema:ext1@: > > =# create extension ext2 schema n2; > CREATE EXTENSION > =# select n2.ext2log('hello'); -- things work here > ext1: ext2: hello > > By creating "ext2", "ext1" becomes effectively non-relocatable: > > =# alter extension ext1 set schema n2; > ERROR: cannot SET SCHEMA of extension ext1 because other extensions > require it > DETAIL: extension ext2 requires extension ext1 > > Drop "ext2" makes "ext1" relocatable again: > > =# drop extension ext2; > DROP EXTENSION > =# alter extension ext1 set schema n2; > ALTER EXTENSION > > Upon re-creating "ext2" the referenced ext1 schema will be the correct one: > > =# create extension ext2 schema n1; > CREATE EXTENSION > =# select n1.ext2log('hello'); > ext1: ext2: hello > > The code itself builds w/out warnings with: > > mkdir build > cd build > ../configure > make 2> ERR # ERR is empty > > The testsuite reports all successes: > > make check > [...] > === >All 213 tests passed. > === > > Since I didn't see the tests for extension in there, I've also explicitly run that > portion: > > make -C src/test/modules/test_extensions/ check > [...] > test test_extensions ... ok 32 ms > test test_extdepend ... ok 12 ms > [...] > = >All 2 tests passed. > = > > > As mentioned already the downside of this patch is that it would not be > possibile to change the schema of an otherwise relocatable extension once > other extension depend on it, but I can't think of any good reason to allow > that, as it would mean dependent code would need to always dynamically > determine the install location of the objects in that extension, which sounds > dangerous, security wise. > > --strk; > > Libre GIS consultant/developer > https://strk.kbt.io/services.html Oops I had forgotten to submit the updated patch strk was testing against in my fork. He had asked me to clean up the warnings off list and the description. Attached is the revised. Thanks strk for the patient help and guidance. Thanks, Regina 0004-Allow-use-of-extschema-reqextname-to-reference.patch Description: Binary data
RE: Ability to reference other extensions by schema in extension scripts
> So in conclusion we have 3 possible paths to go with this > > 1) Just don't allow any extensions referenced by other extensions to be > relocatable. > It will show a message something like > "SET SCHEMA not allowed because other extensions depend on it" > Given that if you don't specify relocatable in you .control file, the assume is > relocatable = false , this isn't too far off from standard protocol. > > 2) Use objsubid=1 to denote that another extension explicitly references the > schema of another extension so setting schema of other extension is not okay. > So instead of introducing another dependency, we'd update the > DEPENDENCY_NORMAL one between the two schemas with objsubid=1 > instead of 0. > > This has 2 approaches: > > a) Update the existing DEPENDENCY_NORMAL between the two extensions > setting the objsubid=1 > > or > b) Create a new DEPEDENCY_NORMAL between the two extensions with > objsubid=1 > > I'm not sure if either has implications in backup / restore . I suspect b would > be safer since I suspect objsubid might be checked and this dependency only > needs checking during SET SCHEMA time. > > 3) Create a whole new DEPENDENCY type, perhaps calling it something like > DEPENDENCY_EXTENSION_SCHEMA > > 4) Just don't allow @extschema:@ syntax to be used unless > the is marked as relocatable=false. This one I don't like > because it doesn't solve my fundamental issue of > > postgis_tiger_geocoder relying on fuzzystrmatch, which is marked as > relocatable. > > The main issue I was trying to solve is my extension references fuzzystrmatch > functions in a function used for functional indexes, and this fails restore of > table indexes because I can't schema qualify the fuzzystrmatch extension in > the backing function. > > > If no one has any opinion, I'll go with option 1 which is the one that strk had > actually proposed before and seems least programmatically invasive, but > perhaps more annoying user facing. > > My preferred would be #2 > > Thanks, > Regina Attached is my revision 3 patch, which follows the proposed #1. Don't allow schema relocation of an extension if another extension requires it. 0003-Allow-use-of-extschema-reqextname-to-reference.patch Description: Binary data
RE: Ability to reference other extensions by schema in extension scripts
> On Mon, Feb 06, 2023 at 05:19:39AM -0500, Regina Obe wrote: > > > > Attached is a revised version of the original patch. It is revised to > > prevent > > > > ALTER EXTENSION .. SET SCHEMA if there is a dependent extension that > > references the extension in their scripts using > > @extschema:extensionname@ It also adds additional tests to verify that > new feature. > > > > In going thru the code base, I was tempted to add a new dependency > > type instead of using the existing DEPENDENCY_AUTO. I think this > > would be cleaner, but I felt that was overstepping the area a bit, > > since it requires making changes to dependency.h and dependency.c > > > > My main concern with using DEPENDENCY_AUTO is because it was designed > > for cases where an object can be dropped without need for CASCADE. In > > this case, we don't want a dependent extension to be dropped if it's > > required is dropped. However since there will already exist a > > DEPENDENCY_NORMAL between the 2 extensions, I figure we are protected > > against that issue already. > > I was thinking: how about using the "refobjsubid" to encode the "level" of > dependency on an extension ? Right now "refobjsubid" is always 0 when the > referenced object is an extension. > Could we consider subid=1 to mean the dependency is not only on the > extension but ALSO on it's schema location ? > I like that idea. It's only been ever used for tables I think, but I don't see why it wouldn't apply in this case as the concept is kinda the same. Only concern if other parts rely on this being 0. The other question, should this just update the existing DEPENDENCY_NORMAL extension or add a new DEPENDENCY_NORMAL between the extensions with subid=1? > Also: should we really allow extensions to rely on other extension w/out fully- > qualifying calls to their functions ? Or should it be discouraged and thus > forbidden ? If we wanted to forbid it we then would not need to encode any > additional dependency but rather always forbid `ALTER EXTENSION .. SET > SCHEMA` whenever the extension is a dependency of any other extension. > > On the code in the patch itself, I tried with this simple use case: > > - ext1, relocatable, exposes an ext1log(text) function > > - ext2, relocatable, exposes an ext2log(text) function > calling @extschema:ext1@.ext1log() > This would be an okay solution to me too if everyone is okay with it. > What is not good: > > - Drop of ext1 automatically cascades to drop of ext2 without even a > notice: > > test=# create extension ext2 cascade; > NOTICE: installing required extension "ext1" > CREATE EXTENSION > test=# drop extension ext1; > DROP EXTENSION -- no WARNING, no NOTICE, ext2 is gone > Oops. I don't know why I thought the normal dependency would protect against this. I should have tested that. So DEPENDENCY_AUTO is not an option to use and creating a new type of dependency seems like over stepping the bounds of this patch. > What is good: > > - ext1 cannot be relocated while ext2 is loaded: > > test=# create extension ext2 cascade; > NOTICE: installing required extension "ext1" > CREATE EXTENSION > test=# alter extension ext1 set schema n1; > ERROR: Extension can not be relocated because dependent > extension references it's location > test=# drop extension ext2; > DROP EXTENSION > test=# alter extension ext1 set schema n1; > ALTER EXTENSION > > --strk; > > Libre GIS consultant/developer > https://strk.kbt.io/services.html So in conclusion we have 3 possible paths to go with this 1) Just don't allow any extensions referenced by other extensions to be relocatable. It will show a message something like "SET SCHEMA not allowed because other extensions depend on it" Given that if you don't specify relocatable in you .control file, the assume is relocatable = false , this isn't too far off from standard protocol. 2) Use objsubid=1 to denote that another extension explicitly references the schema of another extension so setting schema of other extension is not okay. So instead of introducing another dependency, we'd update the DEPENDENCY_NORMAL one between the two schemas with objsubid=1 instead of 0. This has 2 approaches: a) Update the existing DEPENDENCY_NORMAL between the two extensions setting the objsubid=1 or b) Create a new DEPEDENCY_NORMAL between the two extensions with objsubid=1 I'm not sur
RE: Ability to reference other extensions by schema in extension scripts
> > > Here is first version of my patch using the > > > @extschema:extensionname@ syntax you proposed. > > > > > > This patch includes: > > > 1) Changes to replace references of @extschema:extensionname@ with > > > the schema of the required extension > > > 2) Documentation for the feature > > > 3) Tests for the feature. > > > Attached is a revised version of the original patch. It is revised to prevent ALTER EXTENSION .. SET SCHEMA if there is a dependent extension that references the extension in their scripts using @extschema:extensionname@ It also adds additional tests to verify that new feature. In going thru the code base, I was tempted to add a new dependency type instead of using the existing DEPENDENCY_AUTO. I think this would be cleaner, but I felt that was overstepping the area a bit, since it requires making changes to dependency.h and dependency.c My main concern with using DEPENDENCY_AUTO is because it was designed for cases where an object can be dropped without need for CASCADE. In this case, we don't want a dependent extension to be dropped if it's required is dropped. However since there will already exist a DEPENDENCY_NORMAL between the 2 extensions, I figure we are protected against that issue already. The issue I ran into is there doesn't seem to be an easy way of checking if a pg_depend record is already in place, so I ended up dropping it first with deleteDependencyRecordsForSpecific so I wouldn't need to check and then reading it. The reason for that is during CREATE EXTENSION it would need to create the dependency. It would also need to do so with ALTER EXTENSION .. UPDATE, since extension could later on add it in their upgrade scripts and so there end up being dupes after many ALTER EXTENSION UPDATE calls. pg_depends getAutoExtensionsOfObject seemed suited to that check, as is done in alter.c ExecAlterObjectDependsStmt /* Avoid duplicates */ currexts = getAutoExtensionsOfObject(address.classId, address.objectId); if (!list_member_oid(currexts, refAddr.objectId)) recordDependencyOn(&address, &refAddr, DEPENDENCY_AUTO_EXTENSION); but it is hard-coded to only check DEPENDENCY_AUTO_EXTENSION Why isn't there a variant getAutoExtensionsOfObject take a DEPENDENCY type as an option so it would be more useful or is there functionality for that I missed? Thanks, Regina 0002-Allow-use-of-extschema-reqextname-to-reference.patch Description: Binary data
RE: Ability to reference other extensions by schema in extension scripts
> On Mon, Jan 16, 2023 at 11:57:30PM -0500, Regina Obe wrote: > > > What would be more bullet-proof is having an extra column in > > pg_extension or adding an extra array element to > > pg_extension.extcondition[] that allows you to say "Hey, don't allow > > this to be relocatable cause other extensions depend on it that have explicitly > referenced the schema." > > I've given this some more thoughts and I think a good compromise could be to > add the safety net in ALTER EXTESION SET SCHEMA so that it does not only > check "extrelocatable" but also the presence of any extension effectively > depending on it, in which case the operation could be prevented with a more > useful message than "extension does not support SET SCHEMA" (what is > currently output). > > Example query to determine those cases: > > SELECT e.extname, array_agg(v.name) > FROM pg_extension e, pg_available_extension_versions v > WHERE e.extname = ANY( v.requires ) > AND e.extrelocatable > AND v.installed group by e.extname; > > extname|array_agg > ---+-- >fuzzystrmatch | {postgis_tiger_geocoder} > > --strk; The only problem with the above is then it bars an extension from being relocated even if no extensions reference their schema. Note you wouldn't be able to tell if an extension references a schema without analyzing the install script. Which is why I was thinking another property would be better, cause that could be checked during the install/upgrade of the dependent extensions. I personally would be okay with this and is easier to code I think and doesn't require structural changes, but not sure others would be as it's taking away permissions they had before when it wasn't necessary to do so. Thanks, Regina
RE: Ability to reference other extensions by schema in extension scripts
> > On Thu, Dec 15, 2022 at 08:04:22AM -0500, Regina Obe wrote: > > > On Tue, Nov 22, 2022 at 11:24:19PM -0500, Regina Obe wrote: > > > > > > > If an extension is required by another extension and that required > > > > extension schema is referenced in the extension scripts using the > > > > @extschema:extensionname@ syntax, then ideally we should prevent > > > > the required extension from being relocatable. This would prevent > > > > a user from accidentally moving the required extension, thus > > > > breaking the dependent extensions. > > > > > > > > I didn't add that feature cause I wasn't sure if it was > > > > overstepping the bounds of what should be done, or if we leave it > > > > up to the user to just know better. > > > > > > An alternative would be to forbid using @extschema:extensionname@ to > > > reference relocatable extensions. DBA can toggle relocatability of > > > an extension to allow it to be referenced. > > > > That would be hard to do in a DbaaS setup and not many users know they > > can fiddle with extension control files. > > Plus those would get overwritten with upgrades. > > Wouldn't this also be the case if you override relocatability ? > Case: > > - Install fuzzystrmatch, marked as relocatable > - Install ext2 depending on the former, which is them marked > non-relocatable > - Upgrade database -> fuzzystrmatch becomes relocatable again > - Change fuzzystrmatch schema BREAKING ext2 > Somewhat. It would be an issue if someone does ALTER EXTENSION fuzzystrmatch UPDATE; And ALTER EXTENSION fuzzystrmatch SET SCHEMA a_different_schema; Otherwise the relocatability of an already installed extension wouldn't change even during upgrade. I haven't checked pg_upgrade, but I suspect it wouldn't change there either. It's my understanding that once an extension is installed, it's relocatable status is recorded in the pg_extension table. So it doesn't matter at that point what the control file says. However if someone does update the extension, then yes it would look at the control file and make it updatable again. I just tested this fiddling with postgis extension and moving it and then upgrading. UPDATE pg_extension SET extrelocatable = true where extname = 'postgis'; ALTER EXTENSION postgis SET schema postgis; ALTER EXTENSION postgis UPDATE; e.g. if the above is already at latest version, get notice NOTICE: version "3.3.2" of extension "postgis" is already installed (and the extension is still relocatable) -- if the extension can be upgraded ALTER EXTENSION postgis UPDATE; -- no longer relocatable (because postgis control file has relocatable = false) But honestly I don't expect this to be a huge issue, more of just an extra safety block. Not a bullet-proof safety block though. > Allowing to relocate a dependency of other extensions using the > @extschema@ syntax is very dangerous. > > I've seen that PostgreSQL itself doesn't even bother to replace @extschema@ > IF the extension using it doesn't mark itself as non-relocatable. For consistency > this patch should basically refuse to expand @extschema:fuzzystrmatch@ if > "fuzzystrmatch" extension is relocatable. > > Changing the current behaviour of PostgreSQL could be proposed but I don't > think it's to be done in this thread ? > > So my suggestion is to start consistent (do not expand if referenced extension > is relocatable). > > > --strk; I don't agree. That would make this patch of not much use. For example lets revisit my postgis_tiger_geocoder which is a good bulk of the reason why I want this. I use indexes that use postgis_tiger_geocoder functions that call fuzzystrmatch which causes pg_restore to break on reload and other issues, because I'm not explicitly referencing the function schema. With your proposal now I got to demand the postgresql project to make fuzzystrmatch not relocatable so I can use this feature. It is so rare for people to go around moving the locations of their extensions once set, that I honestly don't think the ALTER EXTENSION .. UPDATE hole is a huge deal. I'd be more annoyed having to beg an extension provider to mark their extension as not relocatable so that I could explicitly reference the location of their extensions. And even then - think about it. I ask extension provider to make their extension schema relocatable. They do, but some people are using a version before they marked it as schema relocatable. So now if I change my code, users can't install my extension, cause they are using a version before it was schema relocatable and I'm using the new syntax. What would be more bullet-proof is having an extra column in pg_extension or adding an extra array element to pg_extension.extcondition[] that allows you to say "Hey, don't allow this to be relocatable cause other extensions depend on it that have explicitly referenced the schema." Thanks, Regina
RE: [PATCH] Support % wildcard in extension upgrade filenames
> Sandro Santilli writes: > > On Mon, Jan 09, 2023 at 05:51:49PM -0500, Tom Lane wrote: > >> ... you still need one script file for each supported upgrade step > > > That's exactly the problem we're trying to solve here. > > The include support is nice on itself, but won't solve our problem. > > The script-file-per-upgrade-path aspect solves a problem that you have, > whether you admit it or not; I think you simply aren't realizing that because > you have not had to deal with the consequences of your proposed feature. > Namely that you won't have any control over what the backend will try to do in > terms of upgrade paths. > It would be nice if there were some way to apply at least a range match to upgrades or explicitly state in the control file what paths should be applied for an upgrade. Ultimately as Sandro mentioned, it's the 1000 files issue we are trying to address. The only way we can fix that in the current setup, is to move to a minor version mode which means we can never do micro updates. > As an example, suppose that a database has foo 4.0 installed, and the DBA > decides to try to downgrade to 3.0. With the system as it stands, if you've > provided foo--4.0--3.0.sql then the conversion will go through, and presumably > it will work because you tested that that script does what it is intended to. If > you haven't provided any such downgrade script, then ALTER EXTENSION > UPDATE will say "Sorry Dave, I'm afraid I can't do that" and no harm is done. > In our case we've already addressed that in our script, because our upgrades don't rely on what extension model tells us is the version, ultimately what our postgis..version() tells us is the true determinate (one for the lib file and one for the script). But you are right, that's a selfish way of thinking about it, cause we have internal plumbing to handle that issue already and other projects probably don't. What I was hoping for was to having a section in the control file to say something like Upgrade patterns: 3.0.%--4.0.0.sql, 2.0.%--4.0.0.sql (and perhaps some logic to guarantee no way to match two patterns) So you wouldn't be able to have a set of patterns like Upgrade patterns: %--4.0.0.sql, 3.0.%--4.0.0.sql, 2.0.%--4.0.0.sql That would allow your proposed include something or other to work and for us to be able to use that, and still reduce our file footprint. I'd even settle for absolute paths stated in the control file if we can dispense with the need a file path to push you forward. In that mode, your downgrade issue would never happen even with the way people normally create scripts now. > So I really think this is a case of "be careful what you ask for, you might get it". > Even if PostGIS is willing to put in the amount of infrastructure legwork needed > to make such a design bulletproof, I'm quite sure nobody else will manage to > use such a thing successfully. I'd rather spend our development effort on a > feature that has more than one use-case. > > regards, tom lane I think you are underestimating the number of extensions that have this issue and could benefit (agree not in the current incarnation of the patch). It's not just PostGIS, it's pgRouting (has 56 files), h3 extension (has 37 files in last release, most of them a do nothing, except at the minor update steps) that have the same issue (and both pgRouting and h3 do have little bitty script updates that follows the best practice way of doing this). For pgRouting alone there are 56 files for the last release (of which it can easily be reduced to about 5 files if the paths could be explicitly stated in the control file). Yes all those extensions should dispense with their dreams of having micro updates (I honestly wish they would). Perhaps I'm a little obsessive, but it annoys me to see 1000s of files in my extension folder, when I know even if I followed best practice I only need like 5 files for each extension. And as a packager to have to ship these files is even more annoying. The reality is for micros, no one ships new functions (or at least shouldn't be), so all functions just need to be replaced perhaps with a micro update file you propose. Heck if we could even have the option to itemize our own upgrade file paths explicitly in the control file, Like: paths: 3.0.1--4.0.0 = 3--4.0.0.sql, 3.0.2--4.0.0 = 3--4.0.0.sql, 2--4.0.0.sql = 2.0.2--4.0.0.sql I'd be happy, and PostgreSQL can do the math pretending there are files it thinks it needs. So if we could somehow rework this patch perhaps adding something in the control to explicitly state the upgrade paths. I think that would satisfy your concern? And I'd be eternally grateful. Thanks, Regina
RE: [PATCH] Support % wildcard in extension upgrade filenames
> I continue to think that this is a fundamentally bad idea. It creates all sorts of > uncertainties about what is a valid update path and what is not. Restrictions > like > > + Such wildcard update > + scripts will only be used when no explicit path is found from > + old to target version. > > are just band-aids to try to cover up the worst problems. > > Have you considered the idea of instead inventing a "\include" facility for > extension scripts? Then, if you want to use one-monster-script to handle > different upgrade cases, you still need one script file for each supported > upgrade step, but those can be one-liners including the common script file. > Plus, such a facility could be of use to people who want intermediate > factorization solutions (that is, some sharing of code without buying all the > way into one-monster-script). > > regards, tom lane The problem with an include is that it does nothing for us. We still need to ship a ton of script files. We've already dealt with the file size issue. So our PostGIS 3.4.0+ (not yet released) already kind of simulates include using blank script files that have nothing in them but forces the extension machinery to upgrade the version to ANY to get to the required installed version 3.4.0+ So for example postgis--3.3.0--ANY.sql Would contain this: -- Just tag extension postgis version as "ANY" -- Installed by postgis 3.4.0dev -- Built on ... And the file has the upgrade steps: postgis--ANY--3.4.0dev.sql So that when you are on 3.3.0 and want to upgrade to 3.4.0dev ( it takes 3.3.0 -> ANY -> 3.4.0dev) The other option I had proposed was getting rid of the micro version, but I got shot down on that argument -- with PostGIS PSC complaining about people not being able to upgrade a micro if per chance we have some security patch released in a micro. https://lists.osgeo.org/pipermail/postgis-devel/2022-June/029673.html https://lists.osgeo.org/pipermail/postgis-devel/2022-July/029713.html Thanks, Regina
RE: Ability to reference other extensions by schema in extension scripts
> On Tue, Nov 22, 2022 at 11:24:19PM -0500, Regina Obe wrote: > > > Here is first version of my patch using the @extschema:extensionname@ > > syntax you proposed. > > > > This patch includes: > > 1) Changes to replace references of @extschema:extensionname@ with the > > schema of the required extension > > 2) Documentation for the feature > > 3) Tests for the feature. > > > > There is one issue I thought about that is not addressed by this. > > > > If an extension is required by another extension and that required > > extension schema is referenced in the extension scripts using the > > @extschema:extensionname@ syntax, then ideally we should prevent the > > required extension from being relocatable. This would prevent a user > > from accidentally moving the required extension, thus breaking the > > dependent extensions. > > > > I didn't add that feature cause I wasn't sure if it was overstepping > > the bounds of what should be done, or if we leave it up to the user to > > just know better. > > An alternative would be to forbid using @extschema:extensionname@ to > reference relocatable extensions. DBA can toggle relocatability of an extension > to allow it to be referenced. > > --strk; That would be hard to do in a DbaaS setup and not many users know they can fiddle with extension control files. Plus those would get overwritten with upgrades. In my case for example I have postgis_tiger_geocoder that relies on both postgis and fuzzystrmatch. I'd rather not have to explain to users how to fiddle with the fuzzystrmatch.control file to make it not relocatable. But I don't think anyone would mind if it's forced after install because it's a rare thing for people to be moving extensions to different schemas after install. Thanks, Regina
RE: Ability to reference other extensions by schema in extension scripts
> > "Regina Obe" writes: > >> I have a distinct sense of deja vu here. I think this idea, or > >> something isomorphic to it, was previously discussed with some other > syntax details. > > > I found the old discussion I recalled having and Stephen had suggested > > using @extschema{'postgis'}@ On this thread -- > > https://www.postgresql.org/message- > id/20160425232251.GR10850@tamriel.s > > nowman.net > > Is that the one you remember? > > Hmmm ... no, ISTM it was considerably more recent than that. > [ ...digs... ] Here we go, it was in the discussion around converting contrib SQL > functions to new-style: > > https://www.postgresql.org/message- > id/flat/3395418.1618352794%40sss.pgh.pa.us > > There are a few different ideas bandied around in there. > Personally I still like the @extschema:extensionname@ option the best, > though. > > regards, tom lane Here is first version of my patch using the @extschema:extensionname@ syntax you proposed. This patch includes: 1) Changes to replace references of @extschema:extensionname@ with the schema of the required extension 2) Documentation for the feature 3) Tests for the feature. There is one issue I thought about that is not addressed by this. If an extension is required by another extension and that required extension schema is referenced in the extension scripts using the @extschema:extensionname@ syntax, then ideally we should prevent the required extension from being relocatable. This would prevent a user from accidentally moving the required extension, thus breaking the dependent extensions. I didn't add that feature cause I wasn't sure if it was overstepping the bounds of what should be done, or if we leave it up to the user to just know better. Thanks, Regina 0001-Allow-use-of-extschema-reqextname-to-reference.patch Description: Binary data
Re: [PATCH] Support % wildcard in extension upgrade filenames
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: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.
RE: [PATCH] Support % wildcard in extension upgrade filenames
> On Sun, Nov 13, 2022 at 11:46:50PM -0500, Regina Obe wrote: > > > 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 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 > > > > > > 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. > > > > 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. > > 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
RE: [PATCH] Support % wildcard in extension upgrade filenames
> 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 > > 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 > > 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 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
RE: Ability to reference other extensions by schema in extension scripts
> "Regina Obe" writes: > >> I have a distinct sense of deja vu here. I think this idea, or > >> something isomorphic to it, was previously discussed with some other > syntax details. > > > I found the old discussion I recalled having and Stephen had suggested > > using @extschema{'postgis'}@ On this thread -- > > https://www.postgresql.org/message- > id/20160425232251.GR10850@tamriel.s > > nowman.net > > Is that the one you remember? > > Hmmm ... no, ISTM it was considerably more recent than that. > [ ...digs... ] Here we go, it was in the discussion around converting contrib SQL > functions to new-style: > > https://www.postgresql.org/message- > id/flat/3395418.1618352794%40sss.pgh.pa.us > > There are a few different ideas bandied around in there. > Personally I still like the @extschema:extensionname@ option the best, > though. > > regards, tom lane I had initially thought of a syntax that could always be used even outside of extension install as some mentioned. Like the PG_EXTENSION_SCHEMA(cube) example. Main benefit I see with that is that even if an extension is moved, all the dependent extensions that reference it would still work fine. I had dismissed that because it seemed too invasive. Seems like it would require changes to the parser and possibly add query performance overhead to resolve the schema. Not to mention the added testing required to do no harm. The other reason I dismissed it is because at least for PostGIS it would be harder to conditionally replace. The issue with PG_EXTENSION_SCHEMA(cube) is we can't support that in lower PG versions so we'd need to strip for lower versions, and that would introduce the possibility of missing PG_EXTENSION_SCHEMA(cube) vs. PG_EXTENSION_SCHEMA( cube ), not a huge deal though, but not quite as easy and precise as just stripping @extschema:extensionname@. References. With the @extschema:extensionname@, it doesn't solve all problems, but the key ones we care about like breakage of functions used in indexes, materialized views, and added security and is a little easier to strip out. I'll work on producing a patch. Thanks, Regina
Re: [PATCH] Support % wildcard in extension upgrade filenames
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
RE: Ability to reference other extensions by schema in extension scripts
> "Regina Obe" writes: > > My proposal is this. If you think it's a good enough idea I can work > > up a patch for this. > > Extensions currently are allowed to specify a requires in the control file. > > I propose to use this information, to allow replacement of phrases > > @extschema_nameofextension@ as a variable, where nameofextension has > > to be one of the extensions listed in the requires. > > I have a distinct sense of deja vu here. I think this idea, or something > isomorphic to it, was previously discussed with some other syntax details. > I'm too lazy to go searching the archives right now, but I suggest that you try to > find that discussion and see if the discussed syntax seems better or worse than > what you mention. > > I think it might've been along the line of @extschema:nameofextension@, > which seems like it might be superior because colon isn't a valid identifier > character so there's less risk of ambiguity. > > regards, tom lane I found the old discussion I recalled having and Stephen had suggested using @extschema{'postgis'}@ On this thread -- https://www.postgresql.org/message-id/20160425232251.GR10850@tamriel.snowman .net Is that the one you remember? Thanks, Regina
Ability to reference other extensions by schema in extension scripts
I think I had brought this up a while ago, but I forget what the opinion was on the matter. PostGIS has a number of extensions that rely on it. For the extensions that are packaged with PostGIS, we force them all into the same schema except for the postgis_topology and postgis_tiger_geocoder extensions which are already installed in dedicated schemas. This makes it impossible for postgis_topology and postgis_tiger_geocoder to schema qualify their use of postgis. Other extensions like pgRouting, h3-pg, mobilitydb have similar issues. My proposal is this. If you think it's a good enough idea I can work up a patch for this. Extensions currently are allowed to specify a requires in the control file. I propose to use this information, to allow replacement of phrases @extschema_nameofextension@ as a variable, where nameofextension has to be one of the extensions listed in the requires. The extension plumbing will then use this information to look up the schema that the current required extensions are installed in, and replace the variables with the schema of where the dependent extension is installed. Does anyone see any issue with this idea. Thanks, Regina
RE: [PATCH] Support % wildcard in extension upgrade filenames
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) > > --strk; > 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
RE: [PATCH] Support % wildcard in extension upgrade filenames
> 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 [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
substring odd behavior
Is this intentional behavior? -- I can do this SELECT substring('3.2.0' from '[0-9]*\.([0-9]*)\.'); -- But can't do this gives error syntax error at or near "from" SELECT pg_catalog.substring('3.2.0' from '[0-9]*\.([0-9]*)\.'); -- but can do SELECT pg_catalog.substring('3.2.0', '[0-9]*\.([0-9]*)\.'); Thanks, Regina
RE: [postgis-devel] About EXTENSION from UNPACKAGED on PostgreSQL 13
> Presumably 15 years out from the 1.x -> 2.x we can stop worrying about > bundling unpackaged postgis into an extension, and just recommend a hard > upgrade dump/restore to the hardy souls still running 1.x. > > P. > We don't need to worry about 1.x cause 1.x can only do a hard upgrade to 2 or 3. We never supported soft upgrade from 1.x Easy solution there is just to install postgis extension and do pg_restore/postgis_restore of your data. So it's really just the 2.1 -> 3 that are of concern. I think now is a fine time to encourage everyone to upgrade to 3 if they can so they don't need to suffer any crazy solutions we come up with :) Turn this into a convenient emergency. Thanks, Regina
Re: Compressed TOAST Slicing
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:not tested No need for documentation as this is a performance improvement patch I tested on windows mingw64 (as of a week ago) and confirmed the patch applies cleanly and significantly faster for left, substr tests than head.
RE: CTE Changes in PostgreSQL 12, can we have a GUC to get old behavior
> I think there are probably other ways of fixing this query that won't have > such dramatic effects; it doesn't really seem to need to use WITH, and I bet > you could also tweak the WITH query to prevent inlining. Yes I know I can change THIS QUERY. I've changed other ones to work around this. Normally I just use a LATERAL for this. My point is lots of people use CTEs intentionally for this kind of thing because they know they are materialized. It's going to make a lot of people hesitant to upgrade if they think they need to revisit every CTE (that they intentionally wrote cause they thought it would be materialized) to throw in a MATERIALIZED keyword. > I also think > Andres's question about why this gets inlined in the first place is a good > one; > the (m).* seems like it ought to be counted as a multiple reference. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL > Company Well if we can at least prevent the multiple reference thing from inlining that might be good enough to solve most performance regression issues that arise. Thanks, Regina
CTE Changes in PostgreSQL 12, can we have a GUC to get old behavior
The CTE change in PostgreSQL 12 broke several of PostGIS regression tests because many of our tests are negative tests that test to confirm we get warnings in certain cases. In the past, these would output 1 notice because the CTE was materialized, now they output 1 for each column. An example is as follows: WITH data AS ( SELECT '#2911' l, ST_Metadata(ST_Rescale( ST_AddBand( ST_MakeEmptyRaster(10, 10, 0, 0, 1, -1, 0, 0, 0), 1, '8BUI', 0, 0 ), 2.0, -2.0 )) m ) SELECT l, (m).* FROM data; In prior versions this raster test would return one notice: NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API Now it returns 10 notices because the call is being done 10 times (1 for each column) NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API NOTICE: Raster has default geotransform. Adjusting metadata for use of GDAL Warp API The regression errors are easy enough to fix with OFFSET or subquery. What I'm more concerned about is that I expect we'll have performance degradation. Historically PostGIS functions haven't been costed right and can't be because they rely on INLINING of sql functions which gets broken when too high of cost is put on functions. We have a ton of functions like these that return composite objects and this above function is particularly expensive so to have it call that 10 times is almost guaranteed to be a performance killer. I know there is a new MATERIALIZED keyword to get the old behavior, but people are not going to be able to change their apps to introduce new keywords, especially ones meant to be deployed by many versions of PostgreSQL. That said IS THERE or can there be a GUC like set cte_materialized = on; to get the old behavior? Thanks, Regina PostGIS PSC member
RE: "interesting" issue with restore from a pg_dump with a database-wide search_path
> > > a) In this particular case, I have a function that uses fuzzystrmatch and is > used in functional indexes. > > I unfortunately can't schema qualify the use of soundex, because I > > don't know where the user may have installed fuzzystrmatch is > > installed > > b) Stephen Frost had suggested, perhaps we should have some syntax like > @extension_loc(fuzzystrmatch)...@ so that one could reference an extension > dependency location within a function without knowing where it is installed. > > You don't really need any new syntax for this particular case, I think. > You can declare the function in the extension like this: > > create function ... set search_path from current; > > which will cause it to absorb the search path that's set while running the > extension script, which should be what you want. > > regards, tom lane But then the search_path would be local variable to the function. Wouldn't that impact performance? We had originally tried that in PostGIS functions (well not that but explicitly setting the functions local search path to where postgis is installed by adding a search_path variable to the function) And things got 10 times slower.
RE: "interesting" issue with restore from a pg_dump with a database-wide search_path
> From: Paul Ramsey [mailto:pram...@cleverelephant.ca] > Sent: Monday, July 23, 2018 2:42 PM > To: Regina Obe > Subject: Fwd: "interesting" issue with restore from a pg_dump with a > database-wide search_path > > Seen this one? > P > > > -- Forwarded message -- > From: Tom Lane > Date: Fri, Jul 6, 2018 at 1:10 PM > Subject: Re: "interesting" issue with restore from a pg_dump with a > database-wide search_path > To: Larry Rosenman > Cc: "Joshua D. Drake" , pgsql- > hack...@lists.postgresql.org > > > Larry Rosenman writes: > > On Fri, Jul 06, 2018 at 11:35:41AM -0700, Joshua D. Drake wrote: > >> Knowing the errors would be helpful. > > > pg_restore: [archiver (db)] Error while PROCESSING TOC: > > pg_restore: [archiver (db)] Error from TOC entry 12; 3079 887963 > > EXTENSION postgis_tiger_geocoder > > pg_restore: [archiver (db)] could not execute query: ERROR: function > > soundex(character varying) does not exist > > HINT: No function matches the given name and argument types. You > might need to add explicit type casts. > > This looks like a problem with the postgis_tiger_geocoder extension. > It's depending on the fuzzystrmatch extension (which has the soundex > function), but seemingly this dependency is not declared in the extension's > control file. If it were, the search path would've been set to include the > schema of the fuzzystrmatch extension during CREATE EXTENSION. > > regards, tom lane [Regina Obe] Sorry for not posting from the thread. Paul alerted me to this one and I am aware of the issue. 1) I do have fuzzstrmatch listed as a dependency in the control file. I know because I often install the geocoder with CREATE EXTENSION postgis_tiger_geocoder CASCADE; And it installs postgis and fuzzystrmatch 2) I have brought this issue up before and that's why we in fact had to schema qualify all postgis functions cause even with postgis within the same extension, things like materialized views fail to load. 3) My guess as to how this happens a) In this particular case, I have a function that uses fuzzystrmatch and is used in functional indexes. I unfortunately can't schema qualify the use of soundex, because I don't know where the user may have installed fuzzystrmatch is installed b) Stephen Frost had suggested, perhaps we should have some syntax like @extension_loc(fuzzystrmatch)...@ so that one could reference an extension dependency location within a function without knowing where it is installed.
RE: Regression on PostgreSQL 10 ORDER/GROUP BY expression not found in targetlist
> "Regina Obe" writes: > > Here is a trivial example to exercise that doesn't involve PostGIS. > > > CREATE TABLE edge AS SELECT 1 AS edge_id; SELECT 't3280', 'L1b' || > > generate_series(1,10) FROM edge ORDER BY 1; > > That's odd. It works fine in HEAD but not the v10 branch. I suspect that the > fix David mentioned failed to cover some case related to the issue we saw > last week ... but why the cross-version difference? > > Don't have time to investigate further right now. > > regards, tom lane Yes, that concerned me too because our PostgreSQL 11 head is the first to run in our battery of tests So I would have expected any change in 10 would be committed in 11 as well and 11 would fail too, but it didn't. Thanks, Regina
RE: Regression on PostgreSQL 10 ORDER/GROUP BY expression not found in targetlist
> On 29 June 2018 at 21:06, Regina Obe wrote: > > I think the exact query that is triggering it is this one though I > > don't have PostgreSQL 10 head installed locally to confirm. > > > > SELECT 't3280', 'L1b' || topology.TopoGeo_AddLinestring('bug3280', > > geom) FROM bug3280.edge where edge_id = 1 ORDER BY 1; > > Can you try in REL_10_STABLE? There was a fix committed [1] for something > very similar just a week ago. > > [1] > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=a4c95b0b80 > c70677c09c0d5c82a6fba875160288 > > -- > David Rowley http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services Thanks for this input. I guess I was mistaken about the commit that caused this and it's most likely this one since this one fails as well. Here is a trivial example to exercise that doesn't involve PostGIS. CREATE TABLE edge AS SELECT 1 AS edge_id; SELECT 't3280', 'L1b' || generate_series(1,10) FROM edge ORDER BY 1; Now that said, I think I should change our test since it's using questionable assumptions anyway. My only concern with this issue is that it's behavior that was allowed before in 10 a stable branch and it's changing in a micro release. I doubt few others would be bitten by though, but probably still worthwhile to put a note about it in the upcoming release notes if this new behavior is kept. Thanks, Regina Obe
Regression on PostgreSQL 10 ORDER/GROUP BY expression not found in targetlist
On June 26th, our PostGIS debian bot started failing on PostgreSQL 10 with one of our regression tests for topology. It started erroring out with ORDER/GROUP BY expression not found in targetlist Related ticket here: https://trac.osgeo.org/postgis/ticket/4111 I think the exact query that is triggering it is this one though I don't have PostgreSQL 10 head installed locally to confirm. SELECT 't3280', 'L1b' || topology.TopoGeo_AddLinestring('bug3280', geom) FROM bug3280.edge where edge_id = 1 ORDER BY 1; This bot follows the dev, and stable git branches of PostgreSQL, and it was on PostgreSQL 10 that it started failing. PostgreSQL 11 was okay. I'm assuming this is an unintended consequence of this commit https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3566873f2 1909397561418fab22c98332fa8b72a as that's when the issue started happening. Just want to confirm that and not something in our code we've just been doing incorrectly. If needed I'll try to put together an example that doesn't involve PostGIS to exercise the problem. -- more details from the commit and it seemed like lots of files were changed Add PGTYPESchar_free() to avoid cross-module problems on Windows. (details) Commit 3566873f21909397561418fab22c98332fa8b72a by tmunro https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3566873f2 1909397561418fab22c98332fa8b72a Add PGTYPESchar_free() to avoid cross-module problems on Windows. On Windows, it is sometimes important for corresponding malloc() and free() calls to be made from the same DLL, since some build options can result in multiple allocators being active at the same time. For that reason we already provided PQfreemem(). This commit adds a similar function for freeing string results allocated by the pgtypes library. Author: Takayuki Tsunakawa Reviewed-by: Kyotaro Horiguchi Discussion: https://postgr.es/m/0A3221C70F24FB45833433255569204D1F8AD5D6%40G01JPEXMBYT05 Thanks, Regina
MemoryContextCreate change in PG 11 how should contexts be created
On December 13th this change to context creation was committed, which broke PostGIS trunk compile against PostgreSQL 11 head. https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=9fa6f00b1308d d10da4eca2f31ccbfc7b35bb461 Ticketed in PostGIS here: https://trac.osgeo.org/postgis/ticket/3946 It's been broken for a couple of months https://trac.osgeo.org/postgis/ticket/3904 with just core dumping but at least it used to compile before December 13th. In going thru the changes I see that MemoryContextCreate was changed to not return a context anymore and to take in pointer to the context that should be returned among other changes. I also notice that MemoryContextCreate seems to be rarely used in PostgreSQL code in places I would expect and instead AllocSetContextCreateExtended is used. So is the preferred approach to not use MemoryContextCreate in extensions and instead for PG < 11 use AllocSetContextCreate and PG >= use AllocSetContextCreateExtended? Sorry if my questions don't make any sense. Still trying to grapple with all these memory context functions and how each is different from the other. For reference, one of the slices of code we have in place that broke looks something like this and we've got I think at least 5 more such instances sprinkled in PostGIS code base. https://git.osgeo.org/gitea/postgis/postgis/src/branch/svn-trunk/libpgcommon /lwgeom_transform.c#L550 MemoryContext PJMemoryContext; POSTGIS_DEBUGF(3, "adding SRID %d with proj4text \"%s\" to query cache at index %d", srid, proj_str, PROJ4Cache->PROJ4SRSCacheCount); PJMemoryContext = MemoryContextCreate(T_AllocSetContext, 8192, &PROJ4SRSCacheContextMethods, PROJ4Cache->PROJ4SRSCacheContext, "PostGIS PROJ4 PJ Memory Context"); Thanks, Regina