Opened 11 years ago

Closed 9 years ago

#484 closed task (fixed)

[raster] rtpostgis.sql.in.c using old style 1.3 syntax and other arcane stuff

Reported by: robe Owned by: pracine
Priority: low Milestone: PostGIS 2.1.0
Component: raster Version: main
Keywords: Cc:

Description

This I'm not sure you want to change just yet and I forget why we had this convention in 1.3. I think it was to support older PostgreSQL installs like 8.1.

I see in your function you have

CREATEFUNCTION st_convexhull(raster)
    RETURNS GEOMETRY
    AS 'MODULE_PATHNAME','RASTER_convex_hull'
    LANGUAGE 'C' _IMMUTABLE_STRICT;

We do now (though I guess you may want to leave costs out if you want 8.2 users to be able to use it)

CREATE OR REPLACE FUNCTION ST_ConvexHull(geometry)
	RETURNS geometry
	AS 'MODULE_PATHNAME','convexhull'
	LANGUAGE 'C' IMMUTABLE STRICT
	COST 100;

On a side note, I see you have inherited our bad habits of not naming the variables going into input. This was done a long time ago I think pre 8.0 before it was okay to put names of variables in the input. Probably not too worthwhile changing now except its confusing reading it when you see integer that that goes to band number or float8 is x etc. Again postgis sql.in.c files are more guilty of this atrocity.

Also you are not always using $ quoting syntax which is much easier to read and has been supported since PostgreSQL 8.0. Admittedly the whole PostGIS code sql file is guilty of this atrocity and worse so so can't complain too much.

Just thought I'd butt my nose in :)

Change History (13)

comment:1 Changed 11 years ago by robe

Summary: rtpostgis.sql.in.c using old style 1.3 syntax and other arcane stuff[wktraster] rtpostgis.sql.in.c using old style 1.3 syntax and other arcane stuff

comment:2 Changed 11 years ago by pracine

So many things here:

1) Replace "CREATEFUNCTION" with "CREATE OR REPLACE FUNCTION". Actually all the macros defined at the beginning of the file with their real values.

2) Lower case the function names. I checked the PostGIS file and there is a mix of camel case and lowercase names. What to do when?

3) Add a cost. Again I checked the PostGIS file and there is a mix of with and without cost. What to do when?

4) Name the input variables.

5) Use $$ instead of ' to delimitate code.

comment:3 Changed 11 years ago by pracine

1 and 4 done...

comment:4 Changed 11 years ago by pracine

Need more info to do 2, 3 and 5:

2) Is there any rule to camel case the function names? postgis.sql has a mixture of camelcase/lowercase

3) postgis.sql has a mixture of with/without cost. What is the rule?

5) Again what is the rule? Should we blindly replace all ' with $$ in all type of function (C and SQL). All plpgsql functions now use $$.

comment:5 Changed 11 years ago by robe

2) Its a bit easier to read camel case I think and would also match the documentation which is camel case. In the end it doesn't matter too much since when it gets into PostgreSQL it all ends up being lowercase by PostgreSQL.

3) Not all functions need costing. The ones that take a lot of time that are non-SQL functions should have costing where there are conditions where they may not need to be run. I'm not sure we have any of those at this point. Probably best to just wait off on this. For SQL functions -- generally you don't want to cost them since they often get dissolved by the planner into constituent parts.

5) Yes blindly.

comment:6 Changed 11 years ago by pracine

Milestone: WKTRaster FuturePostGIS 2.0.0

comment:7 Changed 11 years ago by pracine

Status: newassigned

comment:8 Changed 11 years ago by pracine

Summary: [wktraster] rtpostgis.sql.in.c using old style 1.3 syntax and other arcane stuff[raster] rtpostgis.sql.in.c using old style 1.3 syntax and other arcane stuff

comment:9 Changed 11 years ago by jorgearevalo

2 and 5 done.

3) We need to list the functions that need cost

comment:10 Changed 10 years ago by pracine

Milestone: PostGIS 2.0.0PostGIS Raster Future

comment:11 Changed 9 years ago by pracine

Milestone: PostGIS Raster FuturePostGIS Future

comment:12 Changed 9 years ago by pracine

In the "Closing very old ones" series, I guess we can close this one too?

comment:13 Changed 9 years ago by Bborie Park

Milestone: PostGIS FuturePostGIS 2.1.0
Resolution: fixed
Status: assignedclosed

Might as well close this. Everything is done except costs but that requires significant real world testing. Something objective would be nice...

Note: See TracTickets for help on using tickets.