Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#3821 closed defect (fixed)

Don't drop st_union when updating postgis extension to 2.4

Reported by: nextstopsun Owned by: robe
Priority: medium Milestone: PostGIS 2.4.0
Component: postgis Version: 2.3.x
Keywords: Cc:

Description

When doing an ALTER EXTENSION postgis UPDATE TO '2.4.0dev' I get an error ERROR: cannot drop function st_union(geometry) because other objects depend on it with a list of my views using this function.

Can it be upgraded without dropping it first? Otherwise I'll have to recreate a lot of objects in my DB.

This line causes trouble: DROP AGGREGATE IF EXISTS ST_Union (geometry)

Change History (26)

comment:1 by robe, 7 years ago

Unfortunately short of mucking with the system catalogs I don't think so.

PostgreSQL doesn't have a create or replace agg like it does for functions and in 2.4 we marked most aggs parallel safe so they need drop/recreate for the changes to take effect.

and alter aggregate doesn't give us much power except to rename aggs.

https://www.postgresql.org/docs/9.6/static/sql-alteraggregate.html

What you could do, if you are on PostgreSQL 9.5 or lower these extra clauses aren't in there so in theory you shouldn't need to drop, so you could just remark out the DROP AGG .. CREATE AGG in the extension scripts.

comment:2 by robe, 7 years ago

Resolution: wontfix
Status: newclosed

I'm going to mark this as wontfix for now as much as I hate to. But I will note as a breaking change in the NEWS.

comment:3 by robe, 7 years ago

Milestone: PostGIS 2.4.0PostGIS 2.5.0
Resolution: wontfix
Status: closedreopened

comment:4 by robe, 7 years ago

Owner: changed from pramsey to robe
Status: reopenednew

Tell yah what we probably can't fix this for PostGIS 2.4, but given that all the views source is stored in information_schema.views.view_definition

I think we might be able to at least in future provide a function people can run to generate the drop and recreate of views affected. The tricky part is that views often depend on other views so they have to be loaded in order.

comment:5 by robe, 7 years ago

In 15601:

note that the drop and recreate of views is a breaking change
References #3821

comment:6 by nextstopsun, 7 years ago

Can you give some more details about the update process? st_union can't be dropped in separate transaction, because postgis extension depends on it. Dropping the whole postgis extension means I have to drop almost everything in my DB.

So dropping and recreation of st_union has to be in one transaction, right? Then why does it throw error when dropping it without reaching the end of transaction.

comment:7 by nextstopsun, 7 years ago

And I've got cascade of dependent matviews, so that case with recreation ordering is exactly my case.

comment:8 by robe, 7 years ago

I thought we had logic in place to DROP the aggregate from the extension before dropping the aggregate, but not seeing that logic here anymore so perhaps the extension machinery takes care of that now (it didn't in early days) like it does with CREATE where it is added to the extension if it is created during extension upgrade transaction and it is not already part of the extension.

If you were working outside of the extension upgrade machinery, you would have to do:

ALTER EXTENSION postgis DROP AGGREGATE st_union(geometry);
DROP AGGREGATE st_union(geometry);

CREATE AGGREGATE st_union(geometry) (
  SFUNC=pgis_geometry_accum_transfn,
  STYPE=pgis_abs,
  FINALFUNC=pgis_geometry_union_finalfn
);
ALTER EXTENSION postgis ADD AGGREGATE st_union(geometry);

As far as why it fails before hitting end of transaction, I suspect DDL transactions always check dependencies even if the after state still satifies initial state because it can't absolutely be sure.

So which version of PostgreSQL are you using?

comment:9 by nextstopsun, 7 years ago

I'm on 9.6. I've got an early postgis 2.4.0dev, but I need some recent ST_AsMVTGeom fixes, so now I face this DROP AGGREGATE problem. I understand it's mostly an upstream problem not being able to set aggregate properties. So if I'll accept the pain of killing my DB and recreating almost from scratch, should I get more of this when doing further extension updates? I can think of avoiding it by wrapping postgis functions in my own to decouple from view definitions, but it seems ugly to even think of this kind of workaround.

comment:10 by strk, 7 years ago

Isn't there any way to set the "parallel" flag against an existing aggregate ?

comment:11 by robe, 7 years ago

Sadly I'm afraid since we don't keep track of changes within dev, going from PostGIS 2.4.0dev to PostGIS 2.4.0devnext for example will always try to drop and recreate the aggs. Fussing with sys catalogs is beginning to feel more appealing to me now.

What you could do for now if you really don't want to drop and recreate the aggs (or you just need to do it for ST_Union) , is in the extension scripts — e.g. postgis—2.4.0devnext—2.4.0dev.sql

where it has

OR (
      204 = version_from_num AND version_from_isdev
    )

Wipe that out and it won't force drop and recreate aggs. Again you can maybe just do it for ST_Union since I think ST_ASMVT has some aggs and those changes you might want to do.

I have to lookup the sys catalogs but I think there is a way to directly update the sys catalogs to set the parallel safe bit on by directly updating the catalogs.

comment:12 by nextstopsun, 7 years ago

Many hopes in setting that bit instead of dropping stuff. I've got some complex stuff to recreate if that won't be an option.

comment:13 by strk, 7 years ago

How about this:

UPDATE pg_proc SET proparallel = 's'
WHERE oid = 'public.st_union(geometry)'::regprocedure;

comment:14 by robe, 7 years ago

That works for me strk:

When I do this from psql

\df+ ST_Union

it toggles to safe and setting it to 'u' switches it to unsafe.

Now is it worthwhile to make an exception for ST_Union that it's used enough in views that we should just do the unholy and update the system catalog just for this agg function for PostgreSQL 9.6+ users upgrading to 2.4.

and take out the changed flag on it or should we just advice folks steps to take to upgrade if they run into the issue?

nextstopsun — what do you think are our instructions okay for you to follow

1) remove the:

OR (
      204 = version_from_num AND version_from_isdev
    )

2) Do the update thing that strk mentioned — this will work too:

UPDATE pg_proc SET proparallel = 's'
WHERE oid = 'st_union(geometry)'::regprocedure;

comment:15 by strk, 7 years ago

Ideally we'd change our aggregate definition to contain enough information for the postgis_proc_upgrade.pl script to know how to handle things. We did this for OPCLASS (KNN) if I remember correctly, adding the "Availability" comment in a specific component of the definition (one or two operators).

Not sure if it is too late for that (was the PARALLEL thing already shipped?)

comment:16 by nextstopsun, 7 years ago

Thanks robe and strk, I've managed to update extension with commenting out st_union and setting parallel safe manually in pg_proc. What is so unholy in updating catalog anyway? Could the update script detect if it's just this proparallel column needs to be changed and change it instead of dropping and recreating the aggregate? AFAIK function signature didn't change, so no really need to drop.

comment:17 by strk, 7 years ago

Right, that's my proposal: don't drop but just mark parallel. In order to do this automatically we need to remove the "Changed:" label from CREATE AGGREGATE comment so that the script will not DROP it ( or change to "Changed Compatibly" or something like that, which doens't trigger the restart ) and do the PARALLEL bit in next statement, with direct catalog access.

comment:18 by robe, 7 years ago

Milestone: PostGIS 2.5.0PostGIS 2.4.0

Alright we'll just do this for ST_Union cause everyone just so loves ST_Union.

If people complain about other aggs we can add them too.

I like nextstopsum's idea of only updating if it is not set already. We might have to do that anyway for folks going from 9.4 to 10 using the pg_upgrade going from 2.4 to 2.4.

So I'll include script:

UPDATE pg_proc SET proparallel = 's'
WHERE oid = 'st_union(geometry)'::regprocedure AND proparallel = 'u';

and change the note so our proc upgrade helper doesn't try to help out.

comment:19 by nextstopsun, 7 years ago

robe Are you sure proparallel column is u by default? Might there be NULL instead?

comment:20 by robe, 7 years ago

confirmed no nulls in that column.

select * from pg_proc where proparallel is null

yields no records.

From docs default for all aggs and functions is unsafe.

comment:21 by robe, 7 years ago

Resolution: fixed
Status: newclosed

In 15609:

Don't drop st_union when updating postgis extension to 2.4
Closes #3821 for PostGIS 2.4.0

comment:22 by strk, 7 years ago

Why not just using WHERE ≠ 's' ?

comment:23 by robe, 7 years ago

Because if it's anything other than 'u' it means the person intentionally set it so (like set to restricted) and so probably shouldn't be touched though I'm indifferent with changing feel free to change if you want.

comment:24 by robe, 5 years ago

Someone complained about this recently so putting a note here for others who stumble on this ticket

Going from PostGIS 2.4 → 2.5 requires a drop aggregate — so sadly yes you'll need to rebuild your views.

However come PostGIS 3.0 / PostgreSQL 12 with the introduction of CREATE OR REPLACE AGGREGATE, you should be able to go from PostGIS 2.anything to PostGIS 3.0 per #4352 without having to rebuild your views

in reply to:  11 comment:25 by dmkaplan2000, 5 years ago

Replying to robe: Hi,

I know this has been closed for a while, but I am running into this problem upgrading from Postgis 2.4.3 to Postgis 2.5.2 on a Postgresql 10 cluster on Ubuntu. I have desperately been trying to implement your solution of simply not dropping the ST_Union(geometry) aggregate, but I am obviously not editing the correct script as it has been having no effect. Would you mind indicating the correct file to edit? I have tried editing:

/usr/share/postgresql/10/extension/postgis—ANY—2.5.2.sql /usr/share/postgresql/10/contrib/postgis-2.5/postgis_upgrade_for_extension.sql

as well as several other similar file, but nothing seems to happen.

Thanks!

Sadly I'm afraid since we don't keep track of changes within dev, going from PostGIS 2.4.0dev to PostGIS 2.4.0devnext for example will always try to drop and recreate the aggs. Fussing with sys catalogs is beginning to feel more appealing to me now.

What you could do for now if you really don't want to drop and recreate the aggs (or you just need to do it for ST_Union) , is in the extension scripts — e.g. postgis—2.4.0devnext—2.4.0dev.sql

where it has

OR (
      204 = version_from_num AND version_from_isdev
    )

Wipe that out and it won't force drop and recreate aggs. Again you can maybe just do it for ST_Union since I think ST_ASMVT has some aggs and those changes you might want to do.

I have to lookup the sys catalogs but I think there is a way to directly update the sys catalogs to set the parallel safe bit on by directly updating the catalogs.

comment:26 by robe, 5 years ago

dmkaplan2000,

Sadly as mentioned about going from 2.4 to 2.5.2 DOES REQUIRE a DROP AGGREGATE. You don't even want to edit this script as that would put your system in an unstable state if you do not drop the aggregate.

What was discussed above was when we changed midstream in a development cycle, marking ST_Union as parallel safe — that change can be done with a catalog update and even if you don't not a huge deal — just means your ST_Union couldn't use paralle constructs.

Unfortunately going from 2.4 → 2.5 we changed the underlying helper functions so not a trivial catalog update. A PostgreSQL team member (Andrew aka (RhodiumToad) ), heard our pains and blessed us with an ALTER AGGREGATE SQL construct in PostgreSQL 12 which we can use for most cases. So if running PostgreSQL 12 or above we use ALTER AGGREGATE to make changes. That doesn't come till 3.0 though.

Note: See TracTickets for help on using tickets.