AddGeometryColumn inappropriately defaults to current_schema() if given nonexistent schema

[reid@…]

** What steps will reproduce the problem?

Execute the following SQL (assuming schema XXX does not exist):

create temp table foo ( ); select AddGeometryColumn?('XXX', 'foo', 'foopoint', 26915, 'POINT', 2);

** Actual behavior:

PostGIS sees that schema 'XXX' does not exist, then looks for table 'foo' in schema public, e.g. error message from above:

NOTICE: Invalid schema name - using current_schema() CONTEXT: SQL statement 'SELECT AddGeometryColumn?(, $1 , $2 , $3 , $4 , $5 , $6 )' PL/pgSQL function 'addgeometrycolumn' line 4 at SQL statement ERROR: relation 'public.foo' does not exist CONTEXT: SQL statement 'ALTER TABLE public.foo ADD COLUMN foopoint geometry ' PL/pgSQL function 'addgeometrycolumn' line 86 at execute statement SQL statement 'SELECT AddGeometryColumn?(, $1 , $2 , $3 , $4 , $5 , $6 )' PL/pgSQL function 'addgeometrycolumn' line 4 at SQL statement

As an aside, this error message is verbose and confusing; I initially missed the first line and was rather baffled that PostGIS insisted on trying 'public' when I had clearly asked for 'XXX'.

** Expected behavior:

PostGIS looks for table in given schema, fails, and gives up with an error, e.g. desired error message for above:

ERROR: schema 'XXX' does not exist

** What version of the product are you using? On what operating system?

Ubuntu Hardy PostgreSQL 8.2.7 on i486-pc-linux-gnu, compiled by GCC cc (GCC) 4.2.3 (Ubuntu 4.2.3-2ubuntu4) POSTGIS='1.3.3' GEOS='3.0.0-CAPI-1.4.1' PROJ='Rel. 4.6.0, 21 Dec 2007' USE_STATS

[kneufeld]

This is because this function is a wrapper for AddGeometryColumn?(table_name, column_name, srid, type, dimension), where the schemaname is obtained from the search path. You'd prefer that this is not the case? The alternative would be duplicate the code to the schema-qualified variant and simply error on an incorrect schemaname.

If others are with you on this behaviour, it's certainly possible to change this.

As for the CONTEXT messages: whenever you raise a notice or error in plpgsql, a context message is also displayed. Unless someone knows how to separate this verbose debugging information from a regular notice or error message, this is the best we can do at the moment with plpgsql. I suppose we could move these functions to C and write error messages exactly as we see fit, but most people haven't seen the need for that yet.

Hope this helps, Kevin

[reid@…]

This is because this function is a wrapper for AddGeometryColumn?(table_name, column_name, srid, type, dimension), where the schemaname is obtained from the search path. You'd prefer that this is not the case? The alternative would be duplicate the code to the schema-qualified variant and simply error on an incorrect schemaname. If others are with you on this behaviour, it's certainly possible to change this.

Actually (in 1.3.3 at least), it seems that both AddGeometryColumn?(table_name, ...) and AddGeometryColumn?(schema_name, ...) are wrappers for a third variant, AddGeometryColumn?(catalog_name, schema_name, table_name, ...).

As for a fix: can the RAISE NOTICE on line 2840 of lwpostgis.sql be replaced with RAISE EXCEPTION and line 2841 deleted?

As for the CONTEXT messages: whenever you raise a notice or error in plpgsql, a context message is also displayed. Unless someone knows how to separate this verbose debugging information from a regular notice or error message, this is the best we can do at the moment with plpgsql.

That's a minor problem. I wouldn't be too sad if the CONTEXT remained.

[kneufeld]

Actually (in 1.3.3 at least), it seems that both AddGeometryColumn?(table_name, ...)

Yup, you're right, I was just simplifying my response.

As for a fix: can the RAISE NOTICE on line 2840 of lwpostgis.sql be replaced with

RAISE EXCEPTION and line 2841 deleted?

Ah, I see what you're saying... yup, that works. You should note that this has already been fixed in SVN trunk. This function was redone to remove all references to current_schema() in favour of pg_table_is_visible() a while back.  http://code.google.com/p/postgis/issues/detail?id=74&can=1

If you want this fixed in your 1.3 branch, apply the attached patch. I don't think we can actually apply this patch to SVN since this is a behavioural change (albeit small) to the function. I would apply this to 1.4, but this has already been addressed.

If it ok with you, I'll close this ticket.

Cheers, Kevin

[kneufeld]

This issue is already addressed in trunk (the next minor release). Applying the patch to 1.3 would change the behaviour of the method, which is not appropriate for a micro release.

[reid@…]

Kevin,

Very glad to hear it'll be fixed in 1.4.

Applying the patch to 1.3 would change the behaviour of the method, which is not appropriate for a micro release.

Without knowing the PostGIS policy on this (it's certainly not that changing the behavior of methods is out, since that excludes all bugfixes), I'd like to push back a bit: the current behavior is not documented, so I don't see how it changes the advertised interface, and I do believe it violates the principle of least surprise rather severely.

It could also lead to incorrect behavior: suppose that I have two tables, pg_temp1.foo_tmp and public.foo. If I call AddGeometryColumn?('temp', 'foo', ...) -- i.e., I got the name of the temp tablespace wrong and I forgot the _tmp suffix -- then it would change the wrong table (foo instead of foo_tmp). This scenario could arise if (say) I created a temp copy of foo to do some analysis.

I agree that there are bigger fish to fry, but it's a trivial patch.

If it ok with you, I'll close this ticket.

Not sure if you were actually waiting for me on this, but if you were, 17 hours doesn't seem a long enough wait for someone to reply.

Thanks,

Reid

[kneufeld]

Hi Reid,

I appreciate your feedback. You're right that this subtle case is not well documented (something I just fixed in the trunk docs). Fair enough, it's a subtle change and could be classified as a bug since an error is being thrown anyway. I just committed the patch to 1.3

Cheers, Kevin

[reid@…]

 http://postgis.googlecode.com/issues/attachment?aid=8008922217746647594&name=lwpostgis.sql.in.patch

[reid@…]

Thanks, Kevin.

Not sure if this is the right venue, but I would like to express my thanks for your (and everyone else's) work on PostGIS. It's absolutely indispensable for us; there's simply no way our project would exist without it.