[PATCH] Minor cleanups of BRIN code
Started by Marti Raudseppabout 7 years ago1 messages
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
Attachments:
0001-brin-Minor-cleanups-of-BRIN-code.patchtext/x-patch; charset=US-ASCII; name=0001-brin-Minor-cleanups-of-BRIN-code.patchDownload
From 002067288d7d8db0c2107e6266f95c53a1614b1d Mon Sep 17 00:00:00 2001
From: Marti Raudsepp <marti@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