Opened 12 years ago

Closed 12 years ago

#4533 closed task (fixed)

PostGIS Raster driver: Cache connections

Reported by: dzwarg Owned by: dzwarg
Priority: normal Milestone:
Component: GDAL_Raster Version: svn-trunk
Severity: normal Keywords: postgis_raster
Cc: jorgearevalo

Description

Cache connections to the postgres database in the PostGISRasterDriver.

Also, use PG* environment variables as fallback when settings are not provided in the connection string.

Attachments (2)

conninfo.patch (41.4 KB ) - added by dzwarg 12 years ago.
Pool connections in the driver. Get all connections from the driver. This patch includes a fix for the bug reported to PostGIS Raster #1422. This patch also includes the patch for #4530.
conninfo.2.patch (42.0 KB ) - added by dzwarg 12 years ago.
This patch has been run through valgrind -- no memleaks detected. Rock'n'roll with connection caching.

Download all attachments as: .zip

Change History (12)

by dzwarg, 12 years ago

Attachment: conninfo.patch added

Pool connections in the driver. Get all connections from the driver. This patch includes a fix for the bug reported to PostGIS Raster #1422. This patch also includes the patch for #4530.

by dzwarg, 12 years ago

Attachment: conninfo.2.patch added

This patch has been run through valgrind -- no memleaks detected. Rock'n'roll with connection caching.

comment:1 by jorgearevalo, 12 years ago

Resolution: fixed
Status: newclosed

Patch applied in r24088. I close the ticket.

comment:2 by Even Rouault, 12 years ago

Resolution: fixed
Status: closedreopened

This crashes badly when the connection cannot be opened (I get a segfault when running wktraster.py with no postgis raster enabled database).

Here's the Valgrind trace :

$ valgrind gdalinfo "PG:host='localhost' dbname='gisdb' user='gis' password='gis' schema='gis_schema' "
==15194== Memcheck, a memory error detector
==15194== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==15194== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==15194== Command: gdalinfo PG:host='localhost'\ dbname='gisdb'\ user='gis'\ password='gis'\ schema='gis_schema'\ 
==15194== 
ERROR 1: Port parameter must be provided, or PGPORT environment variable must be set. Please set the port and try again.
==15194== Invalid read of size 1
==15194==    at 0x4C2A5AB: bcmp (mc_replace_strmem.c:889)
==15194==    by 0x580EF0A: VSICheckMarker(char*) (cpl_vsisimple.cpp:472)
==15194==    by 0x580F196: VSIFree (cpl_vsisimple.cpp:552)
==15194==    by 0x5648BBC: GetConnection(char const*, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1525)
==15194==    by 0x5648CF3: PostGISRasterDataset::Open(GDALOpenInfo*) (postgisrasterdataset.cpp:1579)
==15194==    by 0x577DF51: GDALOpenInternal(GDALOpenInfo&, char const* const*) (gdaldataset.cpp:2236)
==15194==    by 0x577DE04: GDALOpenInternal(char const*, GDALAccess, char const* const*) (gdaldataset.cpp:2208)
==15194==    by 0x577DDBE: GDALOpen (gdaldataset.cpp:2199)
==15194==    by 0x40328C: main (gdalinfo.c:173)
==15194==  Address 0x16608380 is 0 bytes inside a block of size 26 free'd
==15194==    at 0x4C2779F: free (vg_replace_malloc.c:427)
==15194==    by 0x580F246: VSIFree (cpl_vsisimple.cpp:572)
==15194==    by 0x56487B3: GetConnectionInfo(char const*, char**, char**, char**, char**, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1423)
==15194==    by 0x5648AB9: GetConnection(char const*, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1498)
==15194==    by 0x5648CF3: PostGISRasterDataset::Open(GDALOpenInfo*) (postgisrasterdataset.cpp:1579)
==15194==    by 0x577DF51: GDALOpenInternal(GDALOpenInfo&, char const* const*) (gdaldataset.cpp:2236)
==15194==    by 0x577DE04: GDALOpenInternal(char const*, GDALAccess, char const* const*) (gdaldataset.cpp:2208)
==15194==    by 0x577DDBE: GDALOpen (gdaldataset.cpp:2199)
==15194==    by 0x40328C: main (gdalinfo.c:173)
==15194== 
==15194== Invalid read of size 1
==15194==    at 0x580EF17: VSICheckMarker(char*) (cpl_vsisimple.cpp:476)
==15194==    by 0x580F196: VSIFree (cpl_vsisimple.cpp:552)
==15194==    by 0x5648BBC: GetConnection(char const*, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1525)
==15194==    by 0x5648CF3: PostGISRasterDataset::Open(GDALOpenInfo*) (postgisrasterdataset.cpp:1579)
==15194==    by 0x577DF51: GDALOpenInternal(GDALOpenInfo&, char const* const*) (gdaldataset.cpp:2236)
==15194==    by 0x577DE04: GDALOpenInternal(char const*, GDALAccess, char const* const*) (gdaldataset.cpp:2208)
==15194==    by 0x577DDBE: GDALOpen (gdaldataset.cpp:2199)
==15194==    by 0x40328C: main (gdalinfo.c:173)
==15194==  Address 0x16608383 is 3 bytes inside a block of size 26 free'd
==15194==    at 0x4C2779F: free (vg_replace_malloc.c:427)
==15194==    by 0x580F246: VSIFree (cpl_vsisimple.cpp:572)
==15194==    by 0x56487B3: GetConnectionInfo(char const*, char**, char**, char**, char**, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1423)
==15194==    by 0x5648AB9: GetConnection(char const*, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1498)
==15194==    by 0x5648CF3: PostGISRasterDataset::Open(GDALOpenInfo*) (postgisrasterdataset.cpp:1579)
==15194==    by 0x577DF51: GDALOpenInternal(GDALOpenInfo&, char const* const*) (gdaldataset.cpp:2236)
==15194==    by 0x577DE04: GDALOpenInternal(char const*, GDALAccess, char const* const*) (gdaldataset.cpp:2208)
==15194==    by 0x577DDBE: GDALOpen (gdaldataset.cpp:2199)
==15194==    by 0x40328C: main (gdalinfo.c:173)
==15194== 
==15194== Invalid read of size 1
==15194==    at 0x580EF25: VSICheckMarker(char*) (cpl_vsisimple.cpp:476)
==15194==    by 0x580F196: VSIFree (cpl_vsisimple.cpp:552)
==15194==    by 0x5648BBC: GetConnection(char const*, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1525)
==15194==    by 0x5648CF3: PostGISRasterDataset::Open(GDALOpenInfo*) (postgisrasterdataset.cpp:1579)
==15194==    by 0x577DF51: GDALOpenInternal(GDALOpenInfo&, char const* const*) (gdaldataset.cpp:2236)
==15194==    by 0x577DE04: GDALOpenInternal(char const*, GDALAccess, char const* const*) (gdaldataset.cpp:2208)
==15194==    by 0x577DDBE: GDALOpen (gdaldataset.cpp:2199)
==15194==    by 0x40328C: main (gdalinfo.c:173)
==15194==  Address 0x16608382 is 2 bytes inside a block of size 26 free'd
==15194==    at 0x4C2779F: free (vg_replace_malloc.c:427)
==15194==    by 0x580F246: VSIFree (cpl_vsisimple.cpp:572)
==15194==    by 0x56487B3: GetConnectionInfo(char const*, char**, char**, char**, char**, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1423)
==15194==    by 0x5648AB9: GetConnection(char const*, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1498)
==15194==    by 0x5648CF3: PostGISRasterDataset::Open(GDALOpenInfo*) (postgisrasterdataset.cpp:1579)
==15194==    by 0x577DF51: GDALOpenInternal(GDALOpenInfo&, char const* const*) (gdaldataset.cpp:2236)
==15194==    by 0x577DE04: GDALOpenInternal(char const*, GDALAccess, char const* const*) (gdaldataset.cpp:2208)
==15194==    by 0x577DDBE: GDALOpen (gdaldataset.cpp:2199)
==15194==    by 0x40328C: main (gdalinfo.c:173)
==15194== 
==15194== Invalid read of size 1
==15194==    at 0x580EF33: VSICheckMarker(char*) (cpl_vsisimple.cpp:476)
==15194==    by 0x580F196: VSIFree (cpl_vsisimple.cpp:552)
==15194==    by 0x5648BBC: GetConnection(char const*, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1525)
==15194==    by 0x5648CF3: PostGISRasterDataset::Open(GDALOpenInfo*) (postgisrasterdataset.cpp:1579)
==15194==    by 0x577DF51: GDALOpenInternal(GDALOpenInfo&, char const* const*) (gdaldataset.cpp:2236)
==15194==    by 0x577DE04: GDALOpenInternal(char const*, GDALAccess, char const* const*) (gdaldataset.cpp:2208)
==15194==    by 0x577DDBE: GDALOpen (gdaldataset.cpp:2199)
==15194==    by 0x40328C: main (gdalinfo.c:173)
==15194==  Address 0x16608381 is 1 bytes inside a block of size 26 free'd
==15194==    at 0x4C2779F: free (vg_replace_malloc.c:427)
==15194==    by 0x580F246: VSIFree (cpl_vsisimple.cpp:572)
==15194==    by 0x56487B3: GetConnectionInfo(char const*, char**, char**, char**, char**, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1423)
==15194==    by 0x5648AB9: GetConnection(char const*, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1498)
==15194==    by 0x5648CF3: PostGISRasterDataset::Open(GDALOpenInfo*) (postgisrasterdataset.cpp:1579)
==15194==    by 0x577DF51: GDALOpenInternal(GDALOpenInfo&, char const* const*) (gdaldataset.cpp:2236)
==15194==    by 0x577DE04: GDALOpenInternal(char const*, GDALAccess, char const* const*) (gdaldataset.cpp:2208)
==15194==    by 0x577DDBE: GDALOpen (gdaldataset.cpp:2199)
==15194==    by 0x40328C: main (gdalinfo.c:173)
==15194== 
==15194== Invalid read of size 1
==15194==    at 0x580EF3D: VSICheckMarker(char*) (cpl_vsisimple.cpp:476)
==15194==    by 0x580F196: VSIFree (cpl_vsisimple.cpp:552)
==15194==    by 0x5648BBC: GetConnection(char const*, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1525)
==15194==    by 0x5648CF3: PostGISRasterDataset::Open(GDALOpenInfo*) (postgisrasterdataset.cpp:1579)
==15194==    by 0x577DF51: GDALOpenInternal(GDALOpenInfo&, char const* const*) (gdaldataset.cpp:2236)
==15194==    by 0x577DE04: GDALOpenInternal(char const*, GDALAccess, char const* const*) (gdaldataset.cpp:2208)
==15194==    by 0x577DDBE: GDALOpen (gdaldataset.cpp:2199)
==15194==    by 0x40328C: main (gdalinfo.c:173)
==15194==  Address 0x16608380 is 0 bytes inside a block of size 26 free'd
==15194==    at 0x4C2779F: free (vg_replace_malloc.c:427)
==15194==    by 0x580F246: VSIFree (cpl_vsisimple.cpp:572)
==15194==    by 0x56487B3: GetConnectionInfo(char const*, char**, char**, char**, char**, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1423)
==15194==    by 0x5648AB9: GetConnection(char const*, char**, char**, char**, char**, char**, int*, int*) (postgisrasterdataset.cpp:1498)
==15194==    by 0x5648CF3: PostGISRasterDataset::Open(GDALOpenInfo*) (postgisrasterdataset.cpp:1579)
==15194==    by 0x577DF51: GDALOpenInternal(GDALOpenInfo&, char const* const*) (gdaldataset.cpp:2236)
==15194==    by 0x577DE04: GDALOpenInternal(char const*, GDALAccess, char const* const*) (gdaldataset.cpp:2208)
==15194==    by 0x577DDBE: GDALOpen (gdaldataset.cpp:2199)
==15194==    by 0x40328C: main (gdalinfo.c:173)
==15194== 
ERROR 1: Inconsistant use of VSI memory allocation primitives for 0x16608380 : MISV
==15194== 
==15194== HEAP SUMMARY:
==15194==     in use at exit: 249,153 bytes in 2,578 blocks
==15194==   total heap usage: 2,823 allocs, 245 frees, 353,484 bytes allocated
==15194== 
==15194== LEAK SUMMARY:
==15194==    definitely lost: 1,142 bytes in 67 blocks
==15194==    indirectly lost: 0 bytes in 0 blocks
==15194==      possibly lost: 153,991 bytes in 1,967 blocks
==15194==    still reachable: 94,020 bytes in 544 blocks
==15194==         suppressed: 0 bytes in 0 blocks
==15194== Rerun with --leak-check=full to see details of leaked memory
==15194== 
==15194== For counts of detected and suppressed errors, rerun with: -v
==15194== ERROR SUMMARY: 5 errors from 5 contexts (suppressed: 16 from 6)
Abandon

It would be good to add in the wkraster.py file a test with an invalid connection string by the way.

comment:3 by jorgearevalo, 12 years ago

Status: reopenednew

comment:4 by jorgearevalo, 12 years ago

Cc: jorgearevalo added

comment:5 by jorgearevalo, 12 years ago

Status: newassigned

comment:6 by dzwarg, 12 years ago

Owner: changed from jorgearevalo to dzwarg
Status: assignednew

comment:7 by dzwarg, 12 years ago

rouault: I cannot reproduce this bug. I tried opening a connection to a garbage database:

dzwarg@raster$ gdalinfo "PG:host='localhost' dbname='junk' user='postgres' port='5432'"
ERROR 1: PGconnectcb failed: FATAL:  database "junk" does not exist


ERROR 1: Couldn't establish a database connection
gdalinfo failed - unable to open 'PG:host='localhost' dbname='junk' user='postgres' port='5432''.
dzwarg@raster$

Then I tried opening a connection to a non-spatial enabled database:

dzwarg@raster$ gdalinfo "PG:host='localhost' dbname='postgres' user='postgres' port='5432'"
ERROR 1: Error checking geometry type existence. Is PostGIS correctly installed?:
gdalinfo failed - unable to open 'PG:host='localhost' dbname='postgres' user='postgres' port='5432''.
dzwarg@raster$

Then I tried opening a connection to a non-spatial enabled database, and left out the PGPORT:

dzwarg@raster$ gdalinfo "PG:host='localhost' dbname='postgres' user='postgres'"
ERROR 1: Port parameter must be provided, or PGPORT environment variable must be set. Please set the port and try again.
gdalinfo failed - unable to open 'PG:host='localhost' dbname='postgres' user='postgres''.
dzwarg@raster$

And I can't get it to segfault. Can you provide any more information about the crash?

comment:8 by Even Rouault, 12 years ago

I hoped it would have been pretty obvious, given the above pasted Valgrind trace ;-)

It is possible than you don't run into a segfault systematically if you are lucky, but I'd encourage you to edit your copy of port/cpl_vsisimple.cpp and uncomment the #define DEBUG_VSIMALLOC line (please don't commit the modified file however, this is just for debugging purposes). Or you edit your GDALmake.opt and add -DDDEBUG_VSIMALLOC to CFLAGS and CXXFLAGS. It includes a detection for double frees when using the VSI/CPL allocation routines. But even without that, Valgrind would catch it too.

Well, here's the fix (please test it works with working connection strings) :

r24096 /trunk/gdal/frmts/postgisraster/postgisrasterdataset.cpp: Postgisraster: fix double free (and make the caller responsible for cleaning up return arguments)

By the way, I don't understand why the connection string is parsed such that a host, user and password must be provided. Only the dbname should be compulsory, like for the OGR PG driver. I can do "ogrinfo pg:dbname=autotest" and this connects through a unix socket with my unix user".

comment:9 by pracine, 12 years ago

Keywords: postgis_raster added; postgisraster removed

comment:10 by dzwarg, 12 years ago

Resolution: fixed
Status: newclosed

This fix works with working connection strings. Thank you.

Note: See TracTickets for help on using tickets.