Upgrade to 2.2 doesn't adjust size of pgis_abs type

[dbaston]

The size of the pgis_abs type was changed from 8 bytes to 16 bytes in 2.2.0. It doesn't appear that the upgrade procedure handles this, so if you upgrade to 2.2.x from a previous version, Postgres would still think the size is 8 bytes.

I haven't been able to show this, because I can't actually perform an upgrade from 2.1.x to more recent versions due to #3429. But this seems like it would explain the following issue reported to postgis-users:

On Wed, Feb 17, 2016 at 01:44:54PM +0100, Ronnie Lassche wrote:

I’m trying to use the new st_clusterwithin function, but it keeps failing.

I’ve tried to cluster 20 points with the following query.

SELECT ST_ClusterWithin(geom, 50) FROM winkels

Most of the times I get the message

“ERROR: Tolerance not defined

A few times it did work and gave me a result, but trying again will lead to the same error.

I’m working with postgis 2.2.2dev (but also tried it with 2.2.0).

(The tolerance value is stored in the second 8 bytes of pgis_abs)

[robe]

Dan and I discussed this at the sprint and I think he came up with a patch on his github repo which I had promised to test but haven't gotten to yet.

Anyrate this can't go in 2.2 since it introduces a new type (granted a type behind the scenes). Unless I am mistaken (like strk changed things), I don't think our upgrade logic handles creation of types in micros.

[strk]

The upgrade script considers Major.Minor versions, no Patch version. As long as the "Availability" label is present in the .sql script source, it should be adding the appropriate CREATE TYPE, see ​https://trac.osgeo.org/postgis/browser/tags/2.2.1/utils/postgis_proc_upgrade.pl#L101 and ​https://trac.osgeo.org/postgis/browser/tags/2.2.1/utils/postgis_proc_upgrade.pl#L252

[robe]

The problem is I don't think the note was updated in 2.2.0 and also it was a change to an existing type (not creation of a new type) which I don't think our script handles. So it's too late to slide it in in 2.2.2 since that's a micro.

Dan's patch he worked at the code sprint I instructed him on was to

1. create a new type and put availability on it

2. In the create script put a Changed note for all the aggregates

and have them made with the new type

3. drop the old type in the drop after script

Are those still the right steps.

[strk]

Your recipe should work, yes. But it's best to test it, as we have no real automated upgrade test in place (between different versions).

[robe]

As soon as Dan commits his work to 2.3 I will do a more thorough test. I think he had tested from 2.1-2.3 if I am not mistaken. But he did not try testing the ALTER EXTENSION part.

[dbaston]

If we can't make changes to the type in the 2.2.x branch, maybe we need to find a different approach to resolve the bug. I don't know exactly what Postgres is doing with the internallength parameter. Perhaps, if the bug can be confirmed, we can find a way to fix it that doesn't involve dropping the type. Otherwise, it sounds like we'd have to leave the bug open in the 2.2.x series.

[pramsey]

Why don't we just add a line at the end up the next 2.2 upgrade script to change pt_type.typlen to the value we want in the system tables?

UPDATE pg_type SET typlen = 16 WHERE typname = 'pgis_abs';

[robe]

Wow I didn't know you could do that. That feels a bit dirty but I'm okay with it if t works.

[robe]

The only thing I would change in your update is to put in an additional where so we don't unnecessarily have to update the system catalogs every time.

UPDATE pg_type SET typlen = 16 WHERE typname = 'pgis_abs' AND typlen = 8;

[strk]

My only concern is with different postgresql versions possibly storing those values in a different way or giving different access to update them. As long as it's tested against every supported version it's ok with me.

Do we have a test in place showing how an upgraded database (failing to update the size) corrupts the results ?

[robe]

No we don't but I guess given Paul's proposal we'd just have to have a test taht updates the typlen =8 does the test and then updates it back to 16. I suspect we wouldn't need to go thru the whole process of installing a PostGIS 2.2.0 testing to see the ill effects. Though someone should to confirm his update is the same as a true type always being that.

[dbaston]

Great, I can reproduce the problem if I set typlen=8 in the system table.

UPDATE pg_type SET typlen=8 WHERE typname='pgis_abs';
SELECT ST_ClusterWithin(geom, 0.1) FROM (SELECT ST_MakePoint(random(), random()) AS geom FROM generate_series(1, 100)) sq;

gives

ERROR:  Tolerance not defined

If I set typlen to 16 after this, the query succeeds.

I don't think we want an automated test for this though, because it's possible to not get an error even with typlen=8. If I set it to 16, run a query, and then go back to 8, I don't get an error. It's only if it's set to 8 at the beginning of the session. Even the report we got on postgis-users said the problem was intermittent, which I think is to be expected.

[robe]

Okay feel free to commit the change then so we can close this out. I guess we then need to consider if we really need to create anew type for PostGIS 2.3 or jsut use pramsey's dirty hack and move on.

[strk]

I don't think we want an automated test for this though, because it's possible to not get an error even with typlen=8.

As long as "sane" installs do not give false errors I still think it is worth to include a test that would "occasionally" fail on a broken system.

What's still not being automated is testing the upgrade from a broken version to a fixed one. But it can still be done manually with something like:

    regress/runtest.pl \
        --raster \
        --extension \
        --upgrade-path <broken>--<fixed> \
        raster/test/regress/tickets.sql

And making sure the "tickets.sql" file does have a test tha _might_ trigger the error.

[dbaston]

Thanks for the example; that indeed triggers a failure in an existing testcase:

dbaston@dbaston-lenovo:~/dev/postgis$ regress/run_test.pl --extension --upgrade-path 2.1.8--2.3.0dev regress/cluster.sql 
Upgrade path: 2.1.8 --> 2.3.0dev
PATH is /home/dbaston/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
Checking for shp2pgsql ... found
Checking for pgsql2shp ... found
Checking for raster2pgsql ... found
TMPDIR is /tmp/pgis_reg
Creating database 'postgis_reg' 
Preparing db 'postgis_reg' using: CREATE EXTENSION postgis VERSION '2.1.8'
Upgrading PostGIS in 'postgis_reg' using: ALTER EXTENSION postgis UPDATE TO '2.3.0dev'
PostgreSQL 9.3.11 on x86_64-unknown-linux-gnu, compiled by gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2, 64-bit
  Postgis 2.3.0dev - r14745 - 2016-03-05 01:09:27
  scripts 2.3.0dev r14745
  raster scripts 2.3.0dev r14745
  GEOS: 3.6.0dev-CAPI-1.10.0 r4118
  PROJ: Rel. 4.8.0, 6 March 2012
  GDAL: GDAL 1.10.1, released 2013/08/26

Running tests

 regress/cluster .. failed (diff expected obtained: /tmp/pgis_reg/test_1_diff)
 uninstall .. ok (4112)

Run tests: 2
Failed: 1

It looks like the type update could be added to postgis_drop_before.sql, postgis_drop_after.sql, or inlined in postgis_proc_upgrade.pl. Or I could add a postgis_miscellaneous_hacks.sql.

Any objections to me adding it to postgis_drop_after.sql ? (this makes the above test pass)

[robe]

I'm okay with any of the above. I'm hesitant to add yet another file we need to explain and some hacks may require doing before or after so that's my only gripe with that.

I had a hack for type when some type was renamed. I forget where I put that.

[strk]

I think postgis_drop_after.sql is the correct place for that (if it's a postgis type).

[dbaston]

Committed to trunk at r14766 and 2.2 at r14767.

[strk]

Wait a second, I didn't understand you wanted to put the UPDATE into _drop_after.sql. If it's not a drop, why not putting it directly right after the CREATE TYPE ?