Opened 10 years ago

Closed 10 years ago

#4870 closed defect (fixed)

Broken SQL passed to -sql and run by PostgreSQL driver result in SUCCESS exit code

Reported by: strk Owned by: warmerdam
Priority: normal Milestone: 1.10.0
Component: Utilities Version: 1.9.2
Severity: normal Keywords: ogr2ogr sql
Cc:

Description

Requesting an invalid SQL to the PG driver throws an error message on stderr but doesn't trigger FAILURE exit code. Example:

$ ogr2ogr -f ES'RI Shapefile' tmp 'PG:dbname=template1' -sql 'select as foo' && echo SUCCESS
ERROR 1: ERROR:  syntax error at or near "as"
LINE 1: select as foo
               ^

SUCCESS

Attachments (1)

0001-Have-ogr2ogr-exit-with-FAILURE-code-on-broken-sql-48.patch (734 bytes ) - added by strk 10 years ago.
proposed patch

Download all attachments as: .zip

Change History (8)

comment:1 by strk, 10 years ago

I don't see traces of OGRPGDataSource::ExecuteSQL being called by the above commandline, is it expected ?

comment:2 by strk, 10 years ago

Never mind, now I do see it (was installing in the wrong dir)

comment:3 by strk, 10 years ago

Ok, so ExecuteSQL correctly returns NULL, bug apps/ogr2ogr.cpp doesn't seem to be considering that NULL as a sign of a fatal failure. In which cases it isn't fatal ?

comment:4 by strk, 10 years ago

I made the patch against 1.9.2 -- also tried to provide a pull request but it looks like my git version of gdal is still not setup correctly

comment:5 by Even Rouault, 10 years ago

NULL is a legit return value for ExecuteSQL() in some cases like INSERT, UPDATE, VACCUUM, etc.. statements. But that's more for the ogrinfo -sql case. In ogr2ogr, only SELECT makes sense, and SELECT should never return NULL on success (even if there are 0 rows returned).

In case I missed something (which would be a corner use case), to be really safe, one approach would be to exit(1) only if NULL has been returned by ExecuteSQL() and that CPLGetLastErrorNo() != 0.

comment:6 by strk, 10 years ago

My patch is for ogr2ogr.cpp so wouldn't affect ogrinfo. I confirm CPLGetLastErrorNo() !=0 in my case so I'm fine with that addition. Want me to send another patch or can you do that ?

comment:7 by Even Rouault, 10 years ago

Component: defaultUtilities
Keywords: ogr2ogr sql added
Milestone: 2.0.0
Resolution: fixed
Status: newclosed

Fix committed in r25149

Note: See TracTickets for help on using tickets.