Ticket #484 (closed task: fixed)

Opened 3 years ago

Last modified 6 months ago

[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: trunk
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

Changed 3 years ago by robe

  • summary changed from rtpostgis.sql.in.c using old style 1.3 syntax and other arcane stuff to [wktraster] rtpostgis.sql.in.c using old style 1.3 syntax and other arcane stuff

Changed 3 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.

Changed 3 years ago by pracine

1 and 4 done...

Changed 3 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 $$.

Changed 3 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.

Changed 3 years ago by pracine

  • milestone changed from WKTRaster Future to PostGIS 2.0.0

Changed 3 years ago by pracine

  • status changed from new to assigned

Changed 3 years ago by pracine

  • summary changed from [wktraster] rtpostgis.sql.in.c using old style 1.3 syntax and other arcane stuff to [raster] rtpostgis.sql.in.c using old style 1.3 syntax and other arcane stuff

Changed 3 years ago by jorgearevalo

2 and 5 done.

3) We need to list the functions that need cost

Changed 2 years ago by pracine

  • milestone changed from PostGIS 2.0.0 to PostGIS Raster Future

Changed 15 months ago by pracine

  • milestone changed from PostGIS Raster Future to PostGIS Future

Changed 6 months ago by pracine

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

Changed 6 months ago by dustymugs

  • status changed from assigned to closed
  • resolution set to fixed
  • milestone changed from PostGIS Future to PostGIS 2.1.0

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.