mappostgis.c: BEGIN transaction once in msPOSTGISLayerOpen rather than repeatedly in prepare_database

[dfuhry]

Currently, mappostgis.c in prepare_database() issues a PQexec(layerinfo->conn, "BEGIN") every time a postgis layer is rendered, even if CLOSE_CONNECTION=DEFER is set for the layer.

Since the db connection is recycled, this makes unnecessary calls to the database and results in a:

WARNING: there is already a transaction in progress

message in the postgresql log file each time a postgis layer is rendered. The per-layer call gives no extra safety since a redundant BEGIN will not start a new transaction, nor will one "fix" a transaction in which an error has occurred.

All postgis queries are "DECLARE CURSOR ..." so a transaction is necessary, but the transaction can be started when the layer is opened. The proposed patch moves the PQexec(layerinfo->conn, "BEGIN") call to msPOSTGISLayerOpen(), so that it will be issued only once when the connection is opened.

Tested on Linux with CLOSE_CONNECTION=DEFER layers and FastCGI.

[richf]

Fwiw, I confirmed that applying the attached patch locally to mapserver 5.0.0 on debian 3.1 with postgresql 8.1 and postgis 1.1.2 got rid of the warnings when using CLOSE_CONNECTION=DEFER layers and java mapscript.

Discussed further in this mailing list thread: ​http://www.nabble.com/postgres-transaction-warnings-to15749333.html#a15765216

And another related mailing list thread is: ​http://www.nabble.com/Segmentation-fault---mapserver---postgis-to12653945.html#a12674499

Might this be a simple enough patch to also include in the 5.0.3 release ?

[sdlime]

I'd really like to get confirmation from Paul or somebody at Refractions that this is ok. If so this would be fine for 5.0.3 so I'll change the milestone now.

Steve

[richf]

Fwiw, further evidence of the patch working (when applied to the 5.0.2 release) can be found in this mailing list thread:

​http://www.nabble.com/CLOSE_CONNECTION%3DDEFER----Segmentation-fault-to15786741.html

[richf]

fwiw, in case this might speed along checkin of this patch into either branch-5-0 and/or the trunk, i can report that i have also had success running long stress tests under heavy load against both svn r7429 of the trunk plus this patch, as well as svn r7432 of branch-5-0 plus this patch plus a patch for bug 2533.

these tests were performed in the context of bug 2533.

[richf]

re: comment 4

but this might be a sign of a problem with the patch: ​http://www.nabble.com/CLOSE_CONNECTION%3DDEFER----Segmentation-fault-to15786741.html

[dfuhry]

I posted this in response to Ivan's problem report on mapserver-users. fastcgi may need to be restarted to avoid pulling old connections (with no transaction started) from the connection pool.

Nothing is perfect. There are no problems with applied patch when browsing the map. But when I use identify object query from php5-mapscript the problem is rising again.

I have in postgresql log:

2008-03-05 13:19:30 CET WARNING: there is no transaction in progress

2008-03-05 13:19:30 CET ERROR: cursor "mycursor" does not exist 2008-03-05 13:19:31 CET ERROR: DECLARE CURSOR may only be used in transaction blocks ...

Thanks for the problem report. If you are using fastcgi, you may need to restart it.

The only two places PQconnectdb is called is in msPOSTGISLayerOpen and in msPOSTGRESQLJoinConnect, which operates independently, not using cursors or the connection pool. With a patched msPOSTGISLayerOpen, PQexec(layerinfo->conn, "BEGIN") is always called right after connecting, before any queries and before the connection ends up in the connection pool.

So I'm wondering if you've put the new mapserv executable in your cgi-bin, but not restarted the webserver, and it's pulling old connections out of the fastcgi connection pool, which have not had PQexec(layerinfo->conn, "BEGIN") issued to them (and thus, no transaction started).

Try restarting fastcgi. Depending on your configuration, restarting your webserver may automatically restart fastcgi, or you may have to specifically restart the fastcgi service/daemon on that machine.

[dfuhry]

I updated the diff file attached to this ticket. The new code resolves invalid transaction state in a more comprehensive manner.

A new local function msPOSTGISInitConnection takes whatever reset / ROLLBACK / BEGIN steps necessary to fix an unusable connection. Calls to PQreset() in the current code have been replaced with calls to msPOSTGISInitConnection, so that after a failing query, the connection is always sanitized before being put back into the connection pool.

I have not tested it heavily; good things to test would be broken DATA query strings or other queries which cause a database error. It should fix the known problems to date, including the problem whereby a transaction was ended because of a post-msPOSTGISLayerGetShape ROLLBACK, and later queries would pull the non-transactional connection from the mappool.

Testing & comments welcome.

[richf]

Is the diff for the trunk, or for branch-5-0 ?

It's a little confusing b/c the Version says svn-trunk, but the milestone says 5.0.3.

[dfuhry]

The diff is against trunk. I think "Version" means the (latest) version of mapserver with the defect, and "Milestone" means the mapserver release that we hope to get the patch applied for.

I just updated the patch to apply cleanly to HEAD. There was some unrelated junk in the header.

[dfuhry]

new version of patch, which doesn't clobber necessary query in msPOSTGISLayerGetShape

[pramsey]

Looks fine given a quick read... apply away, from my PoV.

[dfuhry]

updated version of patch with extra error handling

[dfuhry]

New patch attached, with extra error checks as per Paul's comments here: ​http://lists.osgeo.org/pipermail/mapserver-dev/2008-April/007017.html

[dfuhry]

version 5 of patch; removes awkard code supporting postgis < 0.6

[dfuhry]

version 6 of patch with corrected debugging information

[pramsey]

v6 of the patch applied as of r7529.

[pramsey]

v6 of patch applied to branch-5-0 as of r7539