On Wed, Jan 28, 2015 at 3:15 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> So I'm fine with taking out both this documentation text and the dead
> null-pointer checks; but let's do that all in one patch not piecemeal.
> In any case, this is just cosmetic cleanup; there's no actual hazard
> here.
Attached is a patch with all those things done. I added as well an
assertion in gistKeyIsEQ checking if the input datums are NULL. I
believe that this is still useful for developers, feel free to rip it
out from the patch if you think otherwise.
Regards,
--
Michael
From 95b051aac56de68412a7b0635484e46eb4e24ad0 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Wed, 28 Jan 2015 09:36:11 +0900
Subject: [PATCH] Remove dead NULL-pointer checks in GiST code
The key value passed to the same method is generated by either the
compress, union or picksplit methods, but it happens that none of them
actually pass a NULL pointer. Some compress methods do not check for a
NULL pointer, what should have resulted in a crash. Note that
gist_poly_compress() and gist_circle_compress() do check for a NULL,
but they only return a NULL key if the input key is NULL, which cannot
happen because the input comes from a table.
This commit also removes a documentation note added in commit a0a3883
that has been introduced based on the fact that some routines of the
same method were doing NULL-pointer checks, and adds an assertion
in gistKeyIsEQ to ensure that no NULL keys are passed when calling the
same method.
---
contrib/btree_gist/btree_utils_num.c | 8 ++----
contrib/btree_gist/btree_utils_var.c | 10 ++-----
doc/src/sgml/gist.sgml | 5 ----
src/backend/access/gist/gistproc.c | 56 ++++++++++++------------------------
src/backend/access/gist/gistutil.c | 3 ++
5 files changed, 25 insertions(+), 57 deletions(-)
diff --git a/contrib/btree_gist/btree_utils_num.c b/contrib/btree_gist/btree_utils_num.c
index 505633c..7c5b911 100644
--- a/contrib/btree_gist/btree_utils_num.c
+++ b/contrib/btree_gist/btree_utils_num.c
@@ -147,13 +147,9 @@ gbt_num_same(const GBT_NUMKEY *a, const GBT_NUMKEY *b, const gbtree_ninfo *tinfo
b2.lower = &(((GBT_NUMKEY *) b)[0]);
b2.upper = &(((GBT_NUMKEY *) b)[tinfo->size]);
- if (
- (*tinfo->f_eq) (b1.lower, b2.lower) &&
- (*tinfo->f_eq) (b1.upper, b2.upper)
- )
- return TRUE;
- return FALSE;
+ return ((*tinfo->f_eq) (b1.lower, b2.lower) &&
+ (*tinfo->f_eq) (b1.upper, b2.upper));
}
diff --git a/contrib/btree_gist/btree_utils_var.c b/contrib/btree_gist/btree_utils_var.c
index b7dd060..6ad3347 100644
--- a/contrib/btree_gist/btree_utils_var.c
+++ b/contrib/btree_gist/btree_utils_var.c
@@ -337,7 +337,6 @@ bool
gbt_var_same(Datum d1, Datum d2, Oid collation,
const gbtree_vinfo *tinfo)
{
- bool result;
GBT_VARKEY *t1 = (GBT_VARKEY *) DatumGetPointer(d1);
GBT_VARKEY *t2 = (GBT_VARKEY *) DatumGetPointer(d2);
GBT_VARKEY_R r1,
@@ -346,13 +345,8 @@ gbt_var_same(Datum d1, Datum d2, Oid collation,
r1 = gbt_var_key_readable(t1);
r2 = gbt_var_key_readable(t2);
- if (t1 && t2)
- result = ((*tinfo->f_cmp) (r1.lower, r2.lower, collation) == 0 &&
- (*tinfo->f_cmp) (r1.upper, r2.upper, collation) == 0);
- else
- result = (t1 == NULL && t2 == NULL);
-
- return result;
+ return ((*tinfo->f_cmp) (r1.lower, r2.lower, collation) == 0 &&
+ (*tinfo->f_cmp) (r1.upper, r2.upper, collation) == 0);
}
diff --git a/doc/src/sgml/gist.sgml b/doc/src/sgml/gist.sgml
index 5de282b..f9644b0 100644
--- a/doc/src/sgml/gist.sgml
+++ b/doc/src/sgml/gist.sgml
@@ -498,11 +498,6 @@ my_compress(PG_FUNCTION_ARGS)
course.
</para>
- <para>
- Depending on your needs, you could also need to care about
- compressing <literal>NULL</> values in there, storing for example
- <literal>(Datum) 0</> like <literal>gist_circle_compress</> does.
- </para>
</listitem>
</varlistentry>
diff --git a/src/backend/access/gist/gistproc.c b/src/backend/access/gist/gistproc.c
index 4decaa6..1df6357 100644
--- a/src/backend/access/gist/gistproc.c
+++ b/src/backend/access/gist/gistproc.c
@@ -1039,25 +1039,15 @@ gist_poly_compress(PG_FUNCTION_ARGS)
if (entry->leafkey)
{
- retval = palloc(sizeof(GISTENTRY));
- if (DatumGetPointer(entry->key) != NULL)
- {
- POLYGON *in = DatumGetPolygonP(entry->key);
- BOX *r;
+ POLYGON *in = DatumGetPolygonP(entry->key);
+ BOX *r;
- r = (BOX *) palloc(sizeof(BOX));
- memcpy((void *) r, (void *) &(in->boundbox), sizeof(BOX));
- gistentryinit(*retval, PointerGetDatum(r),
- entry->rel, entry->page,
- entry->offset, FALSE);
-
- }
- else
- {
- gistentryinit(*retval, (Datum) 0,
- entry->rel, entry->page,
- entry->offset, FALSE);
- }
+ retval = palloc(sizeof(GISTENTRY));
+ r = (BOX *) palloc(sizeof(BOX));
+ memcpy((void *) r, (void *) &(in->boundbox), sizeof(BOX));
+ gistentryinit(*retval, PointerGetDatum(r),
+ entry->rel, entry->page,
+ entry->offset, FALSE);
}
else
retval = entry;
@@ -1113,28 +1103,18 @@ gist_circle_compress(PG_FUNCTION_ARGS)
if (entry->leafkey)
{
+ CIRCLE *in = DatumGetCircleP(entry->key);
+ BOX *r;
retval = palloc(sizeof(GISTENTRY));
- if (DatumGetCircleP(entry->key) != NULL)
- {
- CIRCLE *in = DatumGetCircleP(entry->key);
- BOX *r;
-
- r = (BOX *) palloc(sizeof(BOX));
- r->high.x = in->center.x + in->radius;
- r->low.x = in->center.x - in->radius;
- r->high.y = in->center.y + in->radius;
- r->low.y = in->center.y - in->radius;
- gistentryinit(*retval, PointerGetDatum(r),
- entry->rel, entry->page,
- entry->offset, FALSE);
- }
- else
- {
- gistentryinit(*retval, (Datum) 0,
- entry->rel, entry->page,
- entry->offset, FALSE);
- }
+ r = (BOX *) palloc(sizeof(BOX));
+ r->high.x = in->center.x + in->radius;
+ r->low.x = in->center.x - in->radius;
+ r->high.y = in->center.y + in->radius;
+ r->low.y = in->center.y - in->radius;
+ gistentryinit(*retval, PointerGetDatum(r),
+ entry->rel, entry->page,
+ entry->offset, FALSE);
}
else
retval = entry;
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 38af0e0..a44cce8 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -276,6 +276,9 @@ gistKeyIsEQ(GISTSTATE *giststate, int attno, Datum a, Datum b)
{
bool result;
+ /* both keys cannot be NULL */
+ Assert(DatumGetPointer(a) != NULL && DatumGetPointer(b) != NULL);
+
FunctionCall3Coll(&giststate->equalFn[attno],
giststate->supportCollation[attno],
a, b,
--
2.2.2
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
