Upgrade does not upgrade geog_brin_inclusion_add_value function to use the updated module

[keiko713]

Hello,

I'm seeing the issue that geog_brin_inclusion_add_value function is not upgraded to use the updated module under the certain condition. Particularly, it has Postgis 2.3.2, and run ALTER EXTENSION postgis UPDATE (with the instance that has 2.4.2 in it), it upgrade all the functions to use $libdir/postgis-2.4 except geog_brin_inclusion_add_value.

For example, this is something I can see (check probin of geog_brin_inclusion_add_value):

> select proname, proowner, prosrc, probin from pg_proc where probin like '%postgis%';
                  proname                  | proowner |                  prosrc                   |        probin         
-------------------------------------------+----------+-------------------------------------------+-----------------------
 st_flipcoordinates                        |       10 | ST_FlipCoordinates                        | $libdir/postgis-2.4
 geog_brin_inclusion_add_value             |       10 | geog_brin_inclusion_add_value             | $libdir/postgis-2.3
 box2df_in                                 |       10 | box2df_in                                 | $libdir/postgis-2.4
 box2df_out                                |       10 | box2df_out                                | $libdir/postgis-2.4
...

Looking at the script, there is CREATE OR REPLACE FUNCTION for geog_brin_inclusion_add_value using the correct MODULE_PATHNAME ($libdir/postgis-2.4 in this case), but this part of code is underneath of the IF statement and if it doesn't go inside of that IF statement, it won't run such thing, therefore upgrade itself finishes without going through CREATE OR REPLACE FUNCTION for geog_brin_inclusion_add_value.

​https://github.com/postgis/postgis/blob/2.3.2/postgis/geography_brin.sql.in#L73-L76

Regardless the missing operator, CREATE OR REPLACE FUNCTION should happen with geog_brin_inclusion_add_value so that it can be loaded with the upgraded version's module.

This is causing the problem with Postgres that went through Postgis upgrade to 2.4, and Postgres version upgrade from Postgres 9.6 to Postgres 10 -- since Postgres 10 still tries to load $libdir/postgis-2.3 due to geog_brin_inclusion_add_value but it's unable to because the machine doesn't have postgis-2.3 with Postgres 10.

As a workaround, I was able to finish the Postgres version upgrade from 9.6 to 10 by manually running following, but would be really nice if it's fixed within Postgis itself (also I'm afraid if I'm missing something else inside of that IF statement, I'm not really familiar with Postgis).

CREATE OR REPLACE FUNCTION geog_brin_inclusion_add_value(internal, internal, internal, internal) RETURNS boolean
        AS '$libdir/postgis-2.4','geog_brin_inclusion_add_value'
        LANGUAGE 'c';

Please let me know if there is anything I can add to this report. Thanks!!

Keiko

Similar issue in StackOverflow: ​https://stackoverflow.com/questions/43226275/can-not-find-postgis-extensions-installed-in-a-different-directory-and-libd

[keiko713]

Is there anything I can add to help to get this fixed? Thanks!

[strk]

I'm not sure exactly why that IF is there, but for sure it looks like the functions can be outside of the IF. Personally I'd drop the IF completely and focus on making the upgrade-generator aware of operator and operator families (it may be aware by now). Upgrade tests can be run via utils/check_all_upgrades.pl if you want to try providing a patch. And consider sending a PR via Gitea mirror, where Drone would be further testing upgrades.

[robe]

The IF def was needed to handle the case of

PostgreSQL 9.4 2.3+ whatever pg_upgrading to PostgreSQL 9.6 2.3+.

Our upgrade scripts would fail since they would assume you already had these BRIN things if you were coming from a version of PostGIS that already had these.

Now that we have PostgreSQL version included as part of the script version, I think we could make our upgrade smarter and run these kind of things -- if version coming from is < version where said thing is allowed.

[robe]

keiko713,

Which version of PostgreSQL/PostGIS were you coming from? Please provide output of

SELECT postgis_full_version() || ' ' || version();

on both your old and new cluster.

I'm looking at the latest upgrade script, and for PostgreSQL 9.5 and above, the geog_brin_inclusion_add_value function is always updated as is the case with all functions.

So the only reason why I can think this would have failed for you is 2

1) You are using scripts that were compiled against PostgreSQL 9.4 or below to upgrade your database. The IF def is there to prevent these functions and types from being installed. So if you are using these upgrade scripts, yah these functions won't be in there.

strk - I forget, did we put in a check in 2.4 to prevent someone from accidentally using scripts compiled against a lower version of PostgreSQL on a higher version of PostgreSQL. If we did that would have caught this case.

2) You are using a PostGIS micro before 2.3.3. I think then though, you would have had issues as the upgrade script would include trying to upgrade types you didn't have and you would have had to manually install these to correct the situation as your upgrade would have failed.

[robe]

I'm closing this for now as I don't see anything wrong here.

strk - to really test this kind of problem out in our battery of tests, we need to have have a pg_upgrade test. The plain old in-place upgrade of PostGIS is not going to catch this. This kind of issue only arises when pg_upgrade is involved.

[keiko713]

Thanks a lot for taking a look this! Let me focus on the issue I'm seeing, to avoid the complexity of pg_upgrade, so please forget about the Postgres version upgrade here.

Here is an easy way to reproduce this issue.

0. using Postgres 9.6
1. install Postgis 2.3.3 (used apt-get install postgresql-9.6-postgis-2.3 postgresql-9.6-postgis-2.3-scripts)
2. CREATE EXTENSION postgis

gisdb=# SELECT postgis_full_version() || ' ' || version();
                                                                                                                           ?column?                                                                                                           
                 
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
-----------------
 POSTGIS="2.3.3 r15473" GEOS="3.4.2-CAPI-1.8.2 r3921" PROJ="Rel. 4.8.0, 6 March 2012" GDAL="GDAL 1.10.1, released 2013/08/26" LIBXML="2.9.1" LIBJSON="0.11.99" RASTER PostgreSQL 9.6.6 on x86_64-pc-linux-gnu, compiled by gcc (Debian 4.9.2-1
0) 4.9.2, 64-bit

3. install postgis 2.4 (used apt-get install postgresql-9.6-postgis-2.4 postgresql-9.6-postgis-2.4-scripts)
4. ALTER EXTENSION postgis UPDATE

gisdb=# SELECT postgis_full_version() || ' ' || version();
                                                                                                                                 ?column?                                                                                                     
                            
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
----------------------------
 POSTGIS="2.4.2 r16113" PGSQL="96" GEOS="3.4.2-CAPI-1.8.2 r3921" PROJ="Rel. 4.8.0, 6 March 2012" GDAL="GDAL 1.10.1, released 2013/08/26" LIBXML="2.9.1" LIBJSON="0.11.99" RASTER PostgreSQL 9.6.6 on x86_64-pc-linux-gnu, compiled by gcc (Deb
ian 4.9.2-10) 4.9.2, 64-bit

5. geog_brin_inclusion_add_value is still pointing 2.3

gisdb=# select proname, prosrc, probin from pg_proc where probin like '%postgis%' and probin not like '%postgis-2.4';
            proname            |            prosrc             |       probin        
-------------------------------+-------------------------------+---------------------
 geog_brin_inclusion_add_value | geog_brin_inclusion_add_value | $libdir/postgis-2.3
(1 row)

The problem here is, the database at the point step 2 has operators and things like that already (because it's created as Postgres 9.5+ and Postgis 2.3), therefore it won't go inside of IF statement and the binary reference never be updated. I would assume that when Postgis is updated from 2.3 to 2.4, all binary reference would point to 2.4 as well. As you can see above, geog_brin_inclusion_add_value is the only function that is not updated in fact.

Does this make sense? Happy to explain more or testing out things. In the meantime, I'll see if I can work on the patch.

[strk]

Thanks, makes a lot of sense. In order to have our bots catch these we'd need to *uninstall* the old version libraries right before running the regress test (and after running the upgrade procedure). Not that easy... uhm

[strk]

Or I might have found another way to catch this via upgrade testing, see how you like r16225 (and let's keep an eye on Drone, which does test upgrade from 2.2.3 to 2.5.0dev (see line 2414 on ​https://drone.osgeo.org/postgis/postgis/783)

[strk]

BTW, I don't think you need to handle the PostgreSQL version upgrade because the upgrade script used should be the one built against the *new* version of PostgreSQL. Is this not the case, with pg_upgrade ? We actually do know we ask the users to upgrade with postgis_extension_upgrade() after pg_upgrade, right?

[strk]

The Drone-tested 2.2.3--2.5.0dev upgrade seems to pass, so doesn't leave any old signature, ah, but that's running with an old PostgreSQL version (9.3)

Regina: we need Jenkins to test upgrades too!

[robe]

keiko713 can you send me the file postgis-2.3.3-2.4.2.sql that got packaged with your postgresql-9.6-postgis-2.4-scripts.

I tested with my windows scripts, and I can't replicate this.

I also looked at my postgis-2.3.3--2.4.2.sql and I see this line

-- Availability: 2.3.0
CREATE OR REPLACE FUNCTION geog_brin_inclusion_add_value(internal, internal, internal, internal) RETURNS boolean
        AS '$libdir/postgis-2.4','geog_brin_inclusion_add_value'
        LANGUAGE 'c';

There is no IF DEF in the script. The function gets reinstalled regardless what version of PostGIS you are coming from UNLESS you are running with PostGIS scripts generated from PostgreSQL < 9.5.

I suspect this is a packaging bug and not a PostGIS bug, but want to verify my suspicion. It's possible debian folks shipped the wrong scripts thinking that all postgis-2.3.3-2.4.3.sql are made the same and they are not.

[robe]

keiko713 disregard my comment. strk was able to replicate and is fixing now. Sorry for my misunderstanding.

[strk]

In 16232:

Simplify syntax for brin opclasses so upgrade script recognizes it

Fixes #3956 for me

[strk]

Please everyone test r16232 - if it works as expected we want to backport it to 2.4 and 2.3 branches. It greatly simplifies the code too, removing the smartness. The way I used to test it:

RUNTESTFLAGS="-v --extension --upgrade-path=2.3.4--2.5.0dev" make -C regress check

[strk]

BRIN first entered in #3591, Giuseppe does the change look fine to you ?

[gbroccolo]

Hi all,

OpFamily definitions were needed because the storage datatype are BOX2DF/GIDX. Unlike GiST, BRIN access methods do not include compress/decompress functions to convert indexed data in the aforementioned storage datatypes. The only way is to implement cross-datatype functions and operators for comparisons between stored and key data in query execution. That's why OpFamily definitions are needed.

[strk]

Are there tests in place to find it non-working if latest commit broke it ? Or does it need manual testing ?

[strk]

it seems we can specify STORAGE with CREATE OPERATOR CLASS too, at least r16235 version of the syntax seems to be accepted: ​https://trac.osgeo.org/postgis/browser/trunk/postgis/geography_brin.sql.in?rev=16235#L65

[strk]

In 16241:

Simplify syntax for brin opclasses so upgrade script recognizes it

Fixes #3956 in 2.4 branch

[strk]

In 16244:

Simplify syntax for brin opclasses so upgrade script recognizes it

Fixes #3956 in 2.3 branch

[strk]

So I've backported my fix to all supported branches (2.3, 2.4, trunk). Giuseppe if you see anything non-functioning please let us know !

[keiko713]

Thank you so much for the patch! Really appreciate it. Do we have any date for the next release? I can see that the milestone of Postgis 2.4.3 is set to Mar 31, 2018, but is this likely going to be the release date?

[strk]

Remember the fix was not backported yet, we were waiting for your confirmation !

[strk]

OOps, sorry, I did backport. No elapsed date for 2.4.3 but maybe we can rush into shipping it for this specific fix. Do you want to bring up the request to the mailing list ?

[keiko713]

Will do, thanks!

[gbroccolo]

Hi Sandro,

Replying to strk:

So I've backported my fix to all supported branches (2.3, 2.4, trunk). Giuseppe if you see anything non-functioning please let us know !

I think it's fine. Thank you for the fix.