Ticket #4533 (closed task: fixed)

Opened 16 months ago

Last modified 15 months ago

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

conninfo.patch Download (41.4 KB) - added by dzwarg 16 months 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 Download (42.0 KB) - added by dzwarg 16 months ago.
This patch has been run through valgrind -- no memleaks detected. Rock'n'roll with connection caching.

Change History

Changed 16 months ago by dzwarg

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.

Changed 16 months ago by dzwarg

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

Changed 16 months ago by jorgearevalo

  • status changed from new to closed
  • resolution set to fixed

Patch applied in r24088. I close the ticket.

Changed 16 months ago by rouault

  • status changed from closed to reopened
  • resolution fixed deleted

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.

Changed 16 months ago by jorgearevalo

  • status changed from reopened to new

Changed 16 months ago by jorgearevalo

  • cc jorgearevalo added

Changed 16 months ago by jorgearevalo

  • status changed from new to assigned

Changed 16 months ago by dzwarg

  • owner changed from jorgearevalo to dzwarg
  • status changed from assigned to new

Changed 16 months ago by dzwarg

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?

Changed 16 months ago by rouault

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".

Changed 16 months ago by pracine

  • keywords postgis_raster added; postgisraster removed

Changed 15 months ago by dzwarg

  • status changed from new to closed
  • resolution set to fixed

This fix works with working connection strings. Thank you.

Note: See TracTickets for help on using tickets.