Opened 8 years ago

Closed 8 years ago

#1343 closed defect (fixed)

[raster] raster Upgrade script doesn't work because of new type

Reported by: robe Owned by: robe
Priority: high Milestone: PostGIS 2.0.0
Component: raster Version: master
Keywords: Cc:

Description

I see someone created a bandmetadata type. Can we get rid of it? and instead use out parameters. The band metadata function that uses it already has OUT parameters so actually may cause a problem if those get out of synch. Though maybe this isn't possible since the first is exposed via a C function.

I also don't see the point of having the C function use a variadic int. Why can't it just atake an array of band numbers so it's consistent with ST_Band?

As a general rule, we should avoid creating types unless they are used a lot or are complex objects such as raster.

The problem with types is that they are difficult to upgrade since PostgreSQL has no CREATE IF NOT EXISTS yet for types and even then it would be impossilbe to change them. We could create functions to check the system catalogs and create them if they don't exist etc, but that gets messy.

Change History (12)

comment:1 Changed 8 years ago by Bborie Park

Resolution: fixed
Status: newclosed

OK. I've removed the type in r8295.

comment:2 Changed 8 years ago by robe

Resolution: fixed
Status: closedreopened

Same issue with geomvalxy. I'll take care of this one Bborie since I see you have your hands full :)

comment:3 Changed 8 years ago by robe

Owner: changed from pracine to robe
Status: reopenednew

comment:4 Changed 8 years ago by Bborie Park

Oh. The only reason ST_Band isn't variadic while ST_BandMetadata is is that when I wrote ST_Band, we were still supporting PGSQL 8.3. Don't tempt me to go make ST_Band variadic ;-)

I like the uber-flexibility that variadic parameters permit...

ST_BandMetadata(rast, 1, 2, 3, 4, 5)

or

ST_BandMetadata(rast, VARIADIC ARRAY[1,2,3,4,5])

comment:5 Changed 8 years ago by robe

I don't for the plain reason that I can't do

array_agg(number_field) or ARRAY[].. which is important when you are dealing with a rast that you don't necessarily know the number of bands you will need before hand.

which I can with arrays

Granted it's not as simple as writing 1,2,3, but with variadic for this kind of thing, I loose one level of flexibility.

comment:6 Changed 8 years ago by robe

oh forgot about the ARRAY(SELECT num from ...) option. That you loose too with making this variadic.

comment:7 Changed 8 years ago by Bborie Park

Valid arguments. OK. I've removed the variadic from ST_BandMetadata.

comment:8 Changed 8 years ago by Bborie Park

Variadic removed in r8300

comment:9 Changed 8 years ago by robe

okay got rid of use of geomvalxy and purged it though still need to add that st_pixelaspolygons or whatever to regress to make sure I didn't break it.

Sadly there is a more difficult one to get rid of so upgrade still doesn't work so I may still need to make concessions for adding new types if they don't exist.

The ST_union aggregate function also introduces a new type rastexpr which it uses for a state transition and all over the place. I'm not sure if that can be irradicated. I think anyway the ST_Union needs a closer look which I'll try to take a look at hopefully this weekend or early next week, because its got WAY TOO MANY moving parts and the internal funcs aren't prefixed with _ to denote they are internals.

comment:10 Changed 8 years ago by robe

forgot changes I made are at r8302

comment:11 Changed 8 years ago by Bborie Park

Summary: raster Upgrade script doesn't work because of new type[raster] raster Upgrade script doesn't work because of new type

comment:12 Changed 8 years ago by robe

Resolution: fixed
Status: newclosed

this works for me now cleanly. Though puzzled what kind of issue Chris could be having in #1372 since my upgrade seems to be working cleanly now for both extension upgrade and using the upgrade script.

Note: See TracTickets for help on using tickets.