From 3f7e2f05570a11b430e40c184b867775cce5efe9 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Thu, 20 Oct 2022 19:55:23 +0200 Subject: [PATCH 2/2] fixup: brin has_nulls --- src/backend/access/brin/brin.c | 76 ++++++++++++++++++--- src/backend/access/brin/brin_minmax_multi.c | 1 + src/backend/access/brin/brin_tuple.c | 13 +++- src/test/regress/expected/brin.out | 2 +- src/test/regress/expected/brin_bloom.out | 2 +- src/test/regress/expected/brin_multi.out | 2 +- 6 files changed, 83 insertions(+), 13 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 20b7d65b948..ee9b3bb0574 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -1568,15 +1568,36 @@ form_and_insert_tuple(BrinBuildState *state) { BrinTuple *tup; Size size; + bool modified = false; + BrinMemTuple *dtuple = state->bs_dtuple; + int i; - tup = brin_form_tuple(state->bs_bdesc, state->bs_currRangeStart, - state->bs_dtuple, &size); - brin_doinsert(state->bs_irel, state->bs_pagesPerRange, state->bs_rmAccess, - &state->bs_currentInsertBuf, state->bs_currRangeStart, - tup, size); - state->bs_numtuples++; + /* + * Was the memtuple modified (any tuples added to it)? + * + * Should be enough to check just the first attribute - either we add a row + * to all columns or none of them. + */ + for (i = 0; i < state->bs_bdesc->bd_tupdesc->natts; i++) + { + if (!(dtuple->bt_columns[i].bv_allnulls && + dtuple->bt_columns[i].bv_hasnulls)) + { + modified = true; + break; + } + } - pfree(tup); + if (modified) + { + tup = brin_form_tuple(state->bs_bdesc, state->bs_currRangeStart, + state->bs_dtuple, &size); + brin_doinsert(state->bs_irel, state->bs_pagesPerRange, state->bs_rmAccess, + &state->bs_currentInsertBuf, state->bs_currRangeStart, + tup, size); + state->bs_numtuples++; + pfree(tup); + } } /* @@ -1710,24 +1731,53 @@ add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup, Datum result; BrinValues *bval; FmgrInfo *addValue; + bool first_row; + bool has_nulls = false; bval = &dtup->bt_columns[keyno]; + /* + * Is this the first tuple we're adding to the range range? We track + * that by setting both bv_hasnulls and bval->bv_allnulls to true + * during initialization. But it's not a valid combination (at most + * one of those flags should be set), so we reset the second flag. + */ + first_row = (bval->bv_hasnulls && bval->bv_allnulls); + + if (bval->bv_hasnulls && bval->bv_allnulls) + { + bval->bv_hasnulls = false; + modified = true; + } + 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. + * + * XXX This used to check "hasnulls" but now that might result in + * having both flags set. That used to be OK, because we just + * ignore hasnulls flag in brin_form_tuple when allnulls=true. + * But now we interpret this combination as "firt row" so it + * would confuse following calls. So make sure to only set one + * of the flags - when allnulls=true we're done, as it already + * marks the range as containing ranges. */ - if (!bval->bv_hasnulls) + if (!bval->bv_allnulls) { bval->bv_hasnulls = true; modified = true; } - continue; } + /* + * Does the range already has NULL values? Either of the flags can + * be set, but we ignore the state before adding first row. + */ + has_nulls = (bval->bv_hasnulls || bval->bv_allnulls) && !first_row; + addValue = index_getprocinfo(idxRel, keyno + 1, BRIN_PROCNUM_ADDVALUE); result = FunctionCall4Coll(addValue, @@ -1736,8 +1786,16 @@ add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup, PointerGetDatum(bval), values[keyno], nulls[keyno]); + /* if that returned true, we need to insert the updated tuple */ modified |= DatumGetBool(result); + + /* + * If we had NULLS, and the opclass didn't set allnulls=true, set + * the hasnulls so that we know there are NULL values. + */ + if (has_nulls && !bval->bv_allnulls) + bval->bv_hasnulls = true; } return modified; diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c index 9a0bcf6698d..d5ce5b47ff4 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -2500,6 +2500,7 @@ brin_minmax_multi_add_value(PG_FUNCTION_ARGS) MemoryContextSwitchTo(oldctx); column->bv_allnulls = false; + modified = true; column->bv_mem_value = PointerGetDatum(ranges); diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c index c0e2dbd23ba..7ea272c2f52 100644 --- a/src/backend/access/brin/brin_tuple.c +++ b/src/backend/access/brin/brin_tuple.c @@ -136,6 +136,9 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple, { int datumno; + Assert(!(tuple->bt_columns[keyno].bv_hasnulls && + tuple->bt_columns[keyno].bv_allnulls)); + /* * "allnulls" is set when there's no nonnull value in any row in the * column; when this happens, there is no data to store. Thus set the @@ -517,7 +520,7 @@ brin_memtuple_initialize(BrinMemTuple *dtuple, BrinDesc *brdesc) { dtuple->bt_columns[i].bv_attno = i + 1; dtuple->bt_columns[i].bv_allnulls = true; - dtuple->bt_columns[i].bv_hasnulls = false; + dtuple->bt_columns[i].bv_hasnulls = true; dtuple->bt_columns[i].bv_values = (Datum *) currdatum; dtuple->bt_columns[i].bv_mem_value = PointerGetDatum(NULL); @@ -585,6 +588,14 @@ brin_deform_tuple(BrinDesc *brdesc, BrinTuple *tuple, BrinMemTuple *dMemtuple) { int i; + Assert(!(allnulls[keyno] && hasnulls[keyno])); + + /* + * Make sure to overwrite the hasnulls flag, because it might have + * been initialized as true by brin_memtuple_initialize. + */ + dtup->bt_columns[keyno].bv_hasnulls = hasnulls[keyno]; + if (allnulls[keyno]) { valueno += brdesc->bd_info[keyno]->oi_nstored; diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out index 73fa38396e4..ebc31222354 100644 --- a/src/test/regress/expected/brin.out +++ b/src/test/regress/expected/brin.out @@ -454,7 +454,7 @@ $$; SELECT brin_summarize_range('brin_summarize_idx', 0); brin_summarize_range ---------------------- - 0 + 1 (1 row) -- nothing: already summarized diff --git a/src/test/regress/expected/brin_bloom.out b/src/test/regress/expected/brin_bloom.out index 32c56a996a2..6e847f9113d 100644 --- a/src/test/regress/expected/brin_bloom.out +++ b/src/test/regress/expected/brin_bloom.out @@ -373,7 +373,7 @@ $$; SELECT brin_summarize_range('brin_summarize_bloom_idx', 0); brin_summarize_range ---------------------- - 0 + 1 (1 row) -- nothing: already summarized diff --git a/src/test/regress/expected/brin_multi.out b/src/test/regress/expected/brin_multi.out index f3309f433f8..e65f1c20d4f 100644 --- a/src/test/regress/expected/brin_multi.out +++ b/src/test/regress/expected/brin_multi.out @@ -407,7 +407,7 @@ $$; SELECT brin_summarize_range('brin_summarize_multi_idx', 0); brin_summarize_range ---------------------- - 0 + 1 (1 row) -- nothing: already summarized -- 2.37.3