public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/6] Move IS [NOT] NULL handling from BRIN support functions
6+ messages / 5 participants
[nested] [flat]
* [PATCH 2/6] Move IS [NOT] NULL handling from BRIN support functions
@ 2021-03-02 18:27 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Tomas Vondra @ 2021-03-02 18:27 UTC (permalink / raw)
The handling of IS [NOT] NULL clauses is independent of an opclass, and
most of the code was exactly the same in both minmax and inclusion. So
instead move the code from support procedures to the AM methods etc.
This simplifies the code quite a bit - especially the support procedures
quite a bit, as they don't need to care about NULL values and flags at
all. It also means the IS [NOT] NULL clauses can be evaluated without
invoking the support procedure at all.
Author: Tomas Vondra <[email protected]>
Author: Nikita Glukhov <[email protected]>
Reviewed-by: Alvaro Herrera <[email protected]>
Reviewed-by: Mark Dilger <[email protected]>
Reviewed-by: Alexander Korotkov <[email protected]>
Reviewed-by: Masahiko Sawada <[email protected]>
Reviewed-by: John Naylor <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/brin/brin.c | 293 +++++++++++++++++------
src/backend/access/brin/brin_inclusion.c | 96 +-------
src/backend/access/brin/brin_minmax.c | 93 +------
src/include/access/brin_internal.h | 3 +
4 files changed, 244 insertions(+), 241 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 963b7079cf..9f2656b8d9 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -35,6 +35,7 @@
#include "storage/freespace.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+#include "utils/datum.h"
#include "utils/index_selfuncs.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -77,7 +78,9 @@ static void form_and_insert_tuple(BrinBuildState *state);
static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a,
BrinTuple *b);
static void brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy);
-
+static bool add_values_to_range(Relation idxRel, BrinDesc *bdesc,
+ BrinMemTuple *dtup, Datum *values, bool *nulls);
+static bool check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys);
/*
* BRIN handler function: return IndexAmRoutine with access method parameters
@@ -179,7 +182,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
OffsetNumber off;
BrinTuple *brtup;
BrinMemTuple *dtup;
- int keyno;
CHECK_FOR_INTERRUPTS();
@@ -243,31 +245,7 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
dtup = brin_deform_tuple(bdesc, brtup, NULL);
- /*
- * Compare the key values of the new tuple to the stored index values;
- * our deformed tuple will get updated if the new tuple doesn't fit
- * the original range (note this means we can't break out of the loop
- * early). Make a note of whether this happens, so that we know to
- * insert the modified tuple later.
- */
- for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
- {
- Datum result;
- BrinValues *bval;
- FmgrInfo *addValue;
-
- bval = &dtup->bt_columns[keyno];
- addValue = index_getprocinfo(idxRel, keyno + 1,
- BRIN_PROCNUM_ADDVALUE);
- result = FunctionCall4Coll(addValue,
- idxRel->rd_indcollation[keyno],
- PointerGetDatum(bdesc),
- PointerGetDatum(bval),
- values[keyno],
- nulls[keyno]);
- /* if that returned true, we need to insert the updated tuple */
- need_insert |= DatumGetBool(result);
- }
+ need_insert = add_values_to_range(idxRel, bdesc, dtup, values, nulls);
if (!need_insert)
{
@@ -390,8 +368,10 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
BrinMemTuple *dtup;
BrinTuple *btup = NULL;
Size btupsz = 0;
- ScanKey **keys;
- int *nkeys;
+ ScanKey **keys,
+ **nullkeys;
+ int *nkeys,
+ *nnullkeys;
int keyno;
opaque = (BrinOpaque *) scan->opaque;
@@ -419,13 +399,18 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
* consistent support procedure. We allocate space for all attributes, so
* that we don't have to bother determining which attributes are used.
*
+ * We keep null and regular keys separate, so that we can pass just the
+ * regular keys to the consistent function easily.
+ *
* XXX The widest table can have ~1600 attributes, so this may allocate a
* couple kilobytes of memory). We could invent a more compact approach
* (with just space for used attributes) but that would make the matching
* more complicated, so it may not be a win.
*/
keys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
+ nullkeys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
nkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts);
+ nnullkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts);
/*
* Preprocess the scan keys - split them into per-attribute arrays.
@@ -445,23 +430,23 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
TupleDescAttr(bdesc->bd_tupdesc,
keyattno - 1)->attcollation));
- /* First time we see this index attribute, so init as needed. */
- if (!keys[keyattno - 1])
+ /*
+ * First time we see this index attribute, so init as needed.
+ *
+ * This is a bit of an overkill - we don't know how many scan keys are
+ * there for a given attribute, so we simply allocate the largest
+ * number possible (as if all scan keys belonged to the same
+ * attribute). This may waste a bit of memory, but we only expect
+ * small number of scan keys in general, so this should be negligible,
+ * and it's probably cheaper than having to repalloc repeatedly.
+ */
+ if (consistentFn[keyattno - 1].fn_oid == InvalidOid)
{
FmgrInfo *tmp;
- /*
- * This is a bit of an overkill - we don't know how many scan keys
- * are there for this attribute, so we simply allocate the largest
- * number possible. This may waste a bit of memory, but we only
- * expect small number of scan keys in general, so this should be
- * negligible, and it's cheaper than having to repalloc
- * repeatedly.
- */
- keys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
-
- /* First time this column, so look up consistent function */
- Assert(consistentFn[keyattno - 1].fn_oid == InvalidOid);
+ /* No key/null arrays for this attribute. */
+ Assert((keys[keyattno - 1] == NULL) && (nkeys[keyattno - 1] == 0));
+ Assert((nullkeys[keyattno - 1] == NULL) && (nnullkeys[keyattno - 1] == 0));
tmp = index_getprocinfo(idxRel, keyattno,
BRIN_PROCNUM_CONSISTENT);
@@ -469,9 +454,23 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
CurrentMemoryContext);
}
- /* Add key to the per-attribute array. */
- keys[keyattno - 1][nkeys[keyattno - 1]] = key;
- nkeys[keyattno - 1]++;
+ /* Add key to the proper per-attribute array. */
+ if (key->sk_flags & SK_ISNULL)
+ {
+ if (!nullkeys[keyattno - 1])
+ nullkeys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
+
+ nullkeys[keyattno - 1][nnullkeys[keyattno - 1]] = key;
+ nnullkeys[keyattno - 1]++;
+ }
+ else
+ {
+ if (!keys[keyattno - 1])
+ keys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
+
+ keys[keyattno - 1][nkeys[keyattno - 1]] = key;
+ nkeys[keyattno - 1]++;
+ }
}
/* allocate an initial in-memory tuple, out of the per-range memcxt */
@@ -549,15 +548,58 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
BrinValues *bval;
Datum add;
- /* skip attributes without any san keys */
- if (nkeys[attno - 1] == 0)
+ /*
+ * skip attributes without any scan keys (both regular and
+ * IS [NOT] NULL)
+ */
+ if (nkeys[attno - 1] == 0 && nnullkeys[attno - 1] == 0)
continue;
bval = &dtup->bt_columns[attno - 1];
+ /*
+ * First check if there are any IS [NOT] NULL scan keys,
+ * and if we're violating them. In that case we can
+ * terminate early, without invoking the support function.
+ *
+ * As there may be more keys, we can only detemine
+ * mismatch within this loop.
+ */
+ if (bdesc->bd_info[attno - 1]->oi_regular_nulls &&
+ !check_null_keys(bval, nullkeys[attno - 1],
+ nnullkeys[attno - 1]))
+ {
+ /*
+ * If any of the IS [NOT] NULL keys failed, the page
+ * range as a whole can't pass. So terminate the loop.
+ */
+ addrange = false;
+ break;
+ }
+
+ /*
+ * So either there are no IS [NOT] NULL keys, or all
+ * passed. If there are no regular scan keys, we're done -
+ * the page range matches. If there are regular keys, but
+ * the page range is marked as 'all nulls' it can't
+ * possibly pass (we're assuming the operators are
+ * strict).
+ */
+
+ /* No regular scan keys - page range as a whole passes. */
+ if (!nkeys[attno - 1])
+ continue;
+
Assert((nkeys[attno - 1] > 0) &&
(nkeys[attno - 1] <= scan->numberOfKeys));
+ /* If it is all nulls, it cannot possibly be consistent. */
+ if (bval->bv_allnulls)
+ {
+ addrange = false;
+ break;
+ }
+
/*
* Check whether the scan key is consistent with the page
* range values; if so, have the pages in the range added
@@ -703,7 +745,6 @@ brinbuildCallback(Relation index,
{
BrinBuildState *state = (BrinBuildState *) brstate;
BlockNumber thisblock;
- int i;
thisblock = ItemPointerGetBlockNumber(tid);
@@ -732,25 +773,8 @@ brinbuildCallback(Relation index,
}
/* Accumulate the current tuple into the running state */
- for (i = 0; i < state->bs_bdesc->bd_tupdesc->natts; i++)
- {
- FmgrInfo *addValue;
- BrinValues *col;
- Form_pg_attribute attr = TupleDescAttr(state->bs_bdesc->bd_tupdesc, i);
-
- col = &state->bs_dtuple->bt_columns[i];
- addValue = index_getprocinfo(index, i + 1,
- BRIN_PROCNUM_ADDVALUE);
-
- /*
- * Update dtuple state, if and as necessary.
- */
- FunctionCall4Coll(addValue,
- attr->attcollation,
- PointerGetDatum(state->bs_bdesc),
- PointerGetDatum(col),
- values[i], isnull[i]);
- }
+ (void) add_values_to_range(index, state->bs_bdesc, state->bs_dtuple,
+ values, isnull);
}
/*
@@ -1529,6 +1553,39 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b)
FmgrInfo *unionFn;
BrinValues *col_a = &a->bt_columns[keyno];
BrinValues *col_b = &db->bt_columns[keyno];
+ BrinOpcInfo *opcinfo = bdesc->bd_info[keyno];
+
+ if (opcinfo->oi_regular_nulls)
+ {
+ /* Adjust "hasnulls". */
+ if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
+ col_a->bv_hasnulls = true;
+
+ /* If there are no values in B, there's nothing left to do. */
+ if (col_b->bv_allnulls)
+ continue;
+
+ /*
+ * Adjust "allnulls". If A doesn't have values, just copy the
+ * values from B into A, and we're done. We cannot run the
+ * operators in this case, because values in A might contain
+ * garbage. Note we already established that B contains values.
+ */
+ if (col_a->bv_allnulls)
+ {
+ int i;
+
+ col_a->bv_allnulls = false;
+
+ for (i = 0; i < opcinfo->oi_nstored; i++)
+ col_a->bv_values[i] =
+ datumCopy(col_b->bv_values[i],
+ opcinfo->oi_typcache[i]->typbyval,
+ opcinfo->oi_typcache[i]->typlen);
+
+ continue;
+ }
+ }
unionFn = index_getprocinfo(bdesc->bd_index, keyno + 1,
BRIN_PROCNUM_UNION);
@@ -1582,3 +1639,103 @@ brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy)
*/
FreeSpaceMapVacuum(idxrel);
}
+
+static bool
+add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup,
+ Datum *values, bool *nulls)
+{
+ int keyno;
+ bool modified = false;
+
+ /*
+ * Compare the key values of the new tuple to the stored index values; our
+ * deformed tuple will get updated if the new tuple doesn't fit the
+ * original range (note this means we can't break out of the loop early).
+ * Make a note of whether this happens, so that we know to insert the
+ * modified tuple later.
+ */
+ for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
+ {
+ Datum result;
+ BrinValues *bval;
+ FmgrInfo *addValue;
+
+ bval = &dtup->bt_columns[keyno];
+
+ if (bdesc->bd_info[keyno]->oi_regular_nulls && nulls[keyno])
+ {
+ /*
+ * If the new value is null, we record that we saw it if it's the
+ * first one; otherwise, there's nothing to do.
+ */
+ if (!bval->bv_hasnulls)
+ {
+ bval->bv_hasnulls = true;
+ modified = true;
+ }
+
+ continue;
+ }
+
+ addValue = index_getprocinfo(idxRel, keyno + 1,
+ BRIN_PROCNUM_ADDVALUE);
+ result = FunctionCall4Coll(addValue,
+ idxRel->rd_indcollation[keyno],
+ PointerGetDatum(bdesc),
+ PointerGetDatum(bval),
+ values[keyno],
+ nulls[keyno]);
+ /* if that returned true, we need to insert the updated tuple */
+ modified |= DatumGetBool(result);
+ }
+
+ return modified;
+}
+
+static bool
+check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys)
+{
+ int keyno;
+
+ /*
+ * First check if there are any IS [NOT] NULL scan keys, and if we're
+ * violating them.
+ */
+ for (keyno = 0; keyno < nnullkeys; keyno++)
+ {
+ ScanKey key = nullkeys[keyno];
+
+ Assert(key->sk_attno == bval->bv_attno);
+
+ /* Handle only IS NULL/IS NOT NULL tests */
+ if (!(key->sk_flags & SK_ISNULL))
+ continue;
+
+ if (key->sk_flags & SK_SEARCHNULL)
+ {
+ /* IS NULL scan key, but range has no NULLs */
+ if (!bval->bv_allnulls && !bval->bv_hasnulls)
+ return false;
+ }
+ else if (key->sk_flags & SK_SEARCHNOTNULL)
+ {
+ /*
+ * For IS NOT NULL, we can only skip ranges that are known to have
+ * only nulls.
+ */
+ if (bval->bv_allnulls)
+ return false;
+ }
+ else
+ {
+ /*
+ * Neither IS NULL nor IS NOT NULL was used; assume all indexable
+ * operators are strict and thus return false with NULL value in
+ * the scan key.
+ */
+ return false;
+ }
+ }
+
+ return true;
+}
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index a260074c91..b17077703c 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -110,6 +110,7 @@ brin_inclusion_opcinfo(PG_FUNCTION_ARGS)
*/
result = palloc0(MAXALIGN(SizeofBrinOpcInfo(3)) + sizeof(InclusionOpaque));
result->oi_nstored = 3;
+ result->oi_regular_nulls = true;
result->oi_opaque = (InclusionOpaque *)
MAXALIGN((char *) result + SizeofBrinOpcInfo(3));
@@ -141,7 +142,7 @@ brin_inclusion_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_BOOL(3);
+ bool isnull PG_USED_FOR_ASSERTS_ONLY = PG_GETARG_BOOL(3);
Oid colloid = PG_GET_COLLATION();
FmgrInfo *finfo;
Datum result;
@@ -149,18 +150,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
AttrNumber attno;
Form_pg_attribute attr;
- /*
- * If the new value is null, we record that we saw it if it's the first
- * one; otherwise, there's nothing to do.
- */
- if (isnull)
- {
- if (column->bv_hasnulls)
- PG_RETURN_BOOL(false);
-
- column->bv_hasnulls = true;
- PG_RETURN_BOOL(true);
- }
+ Assert(!isnull);
attno = column->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -268,52 +258,9 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
int nkeys = PG_GETARG_INT32(3);
Oid colloid = PG_GET_COLLATION();
int keyno;
- bool has_regular_keys = false;
-
- /* Handle IS NULL/IS NOT NULL tests */
- for (keyno = 0; keyno < nkeys; keyno++)
- {
- ScanKey key = keys[keyno];
- Assert(key->sk_attno == column->bv_attno);
-
- /* Skip regular scan keys (and remember that we have some). */
- if ((!key->sk_flags & SK_ISNULL))
- {
- has_regular_keys = true;
- continue;
- }
-
- if (key->sk_flags & SK_SEARCHNULL)
- {
- if (column->bv_allnulls || column->bv_hasnulls)
- continue; /* this key is fine, continue */
-
- PG_RETURN_BOOL(false);
- }
-
- /*
- * For IS NOT NULL, we can only skip ranges that are known to have
- * only nulls.
- */
- if (key->sk_flags & SK_SEARCHNOTNULL)
- {
- if (column->bv_allnulls)
- PG_RETURN_BOOL(false);
-
- continue;
- }
-
- /*
- * Neither IS NULL nor IS NOT NULL was used; assume all indexable
- * operators are strict and return false.
- */
- PG_RETURN_BOOL(false);
- }
-
- /* If there are no regular keys, the page range is considered consistent. */
- if (!has_regular_keys)
- PG_RETURN_BOOL(true);
+ /* make sure we got some scan keys */
+ Assert((nkeys > 0) && (keys != NULL));
/*
* If is all nulls, it cannot possibly be consistent (at this point we
@@ -331,9 +278,8 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
{
ScanKey key = keys[keyno];
- /* Skip IS NULL/IS NOT NULL keys (already handled above). */
- if (key->sk_flags & SK_ISNULL)
- continue;
+ /* NULL keys are handled and filtered-out in bringetbitmap */
+ Assert(!(key->sk_flags & SK_ISNULL));
/*
* When there are multiple scan keys, failure to meet the criteria for
@@ -574,37 +520,11 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
Datum result;
Assert(col_a->bv_attno == col_b->bv_attno);
-
- /* Adjust "hasnulls". */
- if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
- col_a->bv_hasnulls = true;
-
- /* If there are no values in B, there's nothing left to do. */
- if (col_b->bv_allnulls)
- PG_RETURN_VOID();
+ Assert(!col_a->bv_allnulls && !col_b->bv_allnulls);
attno = col_a->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
- /*
- * Adjust "allnulls". If A doesn't have values, just copy the values from
- * B into A, and we're done. We cannot run the operators in this case,
- * because values in A might contain garbage. Note we already established
- * that B contains values.
- */
- if (col_a->bv_allnulls)
- {
- col_a->bv_allnulls = false;
- col_a->bv_values[INCLUSION_UNION] =
- datumCopy(col_b->bv_values[INCLUSION_UNION],
- attr->attbyval, attr->attlen);
- col_a->bv_values[INCLUSION_UNMERGEABLE] =
- col_b->bv_values[INCLUSION_UNMERGEABLE];
- col_a->bv_values[INCLUSION_CONTAINS_EMPTY] =
- col_b->bv_values[INCLUSION_CONTAINS_EMPTY];
- PG_RETURN_VOID();
- }
-
/* If B includes empty elements, mark A similarly, if needed. */
if (!DatumGetBool(col_a->bv_values[INCLUSION_CONTAINS_EMPTY]) &&
DatumGetBool(col_b->bv_values[INCLUSION_CONTAINS_EMPTY]))
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index e116084a02..330bed0487 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -48,6 +48,7 @@ brin_minmax_opcinfo(PG_FUNCTION_ARGS)
result = palloc0(MAXALIGN(SizeofBrinOpcInfo(2)) +
sizeof(MinmaxOpaque));
result->oi_nstored = 2;
+ result->oi_regular_nulls = true;
result->oi_opaque = (MinmaxOpaque *)
MAXALIGN((char *) result + SizeofBrinOpcInfo(2));
result->oi_typcache[0] = result->oi_typcache[1] =
@@ -69,7 +70,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_USED_FOR_ASSERTS_ONLY = PG_GETARG_DATUM(3);
Oid colloid = PG_GET_COLLATION();
FmgrInfo *cmpFn;
Datum compar;
@@ -77,18 +78,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
Form_pg_attribute attr;
AttrNumber attno;
- /*
- * If the new value is null, we record that we saw it if it's the first
- * one; otherwise, there's nothing to do.
- */
- if (isnull)
- {
- if (column->bv_hasnulls)
- PG_RETURN_BOOL(false);
-
- column->bv_hasnulls = true;
- PG_RETURN_BOOL(true);
- }
+ Assert(!isnull);
attno = column->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -156,52 +146,9 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
int nkeys = PG_GETARG_INT32(3);
Oid colloid = PG_GET_COLLATION();
int keyno;
- bool has_regular_keys = false;
-
- /* handle IS NULL/IS NOT NULL tests */
- for (keyno = 0; keyno < nkeys; keyno++)
- {
- ScanKey key = keys[keyno];
-
- Assert(key->sk_attno == column->bv_attno);
-
- /* Skip regular scan keys (and remember that we have some). */
- if ((!key->sk_flags & SK_ISNULL))
- {
- has_regular_keys = true;
- continue;
- }
- if (key->sk_flags & SK_SEARCHNULL)
- {
- if (column->bv_allnulls || column->bv_hasnulls)
- continue; /* this key is fine, continue */
-
- PG_RETURN_BOOL(false);
- }
-
- /*
- * For IS NOT NULL, we can only skip ranges that are known to have
- * only nulls.
- */
- if (key->sk_flags & SK_SEARCHNOTNULL)
- {
- if (column->bv_allnulls)
- PG_RETURN_BOOL(false);
-
- continue;
- }
-
- /*
- * Neither IS NULL nor IS NOT NULL was used; assume all indexable
- * operators are strict and return false.
- */
- PG_RETURN_BOOL(false);
- }
-
- /* If there are no regular keys, the page range is considered consistent. */
- if (!has_regular_keys)
- PG_RETURN_BOOL(true);
+ /* make sure we got some scan keys */
+ Assert((nkeys > 0) && (keys != NULL));
/*
* If is all nulls, it cannot possibly be consistent (at this point we
@@ -215,9 +162,8 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
{
ScanKey key = keys[keyno];
- /* ignore IS NULL/IS NOT NULL tests handled above */
- if (key->sk_flags & SK_ISNULL)
- continue;
+ /* NULL keys are handled and filtered-out in bringetbitmap */
+ Assert(!(key->sk_flags & SK_ISNULL));
/*
* When there are multiple scan keys, failure to meet the criteria for
@@ -307,34 +253,11 @@ brin_minmax_union(PG_FUNCTION_ARGS)
bool needsadj;
Assert(col_a->bv_attno == col_b->bv_attno);
-
- /* Adjust "hasnulls" */
- if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
- col_a->bv_hasnulls = true;
-
- /* If there are no values in B, there's nothing left to do */
- if (col_b->bv_allnulls)
- PG_RETURN_VOID();
+ Assert(!col_a->bv_allnulls && !col_b->bv_allnulls);
attno = col_a->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
- /*
- * Adjust "allnulls". If A doesn't have values, just copy the values from
- * B into A, and we're done. We cannot run the operators in this case,
- * because values in A might contain garbage. Note we already established
- * that B contains values.
- */
- if (col_a->bv_allnulls)
- {
- col_a->bv_allnulls = false;
- col_a->bv_values[0] = datumCopy(col_b->bv_values[0],
- attr->attbyval, attr->attlen);
- col_a->bv_values[1] = datumCopy(col_b->bv_values[1],
- attr->attbyval, attr->attlen);
- PG_RETURN_VOID();
- }
-
/* Adjust minimum, if B's min is less than A's min */
finfo = minmax_get_strategy_procinfo(bdesc, attno, attr->atttypid,
BTLessStrategyNumber);
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 78c89a6961..79440ebe7b 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -27,6 +27,9 @@ typedef struct BrinOpcInfo
/* Number of columns stored in an index column of this opclass */
uint16 oi_nstored;
+ /* Regular processing of NULLs in BrinValues? */
+ bool oi_regular_nulls;
+
/* Opaque pointer for the opclass' private use */
void *oi_opaque;
--
2.26.2
--------------6A37408A2C42B5B56C3B3D49
Content-Type: text/x-patch; charset=UTF-8;
name="0003-Optimize-allocations-in-bringetbitmap-20210303.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0003-Optimize-allocations-in-bringetbitmap-20210303.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Changing the default random_page_cost value
@ 2024-09-27 14:07 Greg Sabino Mullane <[email protected]>
2024-09-27 14:26 ` Re: Changing the default random_page_cost value Roberto Mello <[email protected]>
2024-09-27 15:35 ` Re: Changing the default random_page_cost value Laurenz Albe <[email protected]>
2024-09-27 16:03 ` Re: Changing the default random_page_cost value Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 3 replies; 6+ messages in thread
From: Greg Sabino Mullane @ 2024-09-27 14:07 UTC (permalink / raw)
To: pgsql-hackers
tl;dr let's assume SSDs are popular and HDDs are the exception and flip our
default
As I write this email, it's the year 2024. I think it is time we lower our
"default" setting of random_page_cost (as set in postgresql.conf.sample and
the docs). Even a decade ago, the current default of 4 was considered
fairly conservative and often lowered. The git logs shows that this value
was last touched in 2006, during the age of spinning metal. We are now in a
new era, the age of SSDs, and thus we should lower this default value to
reflect the fact that the vast majority of people using Postgres these days
are doing so on solid state drives. We tend to stay ultra-conservative in
all of our settings, but we also need to recognize when there has been a
major shift in the underlying hardware - and calculations that our defaults
are based on.
Granted, there are other factors involved, and yes, perhaps we should tweak
some of the similar settings as well, but ranom_page_cost is the one
setting most out of sync with today's hardware realities. So I'll be brave
and throw a number out there: 1.2. And change our docs to say wordage like
"if you are using an older hard disk drive technology, you may want to try
raising rpc" to replace our fairly-hidden note about SSDs buried in the
last sentence - of the fourth paragraph - of the rpc docs.
Real data about performance on today's SSDs are welcome, and/or some way to
generate a more accurate default.
Cheers,
Greg
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Changing the default random_page_cost value
2024-09-27 14:07 Changing the default random_page_cost value Greg Sabino Mullane <[email protected]>
@ 2024-09-27 14:26 ` Roberto Mello <[email protected]>
2 siblings, 0 replies; 6+ messages in thread
From: Roberto Mello @ 2024-09-27 14:26 UTC (permalink / raw)
To: Greg Sabino Mullane <[email protected]>; +Cc: pgsql-hackers
On Fri, Sep 27, 2024 at 8:07 AM Greg Sabino Mullane <[email protected]>
wrote:
> tl;dr let's assume SSDs are popular and HDDs are the exception and flip
> our default
>
<snip>
> Granted, there are other factors involved, and yes, perhaps we should
> tweak some of the similar settings as well, but ranom_page_cost is the one
> setting most out of sync with today's hardware realities. So I'll be brave
> and throw a number out there: 1.2. And change our docs to say wordage like
> "if you are using an older hard disk drive technology, you may want to try
> raising rpc" to replace our fairly-hidden note about SSDs buried in the
> last sentence - of the fourth paragraph - of the rpc docs.
>
+1
I suggest a slightly nicer comment in the default conf file, like "For
spinning hard drives, raise this to at least 3 and test"
Roberto
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Changing the default random_page_cost value
2024-09-27 14:07 Changing the default random_page_cost value Greg Sabino Mullane <[email protected]>
@ 2024-09-27 15:35 ` Laurenz Albe <[email protected]>
2 siblings, 0 replies; 6+ messages in thread
From: Laurenz Albe @ 2024-09-27 15:35 UTC (permalink / raw)
To: Greg Sabino Mullane <[email protected]>; pgsql-hackers
On Fri, 2024-09-27 at 10:07 -0400, Greg Sabino Mullane wrote:
> So I'll be brave and throw a number out there: 1.2.
+1
Laurenz Albe
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Changing the default random_page_cost value
2024-09-27 14:07 Changing the default random_page_cost value Greg Sabino Mullane <[email protected]>
@ 2024-09-27 16:03 ` Dagfinn Ilmari Mannsåker <[email protected]>
2024-09-30 14:05 ` Re: Changing the default random_page_cost value Greg Sabino Mullane <[email protected]>
2 siblings, 1 reply; 6+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2024-09-27 16:03 UTC (permalink / raw)
To: Greg Sabino Mullane <[email protected]>; +Cc: pgsql-hackers
Greg Sabino Mullane <[email protected]> writes:
> So I'll be brave and throw a number out there: 1.2. And change our
> docs to say wordage like "if you are using an older hard disk drive
> technology, you may want to try raising rpc" to replace our
> fairly-hidden note about SSDs buried in the last sentence - of the
> fourth paragraph - of the rpc docs.
It might also be worth mentioning cloudy block storage (e.g. AWS' EBS),
which is typically backed by SSDs, but has extra network latency.
- ilmari
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Changing the default random_page_cost value
2024-09-27 14:07 Changing the default random_page_cost value Greg Sabino Mullane <[email protected]>
2024-09-27 16:03 ` Re: Changing the default random_page_cost value Dagfinn Ilmari Mannsåker <[email protected]>
@ 2024-09-30 14:05 ` Greg Sabino Mullane <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Greg Sabino Mullane @ 2024-09-30 14:05 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: pgsql-hackers
On Fri, Sep 27, 2024 at 12:03 PM Dagfinn Ilmari Mannsåker <[email protected]>
wrote:
> It might also be worth mentioning cloudy block storage (e.g. AWS' EBS),
> which is typically backed by SSDs, but has extra network latency.
>
That seems a little too in the weeds for me, but wording suggestions are
welcome. To get things moving forward, I made a doc patch which changes a
few things, namely:
* Mentions the distinction between ssd and hdd right up front.
* Moves the tablespace talk to the very end, as tablespace use is getting
rarer (again, thanks in part to ssds)
* Mentions the capability to set per-database and per-role since we mention
per-tablespace.
* Removes a lot of the talk of caches and justifications for the 4.0
setting. While those are interesting, I've been tuning this parameter for
many years and never really cared about the "90% cache rate". The proof is
in the pudding: rpc is the canonical "try it and see" parameter. Tweak.
Test. Repeat.
Cheers,
Greg
Attachments:
[application/octet-stream] 0001-Lower-random_page_cost-default-to-1.2-and-update-docs-about-it.patch (3.9K, ../../CAKAnmmKxOq8i4JCfc6AzThuetLPenGaOQu25m0EeOhanaQdhLw@mail.gmail.com/3-0001-Lower-random_page_cost-default-to-1.2-and-update-docs-about-it.patch)
download | inline diff:
From 50f4800e3501a84c0d41a5e29cf384bf465f230d Mon Sep 17 00:00:00 2001
From: Greg Sabino Mullane <[email protected]>
Date: Mon, 30 Sep 2024 09:52:28 -0400
Subject: [PATCH] Lower random_page_cost default to 1.2 and update docs about
it.
---
doc/src/sgml/config.sgml | 35 ++++++-------------
src/backend/utils/misc/postgresql.conf.sample | 2 +-
2 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0aec11f443..e901a71613 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5728,14 +5728,15 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
<listitem>
<para>
Sets the planner's estimate of the cost of a
- non-sequentially-fetched disk page. The default is 4.0.
- This value can be overridden for tables and indexes in a particular
- tablespace by setting the tablespace parameter of the same name
- (see <xref linkend="sql-altertablespace"/>).
- </para>
+ non-sequentially-fetched disk page. The default is <literal>1.2</literal>.
+ Note that this value assumes use of a solid-state drive, which
+ has a low random read cost relative to sequential scans. If you
+ are a using a hard disk drive, raising the value to <literal>3</literal>
+ or <literal>4</literal> may give you better performance.
+ </para>
<para>
- Reducing this value relative to <varname>seq_page_cost</varname>
+ Reducing <varname>random_page_cost</varname> relative to <varname>seq_page_cost</varname>
will cause the system to prefer index scans; raising it will
make index scans look relatively more expensive. You can raise
or lower both values together to change the importance of disk I/O
@@ -5744,24 +5745,10 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
</para>
<para>
- Random access to mechanical disk storage is normally much more expensive
- than four times sequential access. However, a lower default is used
- (4.0) because the majority of random accesses to disk, such as indexed
- reads, are assumed to be in cache. The default value can be thought of
- as modeling random access as 40 times slower than sequential, while
- expecting 90% of random reads to be cached.
- </para>
-
- <para>
- If you believe a 90% cache rate is an incorrect assumption
- for your workload, you can increase random_page_cost to better
- reflect the true cost of random storage reads. Correspondingly,
- if your data is likely to be completely in cache, such as when
- the database is smaller than the total server memory, decreasing
- random_page_cost can be appropriate. Storage that has a low random
- read cost relative to sequential, e.g., solid-state drives, might
- also be better modeled with a lower value for random_page_cost,
- e.g., <literal>1.1</literal>.
+ This value can be overridden for tables and indexes in a particular
+ tablespace by setting the tablespace parameter of the same name
+ (see <xref linkend="sql-altertablespace"/>). It can also be set
+ per-database and per-role, as needed.
</para>
<tip>
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 667e0dc40a..424df4a868 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -417,7 +417,7 @@
# - Planner Cost Constants -
#seq_page_cost = 1.0 # measured on an arbitrary scale
-#random_page_cost = 4.0 # same scale as above
+#random_page_cost = 1.2 # same scale as above
#cpu_tuple_cost = 0.01 # same scale as above
#cpu_index_tuple_cost = 0.005 # same scale as above
#cpu_operator_cost = 0.0025 # same scale as above
--
2.30.2
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-09-30 14:05 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-02 18:27 [PATCH 2/6] Move IS [NOT] NULL handling from BRIN support functions Tomas Vondra <[email protected]>
2024-09-27 14:07 Changing the default random_page_cost value Greg Sabino Mullane <[email protected]>
2024-09-27 14:26 ` Re: Changing the default random_page_cost value Roberto Mello <[email protected]>
2024-09-27 15:35 ` Re: Changing the default random_page_cost value Laurenz Albe <[email protected]>
2024-09-27 16:03 ` Re: Changing the default random_page_cost value Dagfinn Ilmari Mannsåker <[email protected]>
2024-09-30 14:05 ` Re: Changing the default random_page_cost value Greg Sabino Mullane <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox