Opened 5 months ago

Closed 3 months ago

Last modified 3 months ago

#5633 closed enhancement (fixed)

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

Reported by: hci Owned by: strk
Priority: low Milestone: PostGIS 3.3.6
Component: upgrade/soft Version: 3.3.x
Keywords: upgrade extensions regexp_replace Cc: hci

Description

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[^(]*\(.*)'

Change History (27)

comment:1 by robe, 5 months ago

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.

comment:2 by robe, 5 months ago

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'

comment:3 by strk, 5 months ago

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 ?

comment:4 by strk, 5 months ago

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

comment:5 by strk, 4 months ago

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 ?

comment:6 by strk, 4 months ago

Another example of issue:

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

comment:7 by strk, 4 months ago

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

comment:8 by robe, 4 months ago

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.

comment:9 by Sandro Santilli <strk@…>, 4 months ago

In fb0a6302/git:

Add hook to set standard_conforming_string off in test database

References #5633

comment:10 by Sandro Santilli <strk@…>, 4 months ago

In 6f0f6ef/git:

[woodie] add upgrade tests with standard_conforming_strings off

References #5633

comment:11 by Sandro Santilli <strk@…>, 4 months ago

In 03e8bc9/git:

Use escaped string to not depend on standard_conforming_strings

References #5633

comment:12 by Sandro Santilli <strk@…>, 4 months ago

In 61b71ba/git:

Use escaped string to not depend on standard_conforming_strings

References #5633 in 3.4 branch (3.4.2dev)

comment:13 by Sandro Santilli <strk@…>, 4 months ago

In 3886dee1/git:

More escaped string usage (regress tests)

References #5633

comment:14 by Sandro Santilli <strk@…>, 4 months ago

In 451e85f6/git:

Add hook to set standard_conforming_string off in test database

References #5633 in 3.4 branch (3.4.2dev)

comment:15 by strk, 4 months ago

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.

comment:16 by Sandro Santilli <strk@…>, 4 months ago

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)

comment:17 by strk, 4 months ago

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

comment:18 by Sandro Santilli <strk@…>, 4 months ago

In 28a10ec/git:

More escaped string usage (regress tests)

References #5633

comment:19 by strk, 4 months ago

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

comment:20 by Sandro Santilli <strk@…>, 4 months ago

In 5c7c2c3e/git:

Use escaped string constants in regression tests

References #5633

comment:21 by Sandro Santilli <strk@…>, 3 months ago

In 809efdc/git:

Use escaped string or set standard_conforming_strings=on

References #5633

comment:22 by Sandro Santilli <strk@…>, 3 months ago

In 10ab2c6/git:

Use escaped string or set standard_conforming_strings=on

References #5633 in 3.4 branch (3.4.2dev)

comment:23 by Sandro Santilli <strk@…>, 3 months ago

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)

comment:24 by Sandro Santilli <strk@…>, 3 months ago

Resolution: fixed
Status: assignedclosed

In 444e5c3/git:

Use escaped string or set standard_conforming_strings=on

Closes #5633 in 3.3 branch (3.3.6dev)

comment:25 by Sandro Santilli <strk@…>, 3 months ago

In 8e06dac/git:

Add hook to set standard_conforming_string off in test database

References #5633 in 3.3 branch (3.3.6dev)

comment:26 by Sandro Santilli <strk@…>, 3 months ago

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)

comment:27 by strk, 3 months ago

Milestone: PostGIS PostgreSQLPostGIS 3.3.6
Note: See TracTickets for help on using tickets.