REVIEW: Optimizing box_penalty
I tried to review the "Optimizing box_penalty" patch:
https://commitfest.postgresql.org/action/patch_view?id=600
as posted here:
http://archives.postgresql.org/message-id/4E088690.5080706@enterprisedb.com
The patch no longer applies to source code, due to other recent GiST
changes. Parts of it were modifying functions which no longer exist.
I picked out the bits which still seemed relevant, and a patch with
that is attached. The improvement in REINDEX time is now only about
0.25% on my machine at home, which is small enough that it could
easily be from code shifting around. (Or such shifts might be hiding
a larger actual savings.) In any event, this patch doesn't seem to
be justified as a performance patch, based on my benchmarks today.
On the other hand, this patch leaves the code a few lines shorter and
eliminates some unnecessary Datum wrapping, PG_FUNCTION_ARGS
parameters on a static function, and allows that function to be
called directly rather than using DirectFunctionCall2(). I find the
resulting code a little cleaner and easier to read. I would prefer
to see it applied on that basis, personally.
Since the author is a committer, and this is a pretty minor code
style patch at this point, I'll mark it "Ready for Committer" and
leave it to Heikki to decide what to do with it.
-Kevin
Attachments:
penalty-v2.patchapplication/octet-stream; name=penalty-v2.patchDownload
*** a/src/backend/access/gist/gistproc.c
--- b/src/backend/access/gist/gistproc.c
***************
*** 23,29 ****
static bool gist_box_leaf_consistent(BOX *key, BOX *query,
StrategyNumber strategy);
! static double size_box(Datum dbox);
static bool rtree_internal_consistent(BOX *key, BOX *query,
StrategyNumber strategy);
--- 23,29 ----
static bool gist_box_leaf_consistent(BOX *key, BOX *query,
StrategyNumber strategy);
! static double size_box(BOX *box);
static bool rtree_internal_consistent(BOX *key, BOX *query,
StrategyNumber strategy);
***************
*** 35,55 **** static bool rtree_internal_consistent(BOX *key, BOX *query,
* Box ops
**************************************************/
! static Datum
! rt_box_union(PG_FUNCTION_ARGS)
{
- BOX *a = PG_GETARG_BOX_P(0);
- BOX *b = PG_GETARG_BOX_P(1);
- BOX *n;
-
- n = (BOX *) palloc(sizeof(BOX));
-
n->high.x = Max(a->high.x, b->high.x);
n->high.y = Max(a->high.y, b->high.y);
n->low.x = Min(a->low.x, b->low.x);
n->low.y = Min(a->low.y, b->low.y);
-
- PG_RETURN_BOX_P(n);
}
/*
--- 35,50 ----
* Box ops
**************************************************/
! /*
! * Calculates union of two boxes, a and b. The result is stored in *n.
! */
! static void
! rt_box_union(BOX *n, BOX *a, BOX *b)
{
n->high.x = Max(a->high.x, b->high.x);
n->high.y = Max(a->high.y, b->high.y);
n->low.x = Min(a->low.x, b->low.x);
n->low.y = Min(a->low.y, b->low.y);
}
/*
***************
*** 166,175 **** gist_box_penalty(PG_FUNCTION_ARGS)
GISTENTRY *origentry = (GISTENTRY *) PG_GETARG_POINTER(0);
GISTENTRY *newentry = (GISTENTRY *) PG_GETARG_POINTER(1);
float *result = (float *) PG_GETARG_POINTER(2);
! Datum ud;
! ud = DirectFunctionCall2(rt_box_union, origentry->key, newentry->key);
! *result = (float) (size_box(ud) - size_box(origentry->key));
PG_RETURN_POINTER(result);
}
--- 161,172 ----
GISTENTRY *origentry = (GISTENTRY *) PG_GETARG_POINTER(0);
GISTENTRY *newentry = (GISTENTRY *) PG_GETARG_POINTER(1);
float *result = (float *) PG_GETARG_POINTER(2);
! BOX *origbox = DatumGetBoxP(origentry->key);
! BOX *newbox = DatumGetBoxP(newentry->key);
! BOX unionbox;
! rt_box_union(&unionbox, origbox, newbox);
! *result = (float) (size_box(&unionbox) - size_box(origbox));
PG_RETURN_POINTER(result);
}
***************
*** 937,947 **** gist_box_leaf_consistent(BOX *key, BOX *query, StrategyNumber strategy)
}
static double
! size_box(Datum dbox)
{
! BOX *box = DatumGetBoxP(dbox);
!
! if (box == NULL || box->high.x <= box->low.x || box->high.y <= box->low.y)
return 0.0;
return (box->high.x - box->low.x) * (box->high.y - box->low.y);
}
--- 934,942 ----
}
static double
! size_box(BOX *box)
{
! if (box->high.x <= box->low.x || box->high.y <= box->low.y)
return 0.0;
return (box->high.x - box->low.x) * (box->high.y - box->low.y);
}
On 08.10.2011 21:51, Kevin Grittner wrote:
I tried to review the "Optimizing box_penalty" patch:
Thanks!
On the other hand, this patch leaves the code a few lines shorter and
eliminates some unnecessary Datum wrapping, PG_FUNCTION_ARGS
parameters on a static function, and allows that function to be
called directly rather than using DirectFunctionCall2(). I find the
resulting code a little cleaner and easier to read. I would prefer
to see it applied on that basis, personally.
Agreed, committed.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com