RE: Can't compile PG 17 (master) from git under Msys2 autoconf

2024-04-05 Thread Regina Obe
> > 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

2024-04-05 Thread Regina Obe
> > 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

2024-04-04 Thread Regina Obe
> > 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

2024-04-04 Thread Regina Obe
> 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

2024-04-03 Thread Regina Obe
> 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

2024-04-02 Thread Regina Obe
> 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

2024-03-27 Thread Regina Obe
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

2024-03-27 Thread Regina Obe
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

2023-08-01 Thread Regina Obe
> > 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

2023-05-01 Thread Regina Obe
> 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

2023-04-28 Thread Regina Obe
> 
> > 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

2023-04-24 Thread Regina Obe
> 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

2023-04-21 Thread Regina Obe
> > 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

2023-04-21 Thread Regina Obe
> 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

2023-04-21 Thread Regina Obe
> > 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

2023-04-21 Thread Regina Obe
> 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

2023-04-21 Thread Regina Obe
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

2023-04-13 Thread Regina Obe
> 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

2023-04-13 Thread Regina Obe
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

2023-04-11 Thread Regina Obe
> 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

2023-04-11 Thread Regina Obe
> 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

2023-04-10 Thread Regina Obe
> 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

2023-03-21 Thread Regina Obe
> 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

2023-03-13 Thread Regina Obe
> 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

2023-03-13 Thread Regina Obe
> > 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

2023-03-13 Thread Regina Obe
> 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

2023-03-11 Thread Regina Obe
> 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

2023-03-10 Thread Regina Obe
> "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

2023-03-10 Thread Regina Obe
> 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

2023-03-10 Thread Regina Obe
> "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(, 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

2023-03-10 Thread Regina Obe



> -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(, 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

2023-03-08 Thread Regina Obe
> 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

2023-03-07 Thread Regina Obe
> 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

2023-03-07 Thread Regina Obe
> 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

2023-03-06 Thread Regina Obe
> 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

2023-03-06 Thread Regina Obe
> 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

2023-03-06 Thread Regina Obe
> 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

2023-02-28 Thread Regina Obe

> 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

2023-02-25 Thread Regina Obe
> 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

2023-02-25 Thread Regina Obe
> 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 sure if either has implications in backup / restore .  I suspect b
w

RE: Ability to reference other extensions by schema in extension scripts

2023-02-06 Thread Regina Obe
> > > 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(, ,
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

2023-01-18 Thread Regina Obe
> 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

2023-01-16 Thread Regina Obe
> 
> 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

2023-01-10 Thread Regina Obe
> 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

2023-01-09 Thread Regina Obe
> 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

2022-12-15 Thread Regina Obe
> 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

2022-11-22 Thread Regina Obe
> 
> "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

2022-11-22 Thread Regina Obe
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

2022-11-16 Thread Regina Obe
> 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

2022-11-13 Thread Regina Obe
> 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

2022-11-10 Thread Regina Obe
> "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

2022-11-10 Thread Regina Obe
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

2022-11-09 Thread Regina Obe
> "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

2022-11-09 Thread Regina Obe
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

2022-10-30 Thread Regina Obe
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

2022-05-27 Thread Regina Obe
> 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

2022-01-27 Thread Regina Obe
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

2020-02-26 Thread Regina Obe
> 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

2019-03-11 Thread Regina Obe
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

2019-02-22 Thread Regina Obe
> 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

2019-02-22 Thread Regina Obe
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

2018-07-23 Thread Regina Obe
> 
> > 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

2018-07-23 Thread Regina Obe
> 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

2018-06-29 Thread Regina Obe
> "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




MemoryContextCreate change in PG 11 how should contexts be created

2017-12-19 Thread Regina Obe
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,
  ,

PROJ4Cache->PROJ4SRSCacheContext,
  "PostGIS PROJ4 PJ Memory
Context");



Thanks,
Regina