On 1/9/23 00:34, Tomas Vondra wrote:
>
> I've been working on this over the past couple days, trying to polish
> and commit it over the weekend - both into master and backbranches.
> Sadly, the backpatching part turned out to be a bit more complicated
> than I expected, because of the BRIN reworks in PG14 (done by me, as
> foundation for the new opclasses, so ... well).
>
> Anyway, I got it done, but it's a bit uglier than I hoped for and I
> don't feel like pushing this on Sunday midnight. I think it's correct,
> but maybe another pass to polish it a bit more is better.
>
> So here are two patches - one for 11-13, the other for 14-master.
>
I spent a bit more time on this fix. I realized there are two more
places that need fixes.
Firstly, the placeholder tuple needs to be marked as "empty" too, so
that it can be correctly updated by other backends etc.
Secondly, union_tuples had a couple bugs in handling empty ranges (this
is related to the placeholder tuple changes). I wonder what's the best
way to test this in an automated way - it's very dependent on timing of
the concurrent updated. For example we need to do something like this:
T1: run pg_summarize_range() until it inserts the placeholder tuple
T2: do an insert into the page range (updates placeholder)
T1: continue pg_summarize_range() to merge into the placeholder
But there are no convenient ways to do this, I think. I had to check the
various cases using breakpoints in gdb etc.
I'm not very happy with the union_tuples() changes - it's quite verbose,
perhaps a bit too verbose. We have to check for empty ranges first, and
then various combinations of allnulls/hasnulls flags for both BRIN
tuples. There are 9 combinations, and the current code just checks them
one by one - I was getting repeatedly confused by the original code, but
maybe it's too much.
As for the backpatch, I tried to keep it as close to the 14+ fixes as
possible, but it effectively backports some of the 14+ BRIN changes. In
particular, 14+ moved most of the NULL-handling logic from opclasses to
brin.c, and I think it's reasonable to do that for the backbranches too.
The alternative is to apply the same fix to every BRIN_PROCNUM_UNION
opclass procedure out there. I guess doing that for minmax+inclusion is
not a huge deal, but what about external opclasses? And without the fix
the indexes are effectively broken. Fixing this outside in brin.c (in
the union procedure) fixes this for every opclass procedure, without any
actual limitation of functinality (14+ does that anyway).
But maybe someone thinks this is a bad idea and we should do something
else in the backbranches?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 0abf47f311bfb0b03e5349b12c8e67ad3d5c0842 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.von...@postgresql.org>
Date: Sun, 8 Jan 2023 16:43:06 +0100
Subject: [PATCH] Fix handling of NULLs in BRIN indexes
BRIN indexes did not properly distinguish between summaries for empty
(no rows) and all-NULL ranges. All summaries were initialized with
allnulls=true, and the opclasses simply reset allnulls to false when
processing the first non-NULL value. This however fails if the range
starts with a NULL value (or a sequence of NULL values), in which case
we forget the range contains NULL values.
This happens because the allnulls flag is used for two separate
purposes - to mark empty ranges (not representing any rows yet) and
ranges containing only NULL values.
Opclasses don't know which of these cases it is, and so don't know
whether to set hasnulls=true. Setting hasnulls=true in both cases would
make it correct, but it would also make BRIN indexes useless for queries
with IS NULL clauses - all ranges start empty (and thus allnulls=true),
so all ranges would end up with either allnulls=true or hasnulls=true.
The severity of the issue is somewhat reduced by the fact that it only
happens when adding values to an existing summary with allnulls=true,
not when the summarization is processing values in bulk (e.g. during
CREATE INDEX or automatic summarization). In this case the flags were
updated in a slightly different way, not forgetting the NULL values.
The best solution would be to introduce a new flag marking index tuples
representing ranges with no rows, but that would break on-disk format
and/or ABI, depending on where we put the flag. Considering we need to
backpatch this, that's not acceptable.
So instead we use an "impossible" combination of both flags (allnulls
and hasnulls) set to true, to mark "empty" ranges with no rows. In
principle "empty" is a feature of the whole index tuple, which may
contain multiple summaries in a multi-column index, but this is where
the flags are, unfortunately.
We could also skip storing index tuples for empty summaries, but then
we'd have to always process such ranges - even if there are no rows in
large parts of the table (e.g. after a bulk DELETE), it would still
require reading the pages etc. So we store them, but ignore them when
building the bitmap.
Backpatch to 11. The issue exists since BRIN indexes were introduced in
9.5, but older releases are already EOL.
Backpatch-through: 11
Reviewed-by: Justin Pryzby, Matthias van de Meent
Discussion: https://postgr.es/m/402430e4-7d9d-6cf1-09ef-464d80aff...@enterprisedb.com
---
src/backend/access/brin/brin.c | 223 +++++++++++++++++-
src/backend/access/brin/brin_tuple.c | 31 ++-
...summarization-and-inprogress-insertion.out | 8 +-
...ummarization-and-inprogress-insertion.spec | 1 +
4 files changed, 244 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index b5a5fa7b334..a7c2c072bd4 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -70,6 +70,8 @@ typedef struct BrinOpaque
#define BRIN_ALL_BLOCKRANGES InvalidBlockNumber
+#define BRIN_RANGE_IS_EMPTY(col) ((col)->bv_allnulls && (col)->bv_hasnulls)
+
static BrinBuildState *initialize_brin_buildstate(Relation idxRel,
BrinRevmap *revmap, BlockNumber pagesPerRange);
static void terminate_brin_buildstate(BrinBuildState *state);
@@ -591,6 +593,17 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
bval = &dtup->bt_columns[attno - 1];
+ /*
+ * If the range has both allnulls and hasnulls set, it means
+ * there are no rows in the range, so we can skip it (we know
+ * there's nothing to match).
+ */
+ if (BRIN_RANGE_IS_EMPTY(bval))
+ {
+ addrange = false;
+ break;
+ }
+
/*
* First check if there are any IS [NOT] NULL scan keys,
* and if we're violating them. In that case we can
@@ -1615,26 +1628,99 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b)
if (opcinfo->oi_regular_nulls)
{
- /* Adjust "hasnulls". */
- if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
- col_a->bv_hasnulls = true;
+ /*
+ * If B is empty (represents no rows), ignore it and just keep
+ * A as is (might be empty etc.).
+ */
+ if (BRIN_RANGE_IS_EMPTY(col_b))
+ continue;
+
+ /*
+ * Now we know B is not empty - it has either NULLs or data, or
+ * some combination of it. We need to merge it into A somehow.
+ *
+ * If A is empty, we simply copy all the flags and data from B.
+ */
+ if (BRIN_RANGE_IS_EMPTY(col_a))
+ {
+ int i;
+
+ col_a->bv_allnulls = col_b->bv_allnulls;
+ col_a->bv_hasnulls = col_b->bv_hasnulls;
+
+ /* If B has no data, we're done. */
+ if (col_b->bv_allnulls)
+ continue;
+
+ 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);
- /* 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.
+ * Both A and B are not empty, and we need to merge B into A.
+ * There are multiple combinations of allnulls/hasnulls flags.
+ * We've handled the "empty" case on either side above, so we
+ * can ignore those cases - which leaves 3 flag combinations
+ * on each side, so 9 combinations in total.
+ *
+ * A:all A:has B:all B:has
+ * true false true false - nothing to do
+ * true false false true - set A:has=true, copy from B
+ * true false false false - set A:has=true, copy from B
+ *
+ * false true true false - nothing to do
+ * false true false true - flags OK, call union proc
+ * false true false false - flags OK, call union proc
+ *
+ * false false true false - set A:has=true
+ * false false false true - set A:has=true, call union proc
+ * false false false false - flags OK, call union proc
*/
- if (col_a->bv_allnulls)
+ if (col_a->bv_allnulls && col_b->bv_allnulls)
+ {
+ /* nothing to do - both sides are NULL-only */
+ continue;
+ }
+ else if (col_a->bv_allnulls && col_b->bv_hasnulls)
{
- int i;
+ int i;
+ /*
+ * A is NULL-only, but B has some non-NULL values too. So the
+ * result has both NULLs and non-NULL values.
+ */
+ col_a->bv_allnulls = false;
+ col_a->bv_hasnulls = true;
+ /* copy data from B to A */
+ 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;
+ }
+ else if (col_a->bv_allnulls) /* B has no NULLs */
+ {
+ int i;
+
+ /*
+ * A is NULL-only, but B has some non-NULL values too. So the
+ * result has both NULLs and non-NULL values.
+ *
+ * XXX This is the same as the preceding branch, but I've left
+ * it here to keep the branches mapped 1:1 to the table of
+ * combinations.
+ */
col_a->bv_allnulls = false;
+ col_a->bv_hasnulls = true;
+ /* copy data from B to A */
for (i = 0; i < opcinfo->oi_nstored; i++)
col_a->bv_values[i] =
datumCopy(col_b->bv_values[i],
@@ -1643,6 +1729,55 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b)
continue;
}
+ else if (col_a->bv_hasnulls && col_b->bv_allnulls)
+ {
+ /* Nothing to do (flags are correct, no data to copy). */
+ continue;
+ }
+ else if (col_a->bv_hasnulls && col_b->bv_hasnulls)
+ {
+ /*
+ * Flags are correct, but both A and B have non-NULL values.
+ * So we have to call the support proc BRIN_PROCNUM_UNION
+ * (so no 'continue' here).
+ */
+ }
+ else if (col_a->bv_hasnulls) /* B has no NULLs */
+ {
+ /*
+ * B has no NULL values, so flags are OK. But both sides have
+ * some non-NULL values, so we have to call the support proc
+ * (so no 'continue' here).
+ *
+ * XXX Same as the preceding branch, but kept for 1:1 mapping.
+ */
+ }
+ else if (col_b->bv_allnulls) /* A has no NULLs */
+ {
+ /*
+ * Just update the hasnulls flag to remember B has NULL values
+ * and we're done (no data non-NULL values to copy/merge).
+ */
+ col_a->bv_hasnulls = true;
+ continue;
+ }
+ else if (col_b->bv_hasnulls) /* A has no NULLs */
+ {
+ /*
+ * Update the hasnulls flag to remember B has NULL values, but
+ * both sides have some non-NULL data so we needto call the
+ * BRIN_PROCNUM_UNION procedure (so no 'continue' here).
+ */
+ col_a->bv_hasnulls = true;
+ }
+ else
+ {
+ /*
+ * Neither side has any NULL values, both sides have non-NULL
+ * values, so we need to call the BRIN_PROCNUM_UNION proc (so
+ * no 'continue' here).
+ */
+ }
}
unionFn = index_getprocinfo(bdesc->bd_index, keyno + 1,
@@ -1717,19 +1852,67 @@ add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup,
Datum result;
BrinValues *bval;
FmgrInfo *addValue;
+ bool hasnulls;
bval = &dtup->bt_columns[keyno];
+ /*
+ * Does the range have actual NULL values? Either of the flags can
+ * be set, but we ignore the state before adding first row.
+ *
+ * We have to remember this, because we'll modify the flags and we
+ * need to know if the range started as empty.
+ */
+ hasnulls = (!BRIN_RANGE_IS_EMPTY(bval)) &&
+ (bval->bv_hasnulls || bval->bv_allnulls);
+
+ /*
+ * We need to consider whether the range is empty (not representing
+ * any rows yet), i.e. if it has both flags (allnulls hasnulls) set
+ * to true.
+ *
+ * If the range is empty, we clear the hasnulls flag - after adding
+ * a value it won't be empty anymore. Either it'll be all-NULL (and
+ * leaving allnulls=true covers that), or it will have no NULLs at
+ * all (but building the state is up to the opclass).
+ *
+ * If the range is not empty, we remember if there are NULL values.
+ * In this case both flags can't be set to true (that'd be empty
+ * range), so it's either allnulls=true or hasnulls=true. But the
+ * opclasses clear allnulls when adding the first non-NULL value,
+ * so we need to remember this.
+ *
+ * When adding a null value we can do everything locally, without
+ * calling BRIN_PROCNUM_ADDVALUE.
+ */
+ if (BRIN_RANGE_IS_EMPTY(bval))
+ {
+ bval->bv_hasnulls = false;
+ modified = true;
+ }
+
+ /*
+ * If the value we're adding is NULL, handle it locally. Otherwise
+ * call the BRIN_PROCNUM_ADDVALUE procedure.
+ */
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.
+ *
+ * We can't check "bv_hasnulls" because then we might end up with
+ * both flags set to true, which is interpreted as empty range.
+ * But that'd be wrong, because we've just added a value.
+ *
+ * So either the range has allnulls=true, or we have to set the
+ * hasnulls flag. Check if we're changing the value to determine
+ * if the index tuple was modified.
*/
- if (!bval->bv_hasnulls)
+ if (!bval->bv_allnulls)
{
+ modified |= (!bval->bv_hasnulls);
bval->bv_hasnulls = true;
- modified = true;
}
continue;
@@ -1745,6 +1928,20 @@ add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup,
nulls[keyno]);
/* if that returned true, we need to insert the updated tuple */
modified |= DatumGetBool(result);
+
+ /*
+ * If the range was not empty and had NULL values, make sure we don't
+ * forget about the NULL values. Either the allnulls flag is still set
+ * to true, or (if the opclass cleared it) we need to set hasnulls=true.
+ */
+ if (hasnulls && !bval->bv_allnulls)
+ {
+ modified |= (!bval->bv_hasnulls);
+ bval->bv_hasnulls = true;
+ }
+
+ /* We've added a row, so the summary should not be empty. */
+ Assert(!BRIN_RANGE_IS_EMPTY(bval));
}
return modified;
diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c
index 84b79dbfc0d..b2292a0c1a9 100644
--- a/src/backend/access/brin/brin_tuple.c
+++ b/src/backend/access/brin/brin_tuple.c
@@ -417,7 +417,20 @@ brin_form_placeholder_tuple(BrinDesc *brdesc, BlockNumber blkno, Size *size)
*bitP |= bitmask;
}
- /* no need to set hasnulls */
+ /* set hasnulls true for all attributes */
+ for (keyno = 0; keyno < brdesc->bd_tupdesc->natts; keyno++)
+ {
+ if (bitmask != HIGHBIT)
+ bitmask <<= 1;
+ else
+ {
+ bitP += 1;
+ *bitP = 0x0;
+ bitmask = 1;
+ }
+
+ *bitP |= bitmask;
+ }
*size = len;
return rettuple;
@@ -516,8 +529,15 @@ brin_memtuple_initialize(BrinMemTuple *dtuple, BrinDesc *brdesc)
for (i = 0; i < brdesc->bd_tupdesc->natts; i++)
{
dtuple->bt_columns[i].bv_attno = i + 1;
+
+ /*
+ * Each memtuple starts as if it represents no rows, which is indicated
+ * by having bot allnulls and hasnulls set to true. We track this for
+ * all columns, because we don't have a flag for the whole memtuple.
+ */
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 +605,13 @@ brin_deform_tuple(BrinDesc *brdesc, BrinTuple *tuple, BrinMemTuple *dMemtuple)
{
int i;
+ /*
+ * Make sure to overwrite the hasnulls flag, because it was initialized
+ * to true by brin_memtuple_initialize and we don't want to skip it if
+ * allnulls=true.
+ */
+ dtup->bt_columns[keyno].bv_hasnulls = hasnulls[keyno];
+
if (allnulls[keyno])
{
valueno += brdesc->bd_info[keyno]->oi_nstored;
diff --git a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out
index 2a4755d0998..584ac2602f7 100644
--- a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out
+++ b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out
@@ -4,7 +4,7 @@ starting permutation: s2check s1b s2b s1i s2summ s1c s2c s2check
step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value
----------+------+------+--------+--------+-----------+--------
- 1| 0| 1|f |f |f |{1 .. 1}
+ 1| 0| 1|f |t |f |{1 .. 1}
(1 row)
step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
@@ -26,7 +26,7 @@ step s2c: COMMIT;
step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value
----------+------+------+--------+--------+-----------+-----------
- 1| 0| 1|f |f |f |{1 .. 1}
+ 1| 0| 1|f |t |f |{1 .. 1}
2| 1| 1|f |f |f |{1 .. 1000}
(2 rows)
@@ -35,7 +35,7 @@ starting permutation: s2check s1b s1i s2vacuum s1c s2check
step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value
----------+------+------+--------+--------+-----------+--------
- 1| 0| 1|f |f |f |{1 .. 1}
+ 1| 0| 1|f |t |f |{1 .. 1}
(1 row)
step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
@@ -45,7 +45,7 @@ step s1c: COMMIT;
step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value
----------+------+------+--------+--------+-----------+-----------
- 1| 0| 1|f |f |f |{1 .. 1}
+ 1| 0| 1|f |t |f |{1 .. 1}
2| 1| 1|f |f |f |{1 .. 1000}
(2 rows)
diff --git a/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec b/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec
index 19ac18a2e88..18ba92b7ba1 100644
--- a/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec
+++ b/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec
@@ -9,6 +9,7 @@ setup
) WITH (fillfactor=10);
CREATE INDEX brinidx ON brin_iso USING brin (value) WITH (pages_per_range=1);
-- this fills the first page
+ INSERT INTO brin_iso VALUES (NULL);
DO $$
DECLARE curtid tid;
BEGIN
--
2.39.2
From fd5f37eafc27f42674768ea5593e3309f5ad07a7 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.von...@postgresql.org>
Date: Sun, 8 Jan 2023 22:04:41 +0100
Subject: [PATCH] Fix handling of NULLs in BRIN indexes
BRIN indexes did not properly distinguish between summaries for empty
(no rows) and all-NULL ranges. All summaries were initialized with
allnulls=true, and the opclasses simply reset allnulls to false when
processing the first non-NULL value. This however fails if the range
starts with a NULL value (or a sequence of NULL values), in which case
we forget the range contains NULL values.
This happens because the allnulls flag is used for two separate
purposes - to mark empty ranges (not representing any rows yet) and
ranges containing only NULL values.
Opclasses don't know which of these cases it is, and so don't know
whether to set hasnulls=true. Setting hasnulls=true in both cases would
make it correct, but it would also make BRIN indexes useless for queries
with IS NULL clauses - all ranges start empty (and thus allnulls=true),
so all ranges would end up with either allnulls=true or hasnulls=true.
The severity of the issue is somewhat reduced by the fact that it only
happens when adding values to an existing summary with allnulls=true,
not when the summarization is processing values in bulk (e.g. during
CREATE INDEX or automatic summarization). In this case the flags were
updated in a slightly different way, not forgetting the NULL values.
The best solution would be to introduce a new flag marking index tuples
representing ranges with no rows, but that would break on-disk format
and/or ABI, depending on where we put the flag. Considering we need to
backpatch this, that's not acceptable.
So instead we use an "impossible" combination of both flags (allnulls
and hasnulls) set to true, to mark "empty" ranges with no rows. In
principle "empty" is a feature of the whole index tuple, which may
contain multiple summaries in a multi-column index, but this is where
the flags are, unfortunately.
We could also skip storing index tuples for empty summaries, but then
we'd have to always process such ranges - even if there are no rows in
large parts of the table (e.g. after a bulk DELETE), it would still
require reading the pages etc. So we store them, but ignore them when
building the bitmap.
Backpatch to 11. The issue exists since BRIN indexes were introduced in
9.5, but older releases are already EOL.
Backpatch-through: 11
Reviewed-by: Justin Pryzby, Matthias van de Meent
Discussion: https://postgr.es/m/402430e4-7d9d-6cf1-09ef-464d80aff...@enterprisedb.com
---
src/backend/access/brin/brin.c | 337 +++++++++++++++++-
src/backend/access/brin/brin_inclusion.c | 46 +--
src/backend/access/brin/brin_minmax.c | 43 +--
src/backend/access/brin/brin_tuple.c | 31 +-
...summarization-and-inprogress-insertion.out | 8 +-
...ummarization-and-inprogress-insertion.spec | 1 +
6 files changed, 369 insertions(+), 97 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 0becfde1133..5ede5a88367 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"
@@ -68,6 +69,8 @@ typedef struct BrinOpaque
#define BRIN_ALL_BLOCKRANGES InvalidBlockNumber
+#define BRIN_RANGE_IS_EMPTY(col) ((col)->bv_allnulls && (col)->bv_hasnulls)
+
static BrinBuildState *initialize_brin_buildstate(Relation idxRel,
BrinRevmap *revmap, BlockNumber pagesPerRange);
static void terminate_brin_buildstate(BrinBuildState *state);
@@ -253,18 +256,94 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
Datum result;
BrinValues *bval;
FmgrInfo *addValue;
+ bool hasnulls;
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);
+
+ /*
+ * Does the range have actual NULL values? Either of the flags can
+ * be set, but we ignore the state before adding first row.
+ *
+ * We have to remember this, because we'll modify the flags and we
+ * need to know if the range started as empty.
+ */
+ hasnulls = (!BRIN_RANGE_IS_EMPTY(bval)) &&
+ (bval->bv_hasnulls || bval->bv_allnulls);
+
+ /*
+ * We need to consider whether the range is empty (not representing
+ * any rows yet), i.e. if it has both flags (allnulls hasnulls) set
+ * to true.
+ *
+ * If the range is empty, we clear the hasnulls flag - after adding
+ * a value it won't be empty anymore. Either it'll be all-NULL (and
+ * leaving allnulls=true covers that), or it will have no NULLs at
+ * all (but building the state is up to the opclass).
+ *
+ * If the range is not empty, we remember if there are NULL values.
+ * In this case both flags can't be set to true (that'd be empty
+ * range), so it's either allnulls=true or hasnulls=true. But the
+ * opclasses clear allnulls when adding the first non-NULL value,
+ * so we need to remember this.
+ *
+ * When adding a null value we can do everything locally, without
+ * calling BRIN_PROCNUM_ADDVALUE.
+ */
+ if (BRIN_RANGE_IS_EMPTY(bval))
+ {
+ bval->bv_hasnulls = false;
+ need_insert = true;
+ }
+
+ /*
+ * If the value we're adding is NULL, handle it locally. Otherwise
+ * call the BRIN_PROCNUM_ADDVALUE procedure.
+ */
+ if (nulls[keyno])
+ {
+ /*
+ * We can't check "bv_hasnulls" because then we might end up with
+ * both flags set to true, which is interpreted as empty range.
+ * But that'd be wrong, because we've just added a value.
+ *
+ * So either the range has allnulls=true, or we have to set the
+ * hasnulls flag. Check if we're changing the value to determine
+ * if the index tuple was modified.
+ */
+ if (!bval->bv_allnulls)
+ {
+ /* Are we changing the tuple? */
+ need_insert |= (!bval->bv_hasnulls);
+ bval->bv_hasnulls = true;
+ }
+ }
+ else
+ {
+ 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);
+ }
+
+ /*
+ * If the range was not an empty range (it'd have hasnulls=false),
+ * make sure we remember there were NULL values. Either the allnulls
+ * flag is still set to true, or we need to set the hasnulls flag.
+ */
+ if (hasnulls && !bval->bv_allnulls)
+ {
+ need_insert |= (!bval->bv_hasnulls);
+ bval->bv_hasnulls = true;
+ }
+
+ /* We've added a row, so the summary should not be empty. */
+ Assert(!BRIN_RANGE_IS_EMPTY(bval));
}
if (!need_insert)
@@ -508,6 +587,17 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
CurrentMemoryContext);
}
+ /*
+ * If the range has both allnulls and hasnulls set, it means
+ * there are no rows in the range, so we can skip it (we know
+ * there's nothing to match).
+ */
+ if (BRIN_RANGE_IS_EMPTY(bval))
+ {
+ addrange = false;
+ break;
+ }
+
/*
* Check whether the scan key is consistent with the page
* range values; if so, have the pages in the range added
@@ -645,19 +735,80 @@ brinbuildCallback(Relation index,
FmgrInfo *addValue;
BrinValues *col;
Form_pg_attribute attr = TupleDescAttr(state->bs_bdesc->bd_tupdesc, i);
+ bool hasnulls;
col = &state->bs_dtuple->bt_columns[i];
- addValue = index_getprocinfo(index, i + 1,
- BRIN_PROCNUM_ADDVALUE);
+
+ /*
+ * Does the range have actual NULL values? Either of the flags can
+ * be set, but we ignore the state before adding first row.
+ *
+ * We have to remember this, because we'll modify the flags and we
+ * need to know if the range started as empty.
+ */
+ hasnulls = (!BRIN_RANGE_IS_EMPTY(col)) &&
+ (col->bv_hasnulls || col->bv_allnulls);
+
+ /*
+ * We need to consider whether the range is empty (not representing
+ * any rows yet), i.e. if it has both flags (allnulls hasnulls) set
+ * to true.
+ *
+ * If the range is empty, we clear the hasnulls flag - after adding
+ * a value it won't be empty anymore. Either it'll be all-NULL (and
+ * leaving allnulls=true covers that), or it will have no NULLs at
+ * all (but building the state is up to the opclass).
+ *
+ * If the range is not empty, we remember if there are NULL values.
+ * In this case both flags can't be set to true (that'd be empty
+ * range), so it's either allnulls=true or hasnulls=true. But the
+ * opclasses clear allnulls when adding the first non-NULL value,
+ * so we need to remember this.
+ *
+ * When adding a null value we can do everything locally, without
+ * calling BRIN_PROCNUM_ADDVALUE.
+ */
+ if (BRIN_RANGE_IS_EMPTY(col))
+ col->bv_hasnulls = false;
+
+ if (isnull[i])
+ {
+ /*
+ * We can't check "bv_hasnulls" because then we might end up with
+ * both flags set to true, which is interpreted as empty range.
+ * But that'd be wrong, because we've just added a value.
+ *
+ * So either the range has allnulls=true, or we have to set the
+ * hasnulls flag.
+ */
+ if (!col->bv_allnulls)
+ col->bv_hasnulls = true;
+ }
+ else
+ {
+ 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]);
+ }
/*
- * Update dtuple state, if and as necessary.
+ * If the range was not an empty range (it'd have hasnulls=false),
+ * make sure we remember there were NULL values. Either the allnulls
+ * flag is still set to true, or we need to set the hasnulls flag.
*/
- FunctionCall4Coll(addValue,
- attr->attcollation,
- PointerGetDatum(state->bs_bdesc),
- PointerGetDatum(col),
- values[i], isnull[i]);
+ if (hasnulls && !col->bv_allnulls)
+ col->bv_hasnulls = true;
+
+ /* We've added a row, so the summary should not be empty. */
+ Assert(!BRIN_RANGE_IS_EMPTY(col));
}
}
@@ -1468,9 +1619,159 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b)
for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
{
FmgrInfo *unionFn;
+ BrinOpcInfo *opcinfo = bdesc->bd_info[keyno];
BrinValues *col_a = &a->bt_columns[keyno];
BrinValues *col_b = &db->bt_columns[keyno];
+ /*
+ * If B is empty (represents no rows), ignore it and just keep
+ * A as is (might be empty etc.).
+ */
+ if (BRIN_RANGE_IS_EMPTY(col_b))
+ continue;
+
+ /*
+ * Now we know B is not empty - it has either NULLs or data, or
+ * some combination of it. We need to merge it into A somehow.
+ *
+ * If A is empty, we simply copy all the flags and data from B.
+ */
+ if (BRIN_RANGE_IS_EMPTY(col_a))
+ {
+ int i;
+
+ col_a->bv_allnulls = col_b->bv_allnulls;
+ col_a->bv_hasnulls = col_b->bv_hasnulls;
+
+ /* If B has no data, we're done. */
+ if (col_b->bv_allnulls)
+ continue;
+
+ 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);
+ }
+
+ /*
+ * Both A and B are not empty, and we need to merge B into A.
+ * There are multiple combinations of allnulls/hasnulls flags.
+ * We've handled the "empty" case on either side above, so we
+ * can ignore those cases - which leaves 3 flag combinations
+ * on each side, so 9 combinations in total.
+ *
+ * A:all A:has B:all B:has
+ * true false true false - nothing to do
+ * true false false true - set A:has=true, copy from B
+ * true false false false - set A:has=true, copy from B
+ *
+ * false true true false - nothing to do
+ * false true false true - flags OK, call union proc
+ * false true false false - flags OK, call union proc
+ *
+ * false false true false - set A:has=true
+ * false false false true - set A:has=true, call union proc
+ * false false false false - flags OK, call union proc
+ */
+ if (col_a->bv_allnulls && col_b->bv_allnulls)
+ {
+ /* nothing to do - both sides are NULL-only */
+ continue;
+ }
+ else if (col_a->bv_allnulls && col_b->bv_hasnulls)
+ {
+ int i;
+ /*
+ * A is NULL-only, but B has some non-NULL values too. So the
+ * result has both NULLs and non-NULL values.
+ */
+ col_a->bv_allnulls = false;
+ col_a->bv_hasnulls = true;
+
+ /* copy data from B to A */
+ 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;
+ }
+ else if (col_a->bv_allnulls) /* B has no NULLs */
+ {
+ int i;
+
+ /*
+ * A is NULL-only, but B has some non-NULL values too. So the
+ * result has both NULLs and non-NULL values.
+ *
+ * XXX This is the same as the preceding branch, but I've left
+ * it here to keep the branches mapped 1:1 to the table of
+ * combinations.
+ */
+ col_a->bv_allnulls = false;
+ col_a->bv_hasnulls = true;
+
+ /* copy data from B to A */
+ 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;
+ }
+ else if (col_a->bv_hasnulls && col_b->bv_allnulls)
+ {
+ /* Nothing to do (flags are correct, no data to copy). */
+ continue;
+ }
+ else if (col_a->bv_hasnulls && col_b->bv_hasnulls)
+ {
+ /*
+ * Flags are correct, but both A and B have non-NULL values.
+ * So we have to call the support proc BRIN_PROCNUM_UNION
+ * (so no 'continue' here).
+ */
+ }
+ else if (col_a->bv_hasnulls) /* B has no NULLs */
+ {
+ /*
+ * B has no NULL values, so flags are OK. But both sides have
+ * some non-NULL values, so we have to call the support proc
+ * (so no 'continue' here).
+ *
+ * XXX Same as the preceding branch, but kept for 1:1 mapping.
+ */
+ }
+ else if (col_b->bv_allnulls) /* A has no NULLs */
+ {
+ /*
+ * Just update the hasnulls flag to remember B has NULL values
+ * and we're done (no data non-NULL values to copy/merge).
+ */
+ col_a->bv_hasnulls = true;
+ continue;
+ }
+ else if (col_b->bv_hasnulls) /* A has no NULLs */
+ {
+ /*
+ * Update the hasnulls flag to remember B has NULL values, but
+ * both sides have some non-NULL data so we needto call the
+ * BRIN_PROCNUM_UNION procedure (so no 'continue' here).
+ */
+ col_a->bv_hasnulls = true;
+ }
+ else
+ {
+ /*
+ * Neither side has any NULL values, both sides have non-NULL
+ * values, so we need to call the BRIN_PROCNUM_UNION proc (so
+ * no 'continue' here).
+ */
+ }
+
unionFn = index_getprocinfo(bdesc->bd_index, keyno + 1,
BRIN_PROCNUM_UNION);
FunctionCall3Coll(unionFn,
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 7e380d66ed5..f9217ca8254 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -147,18 +147,8 @@ 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);
- }
+ /* We're not passing NULL values to the opclass anymore. */
+ Assert(!isnull);
attno = column->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -517,36 +507,16 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
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();
+ /*
+ * All-null summaries are no longer passed to the union proc (this also
+ * implies the summaries are not empty).
+ */
+ Assert(!col_a->bv_allnulls);
+ Assert(!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 4b5d6a72135..f2748d2e267 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -75,18 +75,8 @@ 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);
- }
+ /* We're not passing NULL values to the opclass anymore. */
+ Assert(!isnull);
attno = column->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -250,33 +240,16 @@ brin_minmax_union(PG_FUNCTION_ARGS)
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();
+ /*
+ * All-null summaries are no longer passed to the union proc (this also
+ * implies the summaries are not empty).
+ */
+ Assert(!col_a->bv_allnulls);
+ Assert(!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/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c
index b3b453aed12..861dc76b7d3 100644
--- a/src/backend/access/brin/brin_tuple.c
+++ b/src/backend/access/brin/brin_tuple.c
@@ -394,7 +394,20 @@ brin_form_placeholder_tuple(BrinDesc *brdesc, BlockNumber blkno, Size *size)
*bitP |= bitmask;
}
- /* no need to set hasnulls */
+ /* set hasnulls true for all attributes */
+ for (keyno = 0; keyno < brdesc->bd_tupdesc->natts; keyno++)
+ {
+ if (bitmask != HIGHBIT)
+ bitmask <<= 1;
+ else
+ {
+ bitP += 1;
+ *bitP = 0x0;
+ bitmask = 1;
+ }
+
+ *bitP |= bitmask;
+ }
*size = len;
return rettuple;
@@ -493,8 +506,15 @@ brin_memtuple_initialize(BrinMemTuple *dtuple, BrinDesc *brdesc)
for (i = 0; i < brdesc->bd_tupdesc->natts; i++)
{
dtuple->bt_columns[i].bv_attno = i + 1;
+
+ /*
+ * Each memtuple starts as if it represents no rows, which is indicated
+ * by having bot allnulls and hasnulls set to true. We track this for
+ * all columns, because we don't have a flag for the whole memtuple.
+ */
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;
currdatum += sizeof(Datum) * brdesc->bd_info[i]->oi_nstored;
}
@@ -557,6 +577,13 @@ brin_deform_tuple(BrinDesc *brdesc, BrinTuple *tuple, BrinMemTuple *dMemtuple)
{
int i;
+ /*
+ * Make sure to overwrite the hasnulls flag, because it was initialized
+ * to true by brin_memtuple_initialize and we don't want to skip it if
+ * allnulls=true.
+ */
+ dtup->bt_columns[keyno].bv_hasnulls = hasnulls[keyno];
+
if (allnulls[keyno])
{
valueno += brdesc->bd_info[keyno]->oi_nstored;
diff --git a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out
index 2a4755d0998..584ac2602f7 100644
--- a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out
+++ b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out
@@ -4,7 +4,7 @@ starting permutation: s2check s1b s2b s1i s2summ s1c s2c s2check
step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value
----------+------+------+--------+--------+-----------+--------
- 1| 0| 1|f |f |f |{1 .. 1}
+ 1| 0| 1|f |t |f |{1 .. 1}
(1 row)
step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
@@ -26,7 +26,7 @@ step s2c: COMMIT;
step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value
----------+------+------+--------+--------+-----------+-----------
- 1| 0| 1|f |f |f |{1 .. 1}
+ 1| 0| 1|f |t |f |{1 .. 1}
2| 1| 1|f |f |f |{1 .. 1000}
(2 rows)
@@ -35,7 +35,7 @@ starting permutation: s2check s1b s1i s2vacuum s1c s2check
step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value
----------+------+------+--------+--------+-----------+--------
- 1| 0| 1|f |f |f |{1 .. 1}
+ 1| 0| 1|f |t |f |{1 .. 1}
(1 row)
step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
@@ -45,7 +45,7 @@ step s1c: COMMIT;
step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value
----------+------+------+--------+--------+-----------+-----------
- 1| 0| 1|f |f |f |{1 .. 1}
+ 1| 0| 1|f |t |f |{1 .. 1}
2| 1| 1|f |f |f |{1 .. 1000}
(2 rows)
diff --git a/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec b/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec
index 19ac18a2e88..18ba92b7ba1 100644
--- a/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec
+++ b/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec
@@ -9,6 +9,7 @@ setup
) WITH (fillfactor=10);
CREATE INDEX brinidx ON brin_iso USING brin (value) WITH (pages_per_range=1);
-- this fills the first page
+ INSERT INTO brin_iso VALUES (NULL);
DO $$
DECLARE curtid tid;
BEGIN
--
2.39.2
