Extension upgrade needs to selectively drop from extension aggregates and functions

[robe]

I'm guessing this is a problem, but I won't know for sure since we haven't changed aggregates and types in a while.

I'm not seeing it in the script — before we'd just drop all aggregates and functions from the extension with assumption they will be readded. Now we can't do that anymore — have to every where we have a DROP AGG .. DROP FUNCTION .. called, explicitly drop it from the extension before doing the actual drop.

[strk]

That's what I was asking in IRC about "do extension cleanly handle 'DROP'" … You can test this with the new —upgrade-path switch of run_test.pl. If I'm not mistaken there's a script to replace DROPs with "DROP FROM extension; DROP..", not sure if that script runs over the whole upgrade script, needs to be checked.

So please let me know which upgade path shows the problem for you.

[robe]

none I have tested or think I CAN test since we haven't dropped anything that isn't in the drop before/after scripts. I just scanned the generated extension script and expected for example where you have your autogenerated $postgis_proc_upgrade$ DOs

like this case

EXECUTE 'DROP AGGREGATE IF EXISTS ST_Extent(geometry)';

it should be:

SELECT postgis_extension_drop_if_exists('postgis', 'DROP AGGREGATE IF EXISTS  ST_Extent(geometry);')
EXECUTE 'DROP AGGREGATE IF EXISTS ST_Extent(geometry)';

My sed script doesn't replace that construct since I think I only touch the drop/before after scripts to prevent from messing up plpgsql functions.

Since you are doing an EXECUTE anyway wondering if maybe we should move the postgis_extension_drop_if_exists function up (rather create a new one that does your DO and drops from extension) as part of regular upgrade machinery and you just change your EXECUTE to use that new function.

The only reason I had to create that function is because the drop from extension logic doesn't have a DROP IF EXISTS option, so the function just tries to drop from the extension and ignores if it gets an error (that means the extension doesn't have it).

[strk]

Where is your sed script ?

[robe]

Hmm what was I thinking. I just noticed I only do it for raster.

in extensions/postgis/makefile.in

sql_bits/rtpostgis_upgrade.sql: ../../raster/rt_pg/rtpostgis_upgrade.sql
	sed -e 's/BEGIN;//g' -e 's/COMMIT;//g' \
		-e 's/DROP FUNCTION _rename_raster_tables();/ALTER EXTENSION ${EXTENSION} DROP FUNCTION _rename_raster_tables();DROP FUNCTION _rename_raster_tables();/g' \
		-e 's/DROP FUNCTION _drop_st_samealignment();/ALTER EXTENSION ${EXTENSION} DROP FUNCTION _drop_st_samealignment();DROP FUNCTION _drop_st_samealignment();/g' \
		-e 's/DROP CAST\(.*\)/SELECT postgis_extension_drop_if_exists('\''$(EXTENSION)'\'', '\''DROP CAST \1'\'');DROP CAST \1/' \
	$< > $@

I know sloppy. But still I think we'll need to define logic that if you are running PostgreSQL 9.1+, you need to try to drop from postgis extension as well as whatever other drop you are doing and move this logic into the upgrade generation script.

For 2.2 I 9.1 is our minimum version so we can just blindly try to drop from extension.

[strk]

how about removing the "extension" word from that function and having it do extension removal if within extension context and direct removal otherwise ? Such function could very well go into the upgrade script.

As the DROP for obsoleted functions would not belong in the postgis_proc_upgrade.pl script, there's still a place for your postprocessing. I'd rather make the postgis_proc_upgrade.pl output compatible with your sed script.

Even better if your sed script becomes a standalone filter taking are of _any_ drop.

[strk]

Robe I need you to find an actual bug here, or it's a cleanup task. Functions are always created (create or replace being supported). Casts, according to your sed script, are only dropped _IF_ the drop is also found in the upgrade script, so I don't see a problem.

Closing as won't fix, feel free to reopen/retarget/reassign