public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/6] Move IS [NOT] NULL handling from BRIN support functions
3+ messages / 3 participants
[nested] [flat]
* [PATCH 2/6] Move IS [NOT] NULL handling from BRIN support functions
@ 2020-09-17 15:26 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Tomas Vondra @ 2020-09-17 15:26 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 | 106 +-------
src/backend/access/brin/brin_minmax.c | 103 +-------
src/include/access/brin_internal.h | 3 +
4 files changed, 239 insertions(+), 266 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index dc187153aa..14da9ed17f 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;
@@ -416,10 +396,13 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
/*
* Make room for per-attribute lists of scan keys that we'll pass to the
- * consistent support procedure.
+ * consistent support procedure. We keep null and regular keys separate,
+ * so that we can easily pass regular keys to the consistent function.
*/
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.
@@ -440,23 +423,24 @@ 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);
@@ -464,9 +448,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 */
@@ -544,15 +542,57 @@ 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
@@ -695,7 +735,6 @@ brinbuildCallback(Relation index,
{
BrinBuildState *state = (BrinBuildState *) brstate;
BlockNumber thisblock;
- int i;
thisblock = ItemPointerGetBlockNumber(tid);
@@ -724,25 +763,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);
}
/*
@@ -1521,6 +1543,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);
@@ -1574,3 +1629,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 215bc794d3..f4730be3b9 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);
@@ -264,63 +254,6 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
int nkeys = PG_GETARG_INT32(3);
Oid colloid = PG_GET_COLLATION();
int keyno;
- bool regular_keys = false;
-
- /*
- * First check if there are any IS NULL scan keys, and if we're
- * violating them. In that case we can terminate early, without
- * inspecting the ranges.
- */
- for (keyno = 0; keyno < nkeys; keyno++)
- {
- ScanKey key = keys[keyno];
-
- Assert(key->sk_attno == column->bv_attno);
-
- /* handle IS NULL/IS NOT NULL tests */
- if (key->sk_flags & SK_ISNULL)
- {
- 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);
- }
- else
- /* note we have regular (non-NULL) scan keys */
- regular_keys = true;
- }
-
- /*
- * If the page range is all nulls, it cannot possibly be consistent if
- * there are some regular scan keys.
- */
- if (column->bv_allnulls && regular_keys)
- PG_RETURN_BOOL(false);
-
- /* If there are no regular keys, the page range is considered consistent. */
- if (!regular_keys)
- PG_RETURN_BOOL(true);
/* It has to be checked, if it contains elements that are not mergeable. */
if (DatumGetBool(column->bv_values[INCLUSION_UNMERGEABLE]))
@@ -331,9 +264,8 @@ brin_inclusion_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
@@ -572,37 +504,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 12878ff3a0..6c8852d404 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);
@@ -152,72 +142,14 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
int nkeys = PG_GETARG_INT32(3);
Oid colloid = PG_GET_COLLATION();
int keyno;
- bool regular_keys = false;
-
- /*
- * First check if there are any IS NULL scan keys, and if we're
- * violating them. In that case we can terminate early, without
- * inspecting the ranges.
- */
- for (keyno = 0; keyno < nkeys; keyno++)
- {
- ScanKey key = keys[keyno];
-
- Assert(key->sk_attno == column->bv_attno);
-
- /* handle IS NULL/IS NOT NULL tests */
- if (key->sk_flags & SK_ISNULL)
- {
- 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);
- }
- else
- /* note we have regular (non-NULL) scan keys */
- regular_keys = true;
- }
-
- /*
- * If the page range is all nulls, it cannot possibly be consistent if
- * there are some regular scan keys.
- */
- if (column->bv_allnulls && regular_keys)
- PG_RETURN_BOOL(false);
-
- /* If there are no regular keys, the page range is considered consistent. */
- if (!regular_keys)
- PG_RETURN_BOOL(true);
/* Check that the range is consistent with all scan keys. */
for (keyno = 0; keyno < nkeys; keyno++)
{
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
@@ -308,34 +240,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
--------------310A2AE1CC4C2E2E77559E3D
Content-Type: text/x-patch; charset=UTF-8;
name="0003-Optimize-allocations-in-bringetbitmap-20210114-2.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0003-Optimize-allocations-in-bringetbitmap-20210114-2.patch"
^ permalink raw reply [nested|flat] 3+ messages in thread
* SSL tests fail on OpenSSL v3.2.0
@ 2023-11-27 18:05 Nazir Bilal Yavuz <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Nazir Bilal Yavuz @ 2023-11-27 18:05 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Andres Freund <[email protected]>
Hi,
SSL tests fail on OpenSSL v3.2.0. I tested both on macOS (CI) and
debian (my local) and both failed with the same errors. To trigger
these errors on CI, you may need to clear the repository cache;
otherwise macOS won't install the v3.2.0 of the OpenSSL.
001_ssltests:
psql exited with signal 6 (core dumped): 'psql: error: connection to
server at "127.0.0.1", port 56718 failed: server closed the connection
unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
SSL SYSCALL error: Connection reset by peer' while running 'psql -XAtq
-d sslkey=invalid sslcert=invalid sslrootcert=invalid sslcrl=invalid
sslcrldir=invalid user=ssltestuser dbname=trustdb hostaddr=127.0.0.1
host=common-name.pg-ssltest.test sslrootcert=invalid sslmode=require
-f - -w' at /Users/admin/pgsql/src/test/perl/PostgreSQL/Test/Cluster.pm
002_scram:
psql exited with signal 6 (core dumped): 'psql: error: connection to
server at "127.0.0.1", port 54531 failed: server closed the connection
unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
SSL SYSCALL error: Connection reset by peer' while running 'psql -XAtq
-d dbname=trustdb sslmode=require sslcert=invalid sslrootcert=invalid
hostaddr=127.0.0.1 host=localhost user=ssltestuser -f - -w' at
/Users/admin/pgsql/src/test/perl/PostgreSQL/Test/Cluster.pm line 1997.
003_sslinfo:
psql exited with signal 6 (core dumped): 'psql: error: connection to
server at "127.0.0.1", port 59337 failed: server closed the connection
unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
SSL SYSCALL error: Connection reset by peer' while running 'psql -XAtq
-d sslkey=invalid sslcert=invalid sslrootcert=invalid sslcrl=invalid
sslcrldir=invalid sslrootcert=ssl/root+server_ca.crt sslmode=require
dbname=certdb hostaddr=127.0.0.1 host=localhost user=ssltestuser
sslcert=ssl/client_ext.crt
sslkey=/Users/admin/pgsql/build/testrun/ssl/003_sslinfo/data/tmp_test_q11O/client_ext.key
-f - -w' at /Users/admin/pgsql/src/test/perl/PostgreSQL/Test/Cluster.pm
line 1997.
macOS CI run: https://cirrus-ci.com/task/5128008789393408
I couldn't find the cause yet but just wanted to inform you.
--
Regards,
Nazir Bilal Yavuz
Microsoft
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: SSL tests fail on OpenSSL v3.2.0
@ 2023-11-27 18:17 Tristan Partin <[email protected]>
parent: Nazir Bilal Yavuz <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Tristan Partin @ 2023-11-27 18:17 UTC (permalink / raw)
To: Nazir Bilal Yavuz <[email protected]>; +Cc: Andres Freund <[email protected]>; Bo Anderson <[email protected]>; pgsql-hackers
Nazir,
Thanks for opening a thread. Was just about to start one, here what we
came up with so far.
Homebrew users discovered a regression[0] when using Postgres compiled
and linked against OpenSSL version 3.2.
$ psql "postgresql://$DB?sslmode=require"
psql: error: connection to server at "redacted" (redacted), port 5432 failed: ERROR: Parameter 'user' is missing in startup packet.
double free or corruption (out)
Aborted (core dumped)
Analyzing the backtrace, OpenSSL was overwriting heap-allocated data in
our PGconn struct because it thought BIO::ptr was a struct bss_sock_st
*. OpenSSL then called a memset() on a member of that struct, and we
zeroed out data in our PGconn struct.
BIO_get_data(3) says the following:
> These functions are mainly useful when implementing a custom BIO.
>
> The BIO_set_data() function associates the custom data pointed to by ptr
> with the BIO a. This data can subsequently be retrieved via a call to
> BIO_get_data(). This can be used by custom BIOs for storing
> implementation specific information.
If you take a look at my_BIO_s_socket(), we create a partially custom
BIO, but for the most part are defaulting to the methods defined by
BIO_s_socket(). We need to set application-specific data and not BIO
private data, so that the BIO implementation we rely on, can properly
assert that its private data is what it expects.
The ssl test suite continues to pass with this patch. This patch should
be backported to every supported Postgres version most likely.
[0]: https://github.com/Homebrew/homebrew-core/issues/155651
--
Tristan Partin
Neon (https://neon.tech)
Attachments:
[text/x-patch] v1-0001-Use-BIO_-get-set-_app_data-instead-of-BIO_-get-se.patch (6.4K, ../../[email protected]/2-v1-0001-Use-BIO_-get-set-_app_data-instead-of-BIO_-get-se.patch)
download | inline diff:
From 672103a67aaf0dae5be6c5adcc5ce19e5b6d7676 Mon Sep 17 00:00:00 2001
From: Tristan Partin <[email protected]>
Date: Mon, 27 Nov 2023 11:49:52 -0600
Subject: [PATCH v1] Use BIO_{get,set}_app_data() instead of
BIO_{get,set}_data()
Compiling Postgres against OpenSSL 3.2 exposed a regression:
$ psql "postgresql://$DB?sslmode=require"
psql: error: connection to server at "redacted" (redacted), port 5432 failed: ERROR: Parameter 'user' is missing in startup packet.
double free or corruption (out)
Aborted (core dumped)
Analyzing the backtrace, openssl was overwriting heap-allocated data in
our PGconn struct because it thought BIO::ptr was a struct bss_sock_st
*. OpenSSL then called a memset() on a member of that struct, and we
zeroed out data in our PGconn struct.
BIO_get_data(3) says the following:
> These functions are mainly useful when implementing a custom BIO.
>
> The BIO_set_data() function associates the custom data pointed to by ptr
> with the BIO a. This data can subsequently be retrieved via a call to
> BIO_get_data(). This can be used by custom BIOs for storing
> implementation specific information.
If you take a look at my_BIO_s_socket(), we create a partially custom
BIO, but for the most part are defaulting to the methods defined by
BIO_s_socket(). We need to set application-specific data and not BIO
private data, so that the BIO implementation we rely on, can properly
assert that its private data is what it expects.
Co-authored-by: Bo Andreson <[email protected]>
---
meson.build | 1 -
src/backend/libpq/be-secure-openssl.c | 11 +++--------
src/include/pg_config.h.in | 3 ---
src/interfaces/libpq/fe-secure-openssl.c | 20 +++-----------------
src/tools/msvc/Solution.pm | 2 --
5 files changed, 6 insertions(+), 31 deletions(-)
diff --git a/meson.build b/meson.build
index ee58ee7a06..0095fb183a 100644
--- a/meson.build
+++ b/meson.build
@@ -1285,7 +1285,6 @@ if sslopt in ['auto', 'openssl']
# doesn't have these OpenSSL 1.1.0 functions. So check for individual
# functions.
['OPENSSL_init_ssl'],
- ['BIO_get_data'],
['BIO_meth_new'],
['ASN1_STRING_get0_data'],
['HMAC_CTX_new'],
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 31b6a6eacd..1b8b32c5b3 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -842,11 +842,6 @@ be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
* see sock_read() and sock_write() in OpenSSL's crypto/bio/bss_sock.c.
*/
-#ifndef HAVE_BIO_GET_DATA
-#define BIO_get_data(bio) (bio->ptr)
-#define BIO_set_data(bio, data) (bio->ptr = data)
-#endif
-
static BIO_METHOD *my_bio_methods = NULL;
static int
@@ -856,7 +851,7 @@ my_sock_read(BIO *h, char *buf, int size)
if (buf != NULL)
{
- res = secure_raw_read(((Port *) BIO_get_data(h)), buf, size);
+ res = secure_raw_read(((Port *) BIO_get_app_data(h)), buf, size);
BIO_clear_retry_flags(h);
if (res <= 0)
{
@@ -876,7 +871,7 @@ my_sock_write(BIO *h, const char *buf, int size)
{
int res = 0;
- res = secure_raw_write(((Port *) BIO_get_data(h)), buf, size);
+ res = secure_raw_write(((Port *) BIO_get_app_data(h)), buf, size);
BIO_clear_retry_flags(h);
if (res <= 0)
{
@@ -952,7 +947,7 @@ my_SSL_set_fd(Port *port, int fd)
SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB);
goto err;
}
- BIO_set_data(bio, port);
+ BIO_set_app_data(bio, port);
BIO_set_fd(bio, fd, BIO_NOCLOSE);
SSL_set_bio(port->ssl, bio, bio);
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index d8a2985567..5f16918243 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -66,9 +66,6 @@
/* Define to 1 if you have the `backtrace_symbols' function. */
#undef HAVE_BACKTRACE_SYMBOLS
-/* Define to 1 if you have the `BIO_get_data' function. */
-#undef HAVE_BIO_GET_DATA
-
/* Define to 1 if you have the `BIO_meth_new' function. */
#undef HAVE_BIO_METH_NEW
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 4aeaf08312..dc5aa79fe8 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1806,20 +1806,6 @@ PQsslAttribute(PGconn *conn, const char *attribute_name)
return NULL; /* unknown attribute */
}
-/*
- * Private substitute BIO: this does the sending and receiving using
- * pqsecure_raw_write() and pqsecure_raw_read() instead, to allow those
- * functions to disable SIGPIPE and give better error messages on I/O errors.
- *
- * These functions are closely modelled on the standard socket BIO in OpenSSL;
- * see sock_read() and sock_write() in OpenSSL's crypto/bio/bss_sock.c.
- */
-
-#ifndef HAVE_BIO_GET_DATA
-#define BIO_get_data(bio) (bio->ptr)
-#define BIO_set_data(bio, data) (bio->ptr = data)
-#endif
-
/* protected by ssl_config_mutex */
static BIO_METHOD *my_bio_methods;
@@ -1828,7 +1814,7 @@ my_sock_read(BIO *h, char *buf, int size)
{
int res;
- res = pqsecure_raw_read((PGconn *) BIO_get_data(h), buf, size);
+ res = pqsecure_raw_read((PGconn *) BIO_get_app_data(h), buf, size);
BIO_clear_retry_flags(h);
if (res < 0)
{
@@ -1858,7 +1844,7 @@ my_sock_write(BIO *h, const char *buf, int size)
{
int res;
- res = pqsecure_raw_write((PGconn *) BIO_get_data(h), buf, size);
+ res = pqsecure_raw_write((PGconn *) BIO_get_app_data(h), buf, size);
BIO_clear_retry_flags(h);
if (res < 0)
{
@@ -1968,7 +1954,7 @@ my_SSL_set_fd(PGconn *conn, int fd)
SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB);
goto err;
}
- BIO_set_data(bio, conn);
+ BIO_set_app_data(bio, conn);
SSL_set_bio(conn->ssl, bio, bio);
BIO_set_fd(bio, fd, BIO_NOCLOSE);
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 98a5b5d872..a980b51f5d 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -224,7 +224,6 @@ sub GenerateFiles
HAVE_ATOMICS => 1,
HAVE_ATOMIC_H => undef,
HAVE_BACKTRACE_SYMBOLS => undef,
- HAVE_BIO_GET_DATA => undef,
HAVE_BIO_METH_NEW => undef,
HAVE_COMPUTED_GOTO => undef,
HAVE_COPYFILE => undef,
@@ -502,7 +501,6 @@ sub GenerateFiles
|| ($digit1 >= '1' && $digit2 >= '1' && $digit3 >= '0'))
{
$define{HAVE_ASN1_STRING_GET0_DATA} = 1;
- $define{HAVE_BIO_GET_DATA} = 1;
$define{HAVE_BIO_METH_NEW} = 1;
$define{HAVE_HMAC_CTX_FREE} = 1;
$define{HAVE_HMAC_CTX_NEW} = 1;
--
Tristan Partin
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-11-27 18:17 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17 15:26 [PATCH 2/6] Move IS [NOT] NULL handling from BRIN support functions Tomas Vondra <[email protected]>
2023-11-27 18:05 SSL tests fail on OpenSSL v3.2.0 Nazir Bilal Yavuz <[email protected]>
2023-11-27 18:17 ` Re: SSL tests fail on OpenSSL v3.2.0 Tristan Partin <[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