[postgis-devel] Re: geometry stats

[Mark Cave-Ayland]

Hi strk,

> -----Original Message-----
> From: strk [mailto:strk at keybit.net] 
> Sent: 25 February 2004 13:30
> To: Mark Cave-Ayland
> Cc: 'PostGIS Development Discussion'
> Subject: Re: [postgis-devel] Re: geometry stats
> 
> 
> m.cave-ayland wrote:
> > Hi strk,
> > 
> > Yeah, box_overlap() is a routine from the internal PostgreSQL 
> > geometric functions (see src/backend/utils/adt/geo_ops.c). 
> >
> > The current way it works is that a BOX3D is converted into a BOX and
then 
> > box_overlap() is called. Yuck. The fix in the thread I mentioned 
> > before was that I wrote my own equivalent of the contains() operator

> > and added it into PostGIS instead of calling the PostgreSQL 
> > contains/contained functions which just appeared to be plain broken.
>
> http://homepages.zen.co.uk/zen9662/postgis/ Not Found.
> Anyway, I got your point and added a pgbox_overlap() function inside
postgis_gist_72.c
> that makes the same computation as geometry_overlap(). Its seem to be
working now. I 
> think you were right calling for a completely 'internal' handling of
the GiST, for now I 
> just made a quick fix, but I'm sure we should review the code and make
some cleanups 
> there...
 
Glad that you managed to understand what I did even though the URL is
broken (looks like my ISP changed it to
http://www.zen9662.zen.co.uk/postgis). I totally agree that we should
use our own RTree functions in case the PostgreSQL ones are broken. From
the discussions I remember that Dave wanted to keep the implementation
the same as rtree_gist to help debugging; however it's looking like in
this case we need to make an exception. If you found the overlap
function was broken i.e. matching more geometries than it should have
been, then this may give a speed-up to spatial && queries :)

> Nope, the segfault is there including or excluding elog() calls by
setting 
> DEBUG_GEOMETRY_STATS to 1 (no segfault) or 2 (segfault). It happens at
the end of the >
> analyze; command, right before the end of the function, so it's like
postgresql is faced 
> with wild pointers.
>
> If you want to take a look change this:
> @@ -1432,5 +1432,5 @@
> -#if 0 & DEBUG_GEOMETRY_STATS > 1
> +#if DEBUG_GEOMETRY_STATS > 1
> and set DEBUG_GEOMETRY_STATS to 2
>
> Allocation here happens for the GEOM_STATS struct itself in the
> stats->anl_context context. Other allocations are made in
> the 'current' context but are not meant to leave after
> the end of the function.

OK you twisted my arm.... ;)  I installed the latest CVS PostgreSQL and
PostGIS and found out what the problem was - it's in the normalise code
below:

>	/*
>	 * Normalize histogram
>	 *
>	 * We divide each histogram cell value
>	 * by the number of samples examined.
>	 *
>	 */
>	for (i=0;i<boxesPerSide*boxesPerSide; i++)
>		geomstats->value[i] /= samplerows;

Because of the i++ (postincrement), i is now pointing to
boxesPerSide*boxesPerSide (which is now outside the allocated memory
space)

>
> #if DEBUG_GEOMETRY_STATS > 1
>	{
>		int x, y;
>		for (x=0; x<bps; x++)
>		{
>			for (y=0; y<bps; y++)
>			{
>				geomstats->value[i] = 0;
				
				^^^^^^^^^^^^^^^^^^^^^^^^
This causes your segfault because it is writing outside allocated
palloc'd memory. Is this supposed to set the last value of the histogram
to be 0? (remember this is outside the normalisation loop). The cure is
either to remove this line, or change it to:

				geomstats->value[i - 1] = 0;
				
>
>
>				elog(NOTICE, " histo[%d,%d] = %.15f", x,
y, geomstats->value[x+y*bps]);
>			}
>		}
>	}
> #endif


Hope this helps! Keep up the good work!

Mark.

---

Mark Cave-Ayland
Webbased Ltd.
Tamar Science Park
Derriford
Plymouth
PL6 8BX
England

Tel: +44 (0)1752 764445
Fax: +44 (0)1752 764446


This email and any attachments are confidential to the intended
recipient and may also be privileged. If you are not the intended
recipient please delete it from your system and notify the sender. You
should not copy it or use it for any purpose nor disclose or distribute
its contents to any other person.