postgis_extensions_upgrade() breaks and signalize unbalanced parenteses in regex of regexp_replace function

[hci]

After apt upgrade Postgis 3.1.1 to 3.3.1 by apt pgdg bullseye repo, when we trying to use postgis_extensions_upgrade(), the instruction ' ALTER EXTENSION UPGRADE postgis TO "3.3.1" ' was breaked by a regex expression.

This issue occured for these versions: PostgreSQL 13.9 (Debian 13.9-0+deb11u1) on x86_64-pc-linux-gnu, compiled by gcc (Debian 10.2.1-6) 10.2.1 20210110, 64-bit and POSTGIS="3.3.1 3786b21" [EXTENSION] PGSQL="130" GEOS="3.9.0-CAPI-1.16.2" PROJ="7.2.1" GDAL="GDAL 3.2.2, released 2021/03/05" LIBXML="2.9.10" LIBJSON="0.15" LIBPROTOBUF="1.3.3" WAGYU="0.5.0 (Internal)" (core procs from "3.1.1 aaf4c79" **need upgrade**) TOPOLOGY (topology procs from "3.1.1 aaf4c79" need upgrade) RASTER (raster procs from "3.1.1 aaf4c79" need upgrade) Both installed from pgdg bullseye repo. Notice the messages 'need upgrade' indicating the need to run 'postgres upgrade version()' function.

This issue not occured for PostgreSQL 13.13, suggesting that the regexp_replace function was changed in this most recent PostgreSQL version.

We solved this issue for the PostgreSQL 13.9 by changing the regex expression at line 9869 of postgis—ANY—3.4.0.sql From:

pg_catalog.regexp_replace(rec.proc::text, '_deprecated_by_postgis[^(]*\(.*', '' ); 

To:

pg_catalog.regexp_replace(rec.proc::text, '_deprecated_by_postgis[^(]*\(.*)', '' );

Just by adding the ')' to regex '_deprecated_by_postgis[^(]*\(.*)'

[robe]

Looking at this closer, I think it might be the standard_conforming_strings setting at fault here.

I can replicate the issue with the following:

SET standard_conforming_strings = 'off';

SELECT pg_catalog.regexp_replace(rec.proc::text, '_deprecated_by_postgis[^(]*\(.*', '' )
FROM (SELECT oid, oid::regprocedure AS proc FROM pg_proc) AS rec; 

Yields error:

WARNING:  nonstandard use of escape in a string literal

ERROR:  invalid regular expression: parentheses () not balanced 

SQL state: 2201B

By default, since PostgreSQL I think 9.0, standard_conforming_strings setting is set to on.

Can you do the following on your 13.9

SHOW standard_conforming_strings;

I suspect it's off on your 13.9

At anyrate we should rewrite our check to work whether or not standard_conforming_strings setting is on or off.

[robe]

strk I think the proper way to write this to not care about what the standard_conforming_strings setting is is force escape like so So you need 2
s instead of 1 slash to escape the (

pg_catalog.regexp_replace(rec.proc::text, E'_deprecated_by_postgis[^(]*\\(.*',

With that, this query seems to work properly regardless if I have standard_conforming_strings on or off

SELECT pg_catalog.regexp_replace(rec.proc::text, E'_deprecated_by_postgis[^(]*\\(.*', '' ), proc
    FROM   (SELECT oid, oid::regprocedure AS proc, proname  FROM pg_proc) AS rec
        WHERE proname ~ 'deprecated_by_postgis'

[strk]

Of course now I want some of our bots to run tests with standard_confirming_strings on and off … How can we inject this from the outside, so that not all bots behave the same ?

[strk]

Bot work completed, once it shows the failure I'll push the fix: ​https://git.osgeo.org/gitea/postgis/postgis/pulls/167

[strk]

There are a bunch of other issues with standard conforming strings off, like:

Loading PostGIS into 'postgis_reg'
WARNING:  nonstandard use of escape in a string literal
LINE 4:  FROM pg_catalog.substring(version(), 'PostgreSQL ([0-9\.]+)...

should we force them ON when installing postgis ?

[strk]

Another example of issue:

WARNING:  nonstandard use of escape in a string literal
LINE 21:   font_default json = '{

[strk]

So: more non-balanced errors: ​https://woodie.osgeo.org/repos/30/pipeline/1651/11#L12454

psql:/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/regress/hooks/use-all-functions.sql:54: WARNING: nonstandard use of escape in a string literal

[robe]

Grrh I thought I had fixed the views and view functions, but I guess not.

I think it's okay to force on during install and upgrade, but not in running state, since function gucs do nasty things with the planner make it not allow things like inlining etc.

[Sandro Santilli <strk@…>]

In fb0a6302/git:

Add hook to set standard_conforming_string off in test database

References #5633

[Sandro Santilli <strk@…>]

In 6f0f6ef/git:

[woodie] add upgrade tests with standard_conforming_strings off

References #5633

[Sandro Santilli <strk@…>]

In 03e8bc9/git:

Use escaped string to not depend on standard_conforming_strings

References #5633

[Sandro Santilli <strk@…>]

In 61b71ba/git:

Use escaped string to not depend on standard_conforming_strings

References #5633 in 3.4 branch (3.4.2dev)

[Sandro Santilli <strk@…>]

In 3886dee1/git:

More escaped string usage (regress tests)

References #5633

[Sandro Santilli <strk@…>]

In 451e85f6/git:

Add hook to set standard_conforming_string off in test database

References #5633 in 3.4 branch (3.4.2dev)

[strk]

I've pushed fixes to both master and stable-3.4 branch but test coverage is not full:

• Master branch only tests a single testcase (regress/core/regress)
• 3.4 branch doesn't test standard_conforming_strings off at all

It may be a good idea to have both branches run all tests with all configurations, which would double the CI runtime unless we mix and match different bots to do different things. In 3.4 and older branches woodie and dronie share the same configuration, maybe I will backport the CI configuration from master to have better coverage.

[Sandro Santilli <strk@…>]

In f73b0aec/git:

Have woodie test with standard_conforming_strings off too

Copies the regress woodie workflow from master branch

References #5633 in 3.4 branch (3.4.2dev)

[strk]

3.4 is also tested now, backport to 3.3 will be last one and is being tested in ​https://git.osgeo.org/gitea/postgis/postgis/pulls/168

[Sandro Santilli <strk@…>]

In 28a10ec/git:

More escaped string usage (regress tests)

References #5633

[strk]

The test in 3.3 branch is running ALL tests with standard_conforming_strings=off and is catching more issues: ​​https://woodie.osgeo.org/repos/30/pipeline/1673/10

I guess we could unleash all tests in 3.4 and master branches too, rather than (as we do now) restrict that test to the sole "core/regress.sql" file, would make sense

[Sandro Santilli <strk@…>]

In 5c7c2c3e/git:

Use escaped string constants in regression tests

References #5633

[Sandro Santilli <strk@…>]

In 809efdc/git:

Use escaped string or set standard_conforming_strings=on

References #5633

[Sandro Santilli <strk@…>]

In 10ab2c6/git:

Use escaped string or set standard_conforming_strings=on

References #5633 in 3.4 branch (3.4.2dev)

[Sandro Santilli <strk@…>]

In 9cd9194/git:

woodie: run all tests with standard_conforming_strings=off

We'll leave standard_conforming_strings=on (default) testing to
other bots

References #5633 in 3.4 branch (3.4.2dev)

[Sandro Santilli <strk@…>]

In 444e5c3/git:

Use escaped string or set standard_conforming_strings=on

Closes #5633 in 3.3 branch (3.3.6dev)

[Sandro Santilli <strk@…>]

In 8e06dac/git:

Add hook to set standard_conforming_string off in test database

References #5633 in 3.3 branch (3.3.6dev)

[Sandro Santilli <strk@…>]

In d5cdeb9/git:

woodie: test standard_confirming_strings=off and refactor

Refactors to use custom recipes rather than sharing them
with dronie, then:

• Test pgsql 11 and 15
• Test standard_conforming_strings=off

References #5633 in 3.3 branch (3.3.6dev)