Hi list,
I was messing around a bit with BRIN code and found some cleanups to be made:
1. Remove dead code in brin_form_tuple
2. Add missing Datum conversion macros
3. Use AttrNumber type in places using 1-based attribute numbers
Though it's not hard to find cases all over source code similar to (2)
and (3), are these kinds of cleanups considered useful?
Regards,
Marti Raudsepp
From 002067288d7d8db0c2107e6266f95c53a1614b1d Mon Sep 17 00:00:00 2001
From: Marti Raudsepp <ma...@juffo.org>
Date: Wed, 25 Oct 2017 22:30:14 +0300
Subject: [PATCH] brin: Minor cleanups of BRIN code
* Remove dead code in brin_form_tuple
* Add missing Datum conversion macros
* Use AttrNumber type in places using 1-based attribute numbers
---
src/backend/access/brin/README | 2 +-
src/backend/access/brin/brin.c | 4 ++--
src/backend/access/brin/brin_inclusion.c | 11 ++++++-----
src/backend/access/brin/brin_minmax.c | 16 +++++++++-------
src/backend/access/brin/brin_tuple.c | 1 -
5 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/brin/README b/src/backend/access/brin/README
index 636d96545b..3e80bad62f 100644
--- a/src/backend/access/brin/README
+++ b/src/backend/access/brin/README
@@ -21,7 +21,7 @@ possible to support the amgettuple interface. Instead, we only provide
amgetbitmap support. The amgetbitmap routine returns a lossy TIDBitmap
comprising all pages in those page ranges that match the query
qualifications. The recheck step in the BitmapHeapScan node prunes tuples
-that are not visible according to the query qualifications.
+that are not visible according to the query predicates.
An operator class must have the following entries:
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index e95fbbcea7..0e8f4e0d08 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -252,7 +252,7 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
PointerGetDatum(bdesc),
PointerGetDatum(bval),
values[keyno],
- nulls[keyno]);
+ BoolGetDatum(nulls[keyno]));
/* if that returned true, we need to insert the updated tuple */
need_insert |= DatumGetBool(result);
}
@@ -647,7 +647,7 @@ brinbuildCallback(Relation index,
attr->attcollation,
PointerGetDatum(state->bs_bdesc),
PointerGetDatum(col),
- values[i], isnull[i]);
+ values[i], BoolGetDatum(isnull[i]));
}
}
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 6ce355c6a9..4918138014 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -81,10 +81,11 @@ typedef struct InclusionOpaque
FmgrInfo strategy_procinfos[RTMaxStrategyNumber];
} InclusionOpaque;
-static FmgrInfo *inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno,
+static FmgrInfo *inclusion_get_procinfo(BrinDesc *bdesc, AttrNumber attno,
uint16 procnum);
-static FmgrInfo *inclusion_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno,
- Oid subtype, uint16 strategynum);
+static FmgrInfo * inclusion_get_strategy_procinfo(BrinDesc *bdesc,
+ AttrNumber attno, Oid subtype,
+ uint16 strategynum);
/*
@@ -588,7 +589,7 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
* or null if it is not exists.
*/
static FmgrInfo *
-inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
+inclusion_get_procinfo(BrinDesc *bdesc, AttrNumber attno, uint16 procnum)
{
InclusionOpaque *opaque;
uint16 basenum = procnum - PROCNUM_BASE;
@@ -646,7 +647,7 @@ inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum)
* made here, see that function too.
*/
static FmgrInfo *
-inclusion_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
+inclusion_get_strategy_procinfo(BrinDesc *bdesc, AttrNumber attno, Oid subtype,
uint16 strategynum)
{
InclusionOpaque *opaque;
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 0f6aa33a45..5f28cf362c 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -29,7 +29,7 @@ typedef struct MinmaxOpaque
FmgrInfo strategy_procinfos[BTMaxStrategyNumber];
} MinmaxOpaque;
-static FmgrInfo *minmax_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno,
+static FmgrInfo *minmax_get_strategy_procinfo(BrinDesc *bdesc, AttrNumber attno,
Oid subtype, uint16 strategynum);
@@ -68,7 +68,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
Datum newval = PG_GETARG_DATUM(2);
- bool isnull = PG_GETARG_DATUM(3);
+ bool isnull = PG_GETARG_BOOL(3);
Oid colloid = PG_GET_COLLATION();
FmgrInfo *cmpFn;
Datum compar;
@@ -281,8 +281,9 @@ brin_minmax_union(PG_FUNCTION_ARGS)
/* Adjust minimum, if B's min is less than A's min */
finfo = minmax_get_strategy_procinfo(bdesc, attno, attr->atttypid,
BTLessStrategyNumber);
- needsadj = FunctionCall2Coll(finfo, colloid, col_b->bv_values[0],
- col_a->bv_values[0]);
+ needsadj = DatumGetBool(FunctionCall2Coll(finfo, colloid,
+ col_b->bv_values[0],
+ col_a->bv_values[0]));
if (needsadj)
{
if (!attr->attbyval)
@@ -294,8 +295,9 @@ brin_minmax_union(PG_FUNCTION_ARGS)
/* Adjust maximum, if B's max is greater than A's max */
finfo = minmax_get_strategy_procinfo(bdesc, attno, attr->atttypid,
BTGreaterStrategyNumber);
- needsadj = FunctionCall2Coll(finfo, colloid, col_b->bv_values[1],
- col_a->bv_values[1]);
+ needsadj = DatumGetBool(FunctionCall2Coll(finfo, colloid,
+ col_b->bv_values[1],
+ col_a->bv_values[1]));
if (needsadj)
{
if (!attr->attbyval)
@@ -314,7 +316,7 @@ brin_minmax_union(PG_FUNCTION_ARGS)
* there. If changes are made here, see that function too.
*/
static FmgrInfo *
-minmax_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
+minmax_get_strategy_procinfo(BrinDesc *bdesc, AttrNumber attno, Oid subtype,
uint16 strategynum)
{
MinmaxOpaque *opaque;
diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c
index c82bbbaa7f..7801298613 100644
--- a/src/backend/access/brin/brin_tuple.c
+++ b/src/backend/access/brin/brin_tuple.c
@@ -244,7 +244,6 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple,
*bitP |= bitmask;
}
- bitP = ((bits8 *) (rettuple + SizeOfBrinTuple)) - 1;
}
if (tuple->bt_placeholder)
--
2.19.2
