Opened 7 years ago
Closed 7 years ago
Last modified 7 years ago
#3460 closed defect (fixed)
Upgrade to 2.2 doesn't adjust size of pgis_abs type
|Reported by:||dbaston||Owned by:||dbaston|
Description (last modified by )
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
Change History (21)
comment:1 by , 7 years ago
comment:2 by , 7 years ago
|Milestone:||PostGIS 2.2.2 → PostGIS 2.3.0|
comment:3 by , 7 years ago
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
comment:4 by , 7 years ago
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
- create a new type and put availability on it
- In the create script put a Changed note for all the aggregates
and have them made with the new type
- drop the old type in the drop after script
Are those still the right steps.
comment:5 by , 7 years ago
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).
comment:6 by , 7 years ago
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.
comment:7 by , 7 years ago
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.
comment:8 by , 7 years ago
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';
comment:9 by , 7 years ago
Wow I didn't know you could do that. That feels a bit dirty but I'm okay with it if t works.
comment:10 by , 7 years ago
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;
comment:11 by , 7 years ago
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 ?
comment:12 by , 7 years ago
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.
comment:13 by , 7 years ago
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;
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.
comment:14 by , 7 years ago
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.
comment:15 by , 7 years ago
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.
comment:16 by , 7 years ago
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_after.sql, or inlined in
postgis_proc_upgrade.pl. Or I could add a
Any objections to me adding it to
postgis_drop_after.sql ? (this makes the above test pass)
comment:17 by , 7 years ago
|Milestone:||PostGIS 2.3.0 → PostGIS 2.2.2|
comment:18 by , 7 years ago
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.
comment:19 by , 7 years ago
I think postgis_drop_after.sql is the correct place for that (if it's a postgis type).
comment:20 by , 7 years ago
|Status:||new → closed|
comment:21 by , 7 years ago
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 ?
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. therefore I am pushing this to 2.3.0