view xxx depends on function st_union(geometry)

[strk]

I got striked by this known issue of aggregates upgrade being impossible to do in-place (ie: without a drop).

Should we provide an automatic solution to the problem ? For example, could we drop all views using any postgis function during upgrade and re-create them later ? Or is there any other known workaround to the problem that you are aware of ?

[strk]

Note that ST_Union(geometry) aggregate hasn't changed between 2.0 and 2.1, but still the 20_to_21 upgrade attempts the DROP & CREATE ballet:

​https://github.com/postgis/postgis/blob/2.0.0/postgis/postgis.sql.in.c#L3094-L3099 ​https://github.com/postgis/postgis/blob/2.1.0/postgis/postgis.sql.in#L3332-L3337

[strk]

According to this line, it looks like we're not supposed to replace old views ? ​https://github.com/postgis/postgis/blob/2.0.0/utils/postgis_proc_upgrade.pl#L60

Wasn't that the idea, @pramsey ?

[strk]

I'd target this back in the 2.1.x milestone, as it's blocking upgrades from 2.0

[strk]

ST_Union hadn't changed in 2.1.1 either: ​https://github.com/postgis/postgis/blob/2.1.1/postgis/postgis.sql.in#L3344-L3349

[strk]

I'd tried this fix:

diff --git a/utils/postgis_proc_upgrade.pl b/utils/postgis_proc_upgrade.pl
index 93b82ff..d63d236 100755
--- a/utils/postgis_proc_upgrade.pl
+++ b/utils/postgis_proc_upgrade.pl
@@ -227,8 +227,17 @@ while(<INPUT>)
                        $aggtype = $1 if ( /basetype\s*=\s*([^,]*)\s*,/i );
                        last if /\);/;
                }
-               print "DROP AGGREGATE IF EXISTS $aggname($aggtype);\n";
-               print $def;
+    my $aggsig = "$aggname($aggtype)";
+               my $ver = $version_from_num + 1;
+               while( $version_from_num < $version_to_num && $ver <= $version_to_num )
+               {
+                       if( $objs->{$ver}->{"aggregates"}->{$aggsig} )
+                       {
+        print "DROP AGGREGATE IF EXISTS $aggsig;\n";
+        print $def;
+                       }
+                       $ver++;
+               }
        }
        
        # This code handles operators by creating them if we are doing a major upgrade

But the *drop* scripts are still getting in the middle, forcing drop of some of the aggregates, and not all of them have a comment telling me when the drop was required:

DROP AGGREGATE IF EXISTS memgeomunion(geometry);
DROP AGGREGATE IF EXISTS geomunion(geometry);
DROP AGGREGATE IF EXISTS polygonize(geometry); -- Deprecated in 1.2.3, Dropped in 2.0.0
DROP AGGREGATE IF EXISTS collect(geometry); -- Deprecated in 1.2.3, Dropped in 2.0.0
DROP AGGREGATE IF EXISTS st_geomunion(geometry);
DROP AGGREGATE IF EXISTS accum_old(geometry);
DROP AGGREGATE IF EXISTS st_accum_old(geometry);

In particular "st_geomunion(geometry)" seems to be a dangerous one, and now I wonder if it was the reason why we started dropping/recreating aggregates in "soft" upgrade procedures in the first place :/

[robe]

st_geomunion was a mistake from the beginning that is why it is dropped. It was created when pramsey went ST_ happy and did a macro replace to put ST_ in front of everything. There has only ever been geomunion and ST_Union ever in use or documented.

Note: the bigger fix is to change the extension scripts. Right now I think they just use postgis_20_21 which has worked because there wasn't a difference between 20_21 and 21_minor. If we change we have to add another conditional loop break out the copy to copy the right versions

[strk]

I went trough postgis-2.0.sql and postgis-2.1.sql and can confirm there is NO DIFFERENCE in aggregate definitions.

[strk]

(the versions I checked are the tips of the respective branches)

[strk]

the only aggregate in topology.sql also hadn't changed between 2.0 and 2.1

[strk]

many differences in aggregates of raster :/

[strk]

With r12240 the 2.1 branch should be fine. Tests welcome

[strk]

it turns out that rtpostgis_upgrade* is NOT being generated using the postgis_proc_upgrade.pl script, any reason why ?

[strk]

I was thinking that the postgis_proc_upgrade.pl script could read the version of last object change (or introduction) directly from the .sql. That way there would be less configuration required. It could read: "Availability: x.y.z" or "Changed: x.y.z", if we find a univoque semantic of those labels. For example, we are NOT interested in behavioural change but only in _definition_ change, while I'm afraid "Changed" signifies a behavioural change.

With r12238 and r12239 i've added some such labels for raster and topology, postgis already has them in place…

[strk]

With r12241 and r12242 raster upgrade is also generated with postgis_proc_upgrade. Will wait for a word of confirmation by robe before closing

[strk]

r12243 ports it all to trunk, adding availability info for new aggregates in raster (plenty!).

I'm almost 100% happy. The only thing that would make me happier would be directly reading availability/last_update from the .sql file so to drop the internal configuration in postgis_proc_upgrade

\cc @pramsey

[strk]

as of r12245 postgis_proc_upgrade.pl will read "last_updated" information from the SQL itself (at least for aggregates) and will print a warning to stderr if that information is missing.

It currently checks for "Availability" or "Changed". Ideally we'd add another label because "Changed" is also sometimes used for semantic change with no change in signature definition…

[robe]

I have a feeling this will break extensions. I just realized extensions drops all aggregate from extension with the assumption they will be readded. So this requires changes to that which I am willing to do but won't be able to get to for a while.

I'll test later today but if it does we need to pull this out of 2.1 if you aren't willing to fix it.

I really don't think we should be pushing such big changes to a stable branch without throughly testing on trunk first.

[robe]

Yap this breaks extensions in way I expected it would. To test I did:

1) before installing new binaries etc:

CREATE EXTENSION postgis VERSION "2.1.2dev"

2) then upgrade to postgis-2.1.2

ALTER EXTENSION postgis UPDATE TO "2.1.2devnext"; 

3) Then to see what was left behind:

CREATE SCHEMA postgis;
ALTER EXTENSION postgis SET SCHEMA postgis;

And whammo the only aggregates that moved over to the postgis schema are the raster aggregates.

[robe]

Forgot to mention this was just testing on 2.1

POSTGIS="2.1.2dev r12242" GEOS="3.4.2-CAPI-1.8.2 r3924" PROJ="Rel. 4.8.0, 6 March 2012" GDAL="GDAL 1.10.0, released 2013/04/24" LIBXML="2.7.8" LIBJSON="UNKNOWN" RASTER

[robe]

strk,

Did you do anything with event trigger functions. I noticed the same problem with those, that if I move my extension to postgis schema, they are still stuck in public.

I confirmed this was not an issue before your change by upgrading from postgis 2.1.0 to postgis 2.1.1 (then moving to schema postgis). In that case the trigger event functions moved along. Trigger functions are

chechauthtrigger() 
postgis_cache_bbox()

I know that my logic for extension drops all functions from the extension package since the assumption is they will be recreated with CREATE OR REPLACE FUNCTION. So this suggests the upgrade script is no longer including these trigger functions.

[strk]

I think you should not drop anything explicitly, except obsoleted signatures (and ideally only those that have been obsoleted _exactly_ by the currently being built version). I haven't done anything (intentionally) with event triggers.

I'll try your testing. Sounds like something that could be automated as it doesn't rely on any older version, right ? Do you think you could encode it into a script ?

[strk]

The query to find "ST_*" named aggregates and their schema:

select n.nspname, p.proname, p.proargtypes from pg_proc p, pg_namespace n where p.pronamespace = n.oid and p.proisagg and p.proname ~ 'st_.*';

I confirm your finding with current 2.1 branch.

Seems to be only a problem with postgis, not with raster:

 nspname |     proname      |   proargtypes
---------+------------------+-----------------
 public  | st_makeline      | 1773725
 public  | st_polygonize    | 1773725
 public  | st_collect       | 1773725
 public  | st_union         | 1773725
 public  | st_accum         | 1773725
 public  | st_memunion      | 1773725
 public  | st_memcollect    | 1773725
 public  | st_3dextent      | 1773725
 public  | st_extent        | 1773725
 postgis | st_union         | 1774451 25
 postgis | st_union         | 1774451
 postgis | st_union         | 1774451 23
 postgis | st_union         | 1774451 23 25
 postgis | st_union         | 1774451 1774905
 postgis | st_samealignment | 1774451
(15 rows)

I'm looking at it.

[strk]

Ok, I see the problem. Your 2.1.2dev—2.1.2devnext.sql script _assumes_ that aggregates are ALWAYS created, while now they are only created IF they need be. And that's intentional, because the subject of this bug is infact being unable to upgrade while having views defined taht _depend_ on those aggregates.

So I think the actual fix would be dropping the assumption, which I belive is built into the call to SELECT postgis_extension_remove_objects('postgis', 'AGGREGATE');

Similarly all other "drops" for extension should be made version-dependent. What's the rationale for extensions NOT using the same scripts used for extension-less upgrades ?

[strk]

Robe I see 2 issues here:

1. The same file (upgrade_prev_this.sql) is used for both micro and minor upgrades
2. Drop of aggregates is forced rather than only performed if also performed by the input, like it's done for casts using sed script in Makefile.in

[strk]

Robe, with r12256 the extension-specific drop of objects is removed, which fixes your specific test. I guess we need some other test to prove that wrong ?

Theoretically, all drops should be already happening in the upgrade procedures, so the only thing that should be extension-specific is to drop from the extension before dropping for real, right ? That part could be done using sed as done for casts already..

[strk]

robe, btw… had you tried regress/run_test.pl —extension any recently ? It seems to be failing despite the upgrade…

[strk]

So for the record, as of r12261 in trunk and r12264 in 2.1 branch you can now use —extension —upgrade switches of run_test.pl to test both install and micro-upgrade using extension. Of course you need to "make install" first for that to work.

You can run the whole testsuite, after make install, with:

make check RUNTESTFLAGS='-v --extension --upgrade'

[robe]

Thanks for doing this strk. I'll test later in the week and revise the bots to test.

What kind of failure are you getting?

Can you add to the docs this new make check option? or I can do later in week.

[strk]

No problem, I was under the "hack fever" spell. Failure I was getting was reported as #2651

The make check options are really not new: RUNTESTFLAGS env variable will be used to pass switches to run_test.pl, available switches can be seen running it with no arguments. I've only "fixed" —upgrade when used togheter with —extension to really use the extension-oriented upgrade procedure.

Looking forward for your test results.

Note that the extension upgrades are still being inferior to straight scripts in that they don't distinguish between minor and micro updates, so they might drop/recreate more than they should. That part I hadn't touched (the Makefile was too crowded for me to touch w/out rewriting completely

[strk]

Overnight I've been thinking that the "source" version passed to postgis_proc_upgrade.pl to generate the *upgrade_<current>_minor.sql script is confusing, because a "minor" upgrade should be for upgrading from any 2.0.0 to any 2.x.x, while 2.1.0 to 2.1.x would be a "micro" upgrade.

The way I encoded it, instead, was to assume "source" was == "target" for a "minor" upgrade, meaning that aggregates added in a minor release (for example) would never be added by the upgrade script.

That said, I start wondering if we should do the version check at runtime, using DO constructs. Doing it at runtime would avoid proliferation of a lot of different X_to_Y combination scripts

[strk]

NOTE: part of the problem with minor vs micro is the name of variables "PREV_big" and "CURV_big" which actually points at _minor_, not "big" (major?) version numbers..

[robe]

Yah I had the DO dream check 2 years ago before I got shot down.

[strk]

Would CREATE and DROP calls within a DO block work nicely with extension ?

[strk]

I've a working experimental script looking for version on the live database itself. Ready to test ?

[strk]

See how you like r12271. With that commit the _upgrade_* scripts should be back to all being the same, no matter which version we come from, because the version is checked by the script itself (in plpgsql).

This worked fine:

run_test.pl —extension —upgrade ticket.sql

I guess you know how to test better…

[robe]

CREATE DROP calls work fine as long as you drop from extension before you recreate.

[strk]

I'm happy with what we have now, as per "view xxx depends on function yyy", so I'm closing. Feel free to file new tickets if new bugs come out. Thanks!

[robe]

I'll put this as a separate ticket since 2.1 seems sorta okay. It's 2.2 that throwing up blood.