Re: Missing update of all_hasnulls in BRIN opclasses
On 5/18/23 20:45, Tomas Vondra wrote: > ... > > 0001 fixes the issue. 0002 is the original fix, and 0003 is just the > pageinspect changes (for master only). > > For the backbranches, I thought about making the code more like master > (by moving some of the handling from opclasses to brin.c), but decided > not to. It'd be low-risk, but it feels wrong to kinda do what the master > does under "oi_regular_nulls" flag. > I've now pushed all these patches into relevant branches, after some minor last-minute tweaks, and so far it didn't cause any buildfarm issues. Assuming this fully fixes the NULL-handling for BRIN, this leaves just the deadlock issue discussed in [1]. It seems rather unfortunate all these issues went unnoticed / unreported essentially since BRIN was introduced in 9.5. To some extent it might be explained by fairly low likelihood of actually hitting the issue (just the right timing, concurrency with summarization, NULL values, ...). It took me quite a bit of time and luck to (accidentally) hit these issues while stress testing the code. But there's also the problem of writing tests for this kind of thing. To exercise the interesting parts (e.g. the union_tuples), it's necessary to coordinate the order of concurrent steps - but what's a good generic way to do that (which we could do in TAP tests)? In manual testing it's doable by setting breakpoints on a particular lines, and step through the concurrent processes that way. But that doesn't seem like a particularly great solution for regression tests. I can imagine adding some sort of "probes" into the code and then attaching breakpoints to those, but surely we're not the first project needing this ... regards [1] https://www.postgresql.org/message-id/261e68bc-f5f5-5234-fb2c-af4f58351...@enterprisedb.com -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Missing update of all_hasnulls in BRIN opclasses
On 5/15/23 12:06, Alvaro Herrera wrote:
> On 2023-May-07, Tomas Vondra wrote:
>
>>> Álvaro wrote:
In backbranches, the new field to BrinMemTuple needs to be at the end of
the struct, to avoid ABI breakage.
>>
>> Unfortunately, this is not actually possible :-(
>>
>> The BrinMemTuple has a FLEXIBLE_ARRAY_MEMBER at the end, so we can't
>> place anything after it. I think we have three options:
>>
>> a) some other approach? - I really can't see any, except maybe for going
>> back to the previous approach (i.e. encoding the info using the existing
>> BrinValues allnulls/hasnulls flags)
>
> Actually, mine was quite the stupid suggestion: the BrinMemTuple already
> has a 3 byte hole in the place where you originally wanted to add the
> flag:
>
> struct BrinMemTuple {
> _Bool bt_placeholder; /* 0 1 */
>
> /* XXX 3 bytes hole, try to pack */
>
> BlockNumberbt_blkno; /* 4 4 */
> MemoryContext bt_context; /* 8 8 */
> Datum *bt_values;/*16 8 */
> _Bool *bt_allnulls; /*24 8 */
> _Bool *bt_hasnulls; /*32 8 */
> BrinValues bt_columns[]; /*40 0 */
>
> /* size: 40, cachelines: 1, members: 7 */
> /* sum members: 37, holes: 1, sum holes: 3 */
> /* last cacheline: 40 bytes */
> };
>
> so putting it there was already not causing any ABI breakage. So, the
> solution to this problem of not being able to put it at the end is just
> to return the struct to your original formulation.
>
Thanks, that's pretty lucky. It means we're not breaking on-disk format
nor ABI, which is great.
Attached is a final version of the patches - I intend to do a bit more
testing, go through the comments once more, and get this committed today
or perhaps tomorrow morning, so that it gets into beta1.
Unfortunately, while polishing the patches, I realized union_tuples()
has yet another long-standing bug with handling NULL values, because it
does this:
/* Adjust "hasnulls". */
if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
col_a->bv_hasnulls = true;
but let's assume "col_a" is a summary representing "1" and "col_b"
represents NULL (col_b->bv_hasnulls=false col_b->bv_allnulls=true).
Well, in that case we fail to "remember" col_a should represent NULL
values too :-(
This is somewhat separate issue, because it's unrelated to empty ranges
(neither of the two ranges is empty). It's hard to test it, because it
requires a particular timing of the concurrent actions, but a breakpoint
in brin.c on the brin_can_do_samepage_update call (in summarize_range)
does the trick for manual testing.
0001 fixes the issue. 0002 is the original fix, and 0003 is just the
pageinspect changes (for master only).
For the backbranches, I thought about making the code more like master
(by moving some of the handling from opclasses to brin.c), but decided
not to. It'd be low-risk, but it feels wrong to kinda do what the master
does under "oi_regular_nulls" flag.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 698609154324f64831ac44e4440a2283691184e2 Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Thu, 18 May 2023 13:00:31 +0200
Subject: [PATCH 1/3] Fix handling of NULLs when merging BRIN summaries
When merging BRIN summaries, union_tuples() did not correctly update the
target hasnulls/allnulls flags. When merging all-NULL summary into a
summary without any NULL values, the result had both flags set to false
(instead of having hasnulls=true).
This happened because the code only considered the hasnulls flags,
ignoring the possibility the source summary has allnulls=true.
Discovered while investigating issues with handling empty BRIN ranges
and handling of NULL values, but it's a separate problem (has nothing to
do with empty ranges).
Fixed by considering both flags on the source summary, and updating the
hasnulls flag on the target summary.
Backpatch to 11. The bug exists since 9.5 (where BRIN indexes were
introduced), but those releases are EOL already.
Discussion: https://postgr.es/m/402430e4-7d9d-6cf1-09ef-464d80afff3b%40enterprisedb.com
---
src/backend/access/brin/brin.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 41bf950a4af..a155525b7df 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1613,10 +1613,13 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b)
BrinValues *col_b = >bt_columns[keyno];
BrinOpcInfo *opcinfo = bdesc->bd_info[keyno];
if (opcinfo->oi_regular_nulls)
{
+ /* Does the "b" summary represent any NULL values? */
+ bool b_has_nulls = (col_b->bv_hasnulls || col_b->bv_allnulls);
+
/* Adjust "hasnulls".
Re: Missing update of all_hasnulls in BRIN opclasses
On 2023-May-07, Tomas Vondra wrote:
> > Álvaro wrote:
> >> In backbranches, the new field to BrinMemTuple needs to be at the end of
> >> the struct, to avoid ABI breakage.
>
> Unfortunately, this is not actually possible :-(
>
> The BrinMemTuple has a FLEXIBLE_ARRAY_MEMBER at the end, so we can't
> place anything after it. I think we have three options:
>
> a) some other approach? - I really can't see any, except maybe for going
> back to the previous approach (i.e. encoding the info using the existing
> BrinValues allnulls/hasnulls flags)
Actually, mine was quite the stupid suggestion: the BrinMemTuple already
has a 3 byte hole in the place where you originally wanted to add the
flag:
struct BrinMemTuple {
_Bool bt_placeholder; /* 0 1 */
/* XXX 3 bytes hole, try to pack */
BlockNumberbt_blkno; /* 4 4 */
MemoryContext bt_context; /* 8 8 */
Datum *bt_values;/*16 8 */
_Bool *bt_allnulls; /*24 8 */
_Bool *bt_hasnulls; /*32 8 */
BrinValues bt_columns[]; /*40 0 */
/* size: 40, cachelines: 1, members: 7 */
/* sum members: 37, holes: 1, sum holes: 3 */
/* last cacheline: 40 bytes */
};
so putting it there was already not causing any ABI breakage. So, the
solution to this problem of not being able to put it at the end is just
to return the struct to your original formulation.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)
Re: Missing update of all_hasnulls in BRIN opclasses
On 5/7/23 07:08, Julien Rouhaud wrote: > Hi, > > On Sun, May 07, 2023 at 12:13:07AM +0200, Tomas Vondra wrote: >> >> c) ignore the issue - AFAICS this would be an issue only for (external) >> code accessing BrinMemTuple structs, but I don't think we're aware of >> any out-of-core BRIN opclasses or anything like that ... > > FTR there's at least postgis that implments BRIN opclasses (for geometries and > geographies), but there's nothing fancy in the implementation and it doesn't > access BrinMemTuple struct. Right. I believe that should be fine, because opclasses don't access the tuple directly - instead we pass pointers to individual pieces. But maybe it'd be a good idea to test this. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Missing update of all_hasnulls in BRIN opclasses
Hi, On Sun, May 07, 2023 at 12:13:07AM +0200, Tomas Vondra wrote: > > c) ignore the issue - AFAICS this would be an issue only for (external) > code accessing BrinMemTuple structs, but I don't think we're aware of > any out-of-core BRIN opclasses or anything like that ... FTR there's at least postgis that implments BRIN opclasses (for geometries and geographies), but there's nothing fancy in the implementation and it doesn't access BrinMemTuple struct.
Re: Missing update of all_hasnulls in BRIN opclasses
On 4/24/23 23:20, Tomas Vondra wrote: > On 4/24/23 17:36, Alvaro Herrera wrote: >> On 2023-Apr-23, Tomas Vondra wrote: >> >>> here's an updated version of the patch, including a backport version. I >>> ended up making the code yet a bit closer to master by introducing >>> add_values_to_range(). The current pre-14 code has the loop adding data >>> to the BRIN tuple in two places, but with the "fixed" logic handling >>> NULLs and the empty_range flag the amount of duplicated code got too >>> high, so this seem reasonable. >> >> In backbranches, the new field to BrinMemTuple needs to be at the end of >> the struct, to avoid ABI breakage. >> Unfortunately, this is not actually possible :-( The BrinMemTuple has a FLEXIBLE_ARRAY_MEMBER at the end, so we can't place anything after it. I think we have three options: a) some other approach? - I really can't see any, except maybe for going back to the previous approach (i.e. encoding the info using the existing BrinValues allnulls/hasnulls flags) b) encoding the info in existing BrinMemTuple flags - e.g. we could use bt_placeholder to store two bits, not just one. Seems a bit ugly. c) ignore the issue - AFAICS this would be an issue only for (external) code accessing BrinMemTuple structs, but I don't think we're aware of any out-of-core BRIN opclasses or anything like that ... regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Missing update of all_hasnulls in BRIN opclasses
On 2023-Apr-24, Tomas Vondra wrote: > On 4/24/23 17:36, Alvaro Herrera wrote: > > (As for your FIXME comment in brin_memtuple_initialize, I think you're > > correct: we definitely need to reset bt_placeholder. Otherwise, we risk > > places that call it in a loop using a BrinMemTuple with one range with > > the flag set, in a range where that doesn't hold. It might be > > impossible for this to happen, given how narrow the conditions are on > > which bt_placeholder is used; but it seems safer to reset it anyway.) > > Yeah. But isn't that a separate preexisting issue, strictly speaking? Yes. > > I did a quick experiment of turning the patches over -- applying test > > first, code fix after (requires some conflict fixing). With that I > > verified that the behavior of 'hasnulls' indeed changes with the code > > fix. > > Thanks. Could you do some testing of the union_tuples stuff too? It's a > bit tricky part - the behavior is timing sensitive, so testing it > requires gdb breakpoints breakpoints or something like that. I think > it's correct, but it'd be nice to check that. I'll have a look. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ really, I see PHP as like a strange amalgamation of C, Perl, Shell inflex: you know that "amalgam" means "mixture with mercury", more or less, right? i.e., "deadly poison"
Re: Missing update of all_hasnulls in BRIN opclasses
On 4/24/23 17:36, Alvaro Herrera wrote: > On 2023-Apr-23, Tomas Vondra wrote: > >> here's an updated version of the patch, including a backport version. I >> ended up making the code yet a bit closer to master by introducing >> add_values_to_range(). The current pre-14 code has the loop adding data >> to the BRIN tuple in two places, but with the "fixed" logic handling >> NULLs and the empty_range flag the amount of duplicated code got too >> high, so this seem reasonable. > > In backbranches, the new field to BrinMemTuple needs to be at the end of > the struct, to avoid ABI breakage. > Good point. > There's a comment in add_values_to_range with a typo "If the range was had". > Also, "So we should not see empty range that was not modified" should > perhaps be "should not see an empty range". > OK, I'll check the wording of the comments. > (As for your FIXME comment in brin_memtuple_initialize, I think you're > correct: we definitely need to reset bt_placeholder. Otherwise, we risk > places that call it in a loop using a BrinMemTuple with one range with > the flag set, in a range where that doesn't hold. It might be > impossible for this to happen, given how narrow the conditions are on > which bt_placeholder is used; but it seems safer to reset it anyway.) > Yeah. But isn't that a separate preexisting issue, strictly speaking? > Some pgindent noise would be induced by this patch. I think it's worth > cleaning up ahead of time. > True. Will do. > I did a quick experiment of turning the patches over -- applying test > first, code fix after (requires some conflict fixing). With that I > verified that the behavior of 'hasnulls' indeed changes with the code > fix. > Thanks. Could you do some testing of the union_tuples stuff too? It's a bit tricky part - the behavior is timing sensitive, so testing it requires gdb breakpoints breakpoints or something like that. I think it's correct, but it'd be nice to check that. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Missing update of all_hasnulls in BRIN opclasses
On 4/24/23 23:05, Tomas Vondra wrote: > > > On 4/24/23 17:46, Tom Lane wrote: >> Alvaro Herrera writes: >>> On 2023-Apr-23, Tomas Vondra wrote: Both cases have a patch extending pageinspect to show the new flag, but obviously we should commit that only in master. In backbranches it's meant only to make testing easier. >> >>> In backbranches, I think it should be reasonably easy to add a >>> --1.7--1.7.1.sql file and set the default version to 1.7.1; that then >>> enables us to have the functionality (and the tests) in older branches >>> too. If you just add a --1.X.1--1.12.sql version to each branch that's >>> identical to that branch's current pageinspect version upgrade script, >>> that would let us have working upgrade paths for all branches. This is >>> a bit laborious but straightforward enough. >> >> "A bit laborious"? That seems enormously out of proportion to the >> benefit of putting this test case into back branches. It will have >> costs for end users too, not only us. As an example, it would >> unecessarily block some upgrade paths, if the upgraded-to installation >> is slightly older and lacks the necessary --1.X.1--1.12 script. >> > > Why would that block the upgrade? Presumably we'd add two upgrade > scripts in the master, to allow upgrade both from 1.X and 1.X.1. > D'oh! I just realized I misunderstood the issue. Yes, if the target version is missing the new script, that won't work. I'm not sure how likely that is - in my experience people refresh versions at the same time - but it's certainly an assumption we shouldn't do, I guess. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Missing update of all_hasnulls in BRIN opclasses
Tomas Vondra writes: > On 4/24/23 17:46, Tom Lane wrote: >> "A bit laborious"? That seems enormously out of proportion to the >> benefit of putting this test case into back branches. It will have >> costs for end users too, not only us. As an example, it would >> unecessarily block some upgrade paths, if the upgraded-to installation >> is slightly older and lacks the necessary --1.X.1--1.12 script. > Why would that block the upgrade? Presumably we'd add two upgrade > scripts in the master, to allow upgrade both from 1.X and 1.X.1. It would for example block updating from 14.8 or later to 15.2, since a 15.2 installation would not have the script to update from 1.X.1. Yeah, people could work around that by only installing the latest version, but there are plenty of real-world scenarios where you'd be creating friction, or at least confusion. I do not think that this test case is worth it. regards, tom lane
Re: Missing update of all_hasnulls in BRIN opclasses
On 4/24/23 17:46, Tom Lane wrote: > Alvaro Herrera writes: >> On 2023-Apr-23, Tomas Vondra wrote: >>> Both cases have a patch extending pageinspect to show the new flag, but >>> obviously we should commit that only in master. In backbranches it's >>> meant only to make testing easier. > >> In backbranches, I think it should be reasonably easy to add a >> --1.7--1.7.1.sql file and set the default version to 1.7.1; that then >> enables us to have the functionality (and the tests) in older branches >> too. If you just add a --1.X.1--1.12.sql version to each branch that's >> identical to that branch's current pageinspect version upgrade script, >> that would let us have working upgrade paths for all branches. This is >> a bit laborious but straightforward enough. > > "A bit laborious"? That seems enormously out of proportion to the > benefit of putting this test case into back branches. It will have > costs for end users too, not only us. As an example, it would > unecessarily block some upgrade paths, if the upgraded-to installation > is slightly older and lacks the necessary --1.X.1--1.12 script. > Why would that block the upgrade? Presumably we'd add two upgrade scripts in the master, to allow upgrade both from 1.X and 1.X.1. >> If you don't feel like adding that, I volunteer to add the necessary >> scripts to all branches after you commit the bugfix, and ensure that all >> upgrade paths work correctly. > > I do not think this should happen at all, whether you're willing to > do the work or not. FWIW I'm fine with not doing that. As mentioned, I only included this patch to make testing the patch easier (otherwise the flag is impossible to inspect directly). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Missing update of all_hasnulls in BRIN opclasses
Alvaro Herrera writes: > On 2023-Apr-23, Tomas Vondra wrote: >> Both cases have a patch extending pageinspect to show the new flag, but >> obviously we should commit that only in master. In backbranches it's >> meant only to make testing easier. > In backbranches, I think it should be reasonably easy to add a > --1.7--1.7.1.sql file and set the default version to 1.7.1; that then > enables us to have the functionality (and the tests) in older branches > too. If you just add a --1.X.1--1.12.sql version to each branch that's > identical to that branch's current pageinspect version upgrade script, > that would let us have working upgrade paths for all branches. This is > a bit laborious but straightforward enough. "A bit laborious"? That seems enormously out of proportion to the benefit of putting this test case into back branches. It will have costs for end users too, not only us. As an example, it would unecessarily block some upgrade paths, if the upgraded-to installation is slightly older and lacks the necessary --1.X.1--1.12 script. > If you don't feel like adding that, I volunteer to add the necessary > scripts to all branches after you commit the bugfix, and ensure that all > upgrade paths work correctly. I do not think this should happen at all, whether you're willing to do the work or not. regards, tom lane
Re: Missing update of all_hasnulls in BRIN opclasses
On 2023-Apr-23, Tomas Vondra wrote: > here's an updated version of the patch, including a backport version. I > ended up making the code yet a bit closer to master by introducing > add_values_to_range(). The current pre-14 code has the loop adding data > to the BRIN tuple in two places, but with the "fixed" logic handling > NULLs and the empty_range flag the amount of duplicated code got too > high, so this seem reasonable. In backbranches, the new field to BrinMemTuple needs to be at the end of the struct, to avoid ABI breakage. There's a comment in add_values_to_range with a typo "If the range was had". Also, "So we should not see empty range that was not modified" should perhaps be "should not see an empty range". (As for your FIXME comment in brin_memtuple_initialize, I think you're correct: we definitely need to reset bt_placeholder. Otherwise, we risk places that call it in a loop using a BrinMemTuple with one range with the flag set, in a range where that doesn't hold. It might be impossible for this to happen, given how narrow the conditions are on which bt_placeholder is used; but it seems safer to reset it anyway.) Some pgindent noise would be induced by this patch. I think it's worth cleaning up ahead of time. I did a quick experiment of turning the patches over -- applying test first, code fix after (requires some conflict fixing). With that I verified that the behavior of 'hasnulls' indeed changes with the code fix. > Both cases have a patch extending pageinspect to show the new flag, but > obviously we should commit that only in master. In backbranches it's > meant only to make testing easier. In backbranches, I think it should be reasonably easy to add a --1.7--1.7.1.sql file and set the default version to 1.7.1; that then enables us to have the functionality (and the tests) in older branches too. If you just add a --1.X.1--1.12.sql version to each branch that's identical to that branch's current pageinspect version upgrade script, that would let us have working upgrade paths for all branches. This is a bit laborious but straightforward enough. If you don't feel like adding that, I volunteer to add the necessary scripts to all branches after you commit the bugfix, and ensure that all upgrade paths work correctly. > I plan to do a bit more testing, I'd welcome some feedback - it's a > long-standing bug, and it'd be good to finally get this fixed. I don't > think the patch can be made any simpler. The approach looks good to me. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Oh, great altar of passive entertainment, bestow upon me thy discordant images at such speed as to render linear thought impossible" (Calvin a la TV)
Re: Missing update of all_hasnulls in BRIN opclasses
Hi,
here's an updated version of the patch, including a backport version. I
ended up making the code yet a bit closer to master by introducing
add_values_to_range(). The current pre-14 code has the loop adding data
to the BRIN tuple in two places, but with the "fixed" logic handling
NULLs and the empty_range flag the amount of duplicated code got too
high, so this seem reasonable.
Both cases have a patch extending pageinspect to show the new flag, but
obviously we should commit that only in master. In backbranches it's
meant only to make testing easier.
I plan to do a bit more testing, I'd welcome some feedback - it's a
long-standing bug, and it'd be good to finally get this fixed. I don't
think the patch can be made any simpler.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 42603e32bd0ad456a53a96ac2d05ce714f97e1ba Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Sun, 23 Apr 2023 19:26:18 +0200
Subject: [PATCH 1/2] 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| 226 ++
src/backend/access/brin/brin_tuple.c | 15 +-
src/include/access/brin_tuple.h | 6 +-
...summarization-and-inprogress-insertion.out | 8 +-
...ummarization-and-inprogress-insertion.spec | 1 +
5 files changed, 201 insertions(+), 55 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 0becfde1133..f99a9ba5cf9 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,8 @@ 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);
/*
* BRIN handler function: return IndexAmRoutine with access method parameters
@@ -173,11 +175,10 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
for (;;)
{
- bool need_insert = false;
+ bool need_insert;
OffsetNumber off;
BrinTuple *brtup;
BrinMemTuple *dtup;
- int keyno;
CHECK_FOR_INTERRUPTS();
@@ -241,31 +242,7 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
dtup =
Re: Missing update of all_hasnulls in BRIN opclasses
Hi,
It took me a while but I finally got back to reworking this to use the
bt_info bit, as proposed by Alvaro. And it turned out to work great,
because (a) it's a tuple-level flag, i.e. the right place, and (b) it
does not overload existing flags.
This greatly simplified the code in add_values_to_range and (especially)
union_tuples, making it much easier to understand, I think.
One disadvantage is we are unable to see which ranges are empty in
current pageinspect, but 0002 addresses that by adding "empty" column to
the brin_page_items() output. That's a matter for master only, though.
It's a trivial patch and it makes it easier/possible to test this, so we
should consider to squeeze it into PG16.
I did quite a bit of testing - the attached 0003 adds extra tests, but I
don't propose to get this committed as is - it's rather overkill. Maybe
some reduced version of it ...
The hardest thing to test is the union_tuples() part, as it requires
concurrent operations with "correct" timing. Easy to simulate by
breakpoints in GDB, not so much in plain regression/TAP tests.
There's also a stress tests, doing a lot of randomized summarizations,
etc. Without the fix this failed in maybe 30% of runs, now I did ~100
runs without a single failure.
I haven't done any backporting, but I think it should be simpler than
with the earlier approach. I wonder if we need to care about starting to
use the previously unused bit - I don't think so, in the worst case
we'll just ignore it, but maybe I'm missing something (e.g. when using
physical replication).
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 10efaf9964806e5a30818994e3cfda879bb90171 Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Sun, 8 Jan 2023 16:43:06 +0100
Subject: [PATCH 1/3] 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.
This introduces a new a new flag marking index tuples representing
ranges with no rows. Luckily we have an unused tuple in the BRIN tuple
header that we can use for this.
We still store index tuples for empty ranges, because otherwise we'd not
be able to say whether a range is empty or not yet summarized, and we'd
have to process them for any query.
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: Alvaro Herrera, Justin Pryzby, Matthias van de Meent
Discussion: https://postgr.es/m/402430e4-7d9d-6cf1-09ef-464d80aff...@enterprisedb.com
---
src/backend/access/brin/brin.c| 115 +-
src/backend/access/brin/brin_tuple.c | 15 ++-
src/include/access/brin_tuple.h | 6 +-
...summarization-and-inprogress-insertion.out | 8 +-
...ummarization-and-inprogress-insertion.spec | 1 +
5 files changed, 137 insertions(+), 8 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 53e4721a54e..162a0c052aa 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -592,6 +592,17 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
bval = >bt_columns[attno - 1];
+ /*
+ * If the BRIN tuple indicates that this range is empty,
+ * we can skip it: there's nothing to match. We don't
+ * need to examine the next columns.
+ */
+ if (dtup->bt_empty_range)
+ {
+ 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
@@ -1590,6 +1601,8 @@ form_and_insert_tuple(BrinBuildState *state)
/*
* Given two deformed tuples, adjust the first one so that it's consistent
* with the summary values in
Re: Missing update of all_hasnulls in BRIN opclasses
On 3/3/23 11:32, Alvaro Herrera wrote:
>
> Thanks for doing all this. (Do I understand correctly that this patch
> is not in the commitfest?)
>
> I think my mental model for this was that "allnulls" meant that either
> there are no values for the column in question or that the values were
> all nulls (For minmax without NULL handling, which is where this all
> started, these two things are essentially the same: the range is not to
> be returned. So this became a bug the instant I added handling for NULL
> values.) I failed to realize that these were two different things, and
> this is likely the origin of all these troubles.
>
> What do you think of using the unused bit in BrinTuple->bt_info to
> denote a range that contains no heap tuples? This also means we need it
> in BrinMemTuple, I think we can do this:
>
> @@ -44,6 +44,7 @@ typedef struct BrinValues
> typedef struct BrinMemTuple
> {
> boolbt_placeholder; /* this is a placeholder tuple */
> + boolbt_empty_range; /* range has no tuples */
> BlockNumber bt_blkno; /* heap blkno that the tuple is for */
> MemoryContext bt_context; /* memcxt holding the bt_columns values
> */
> /* output arrays for brin_deform_tuple: */
> @@ -69,7 +70,7 @@ typedef struct BrinTuple
>*
>* 7th (high) bit: has nulls
>* 6th bit: is placeholder tuple
> - * 5th bit: unused
> + * 5th bit: range has no tuples
>* 4-0 bit: offset of data
>* ---
>*/
> @@ -82,7 +83,7 @@ typedef struct BrinTuple
> * bt_info manipulation macros
> */
> #define BRIN_OFFSET_MASK 0x1F
> -/* bit 0x20 is not used at present */
> +#define BRIN_EMPTY_RANGE 0x20
> #define BRIN_PLACEHOLDER_MASK0x40
> #define BRIN_NULLS_MASK 0x80
>
> (Note that bt_empty_range uses a hole in the struct, so there's no ABI
> change.)
>
> This is BRIN-tuple-level, not column-level, so conceptually it seems
> more appropriate. (In the case where both are empty in union_tuples, we
> can return without entering the per-attribute loop at all, though I
> admit it's not a very interesting case.) This approach avoids having to
> invent the strange combination of all+has to mean empty.
>
Oh, that's an interesting idea! I haven't realized there's an unused bit
at the tuple level, and I absolutely agree it'd be a better match than
having this in individual summaries (like now).
It'd mean we'd not have the option to fix this withing the opclasses,
because we only pass them the BrinValue and not the tuple. But if you
think that's reasonable, that'd be OK.
The other thing I was unsure is if the bit could be set for any existing
tuples, but AFAICS that shouldn't be possible - brin_form_tuple does
palloc0, so it should be 0.
I suspect doing this might make the patch quite a bit simpler, actually.
>
> On 2023-Feb-24, Tomas Vondra wrote:
>
>> 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.
>
> Yeah, I struggled with this during initial development but in the end
> did nothing. I think we would need to introduce some new framework,
> perhaps Korotkov stop-events stuff at
> https://postgr.es/m/CAPpHfdsTeb+hBT5=qxghjng_chcjldanq9sdy9vnwbf2e2p...@mail.gmail.com
> which seemed to me a good fit -- we would add a stop point after the
> placeholder tuple is inserted.
>
Yeah, but we don't have that at the moment. I actually ended up adding a
couple sleeps during development, which allowed me to hit just the right
order of operations - a poor-man's version of those stop-events. Did
work but hardly an acceptable approach.
>> 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.
>
> I think it's okay. I tried to make it more compact (by saying "these
> two combinations here are case 2, and these two other are case 4", and
> keeping each of the other combinations a separate case; so there are
> really 7 cases). But that doesn't make it any easier to follow, on the
> contrary it was more convoluted. I think a dozen extra lines of source
> is not a problem.
>
OK
>> The alternative is to apply the same fix to every BRIN_PROCNUM_UNION
>>
Re: Missing update of all_hasnulls in BRIN opclasses
Thanks for doing all this. (Do I understand correctly that this patch
is not in the commitfest?)
I think my mental model for this was that "allnulls" meant that either
there are no values for the column in question or that the values were
all nulls (For minmax without NULL handling, which is where this all
started, these two things are essentially the same: the range is not to
be returned. So this became a bug the instant I added handling for NULL
values.) I failed to realize that these were two different things, and
this is likely the origin of all these troubles.
What do you think of using the unused bit in BrinTuple->bt_info to
denote a range that contains no heap tuples? This also means we need it
in BrinMemTuple, I think we can do this:
@@ -44,6 +44,7 @@ typedef struct BrinValues
typedef struct BrinMemTuple
{
boolbt_placeholder; /* this is a placeholder tuple */
+ boolbt_empty_range; /* range has no tuples */
BlockNumber bt_blkno; /* heap blkno that the tuple is for */
MemoryContext bt_context; /* memcxt holding the bt_columns values
*/
/* output arrays for brin_deform_tuple: */
@@ -69,7 +70,7 @@ typedef struct BrinTuple
*
* 7th (high) bit: has nulls
* 6th bit: is placeholder tuple
-* 5th bit: unused
+* 5th bit: range has no tuples
* 4-0 bit: offset of data
* ---
*/
@@ -82,7 +83,7 @@ typedef struct BrinTuple
* bt_info manipulation macros
*/
#define BRIN_OFFSET_MASK 0x1F
-/* bit 0x20 is not used at present */
+#define BRIN_EMPTY_RANGE 0x20
#define BRIN_PLACEHOLDER_MASK 0x40
#define BRIN_NULLS_MASK0x80
(Note that bt_empty_range uses a hole in the struct, so there's no ABI
change.)
This is BRIN-tuple-level, not column-level, so conceptually it seems
more appropriate. (In the case where both are empty in union_tuples, we
can return without entering the per-attribute loop at all, though I
admit it's not a very interesting case.) This approach avoids having to
invent the strange combination of all+has to mean empty.
On 2023-Feb-24, Tomas Vondra wrote:
> 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.
Yeah, I struggled with this during initial development but in the end
did nothing. I think we would need to introduce some new framework,
perhaps Korotkov stop-events stuff at
https://postgr.es/m/CAPpHfdsTeb+hBT5=qxghjng_chcjldanq9sdy9vnwbf2e2p...@mail.gmail.com
which seemed to me a good fit -- we would add a stop point after the
placeholder tuple is inserted.
> 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.
I think it's okay. I tried to make it more compact (by saying "these
two combinations here are case 2, and these two other are case 4", and
keeping each of the other combinations a separate case; so there are
really 7 cases). But that doesn't make it any easier to follow, on the
contrary it was more convoluted. I think a dozen extra lines of source
is not a problem.
> 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).
About the hypothetical question, you could as well ask what about
unicorns. I have never seen any hint that any external opclass exist.
I am all for maintaining compatibility, but I think this concern is
overblown for BRIN. Anyway, I think your proposed fix is better than
changing individual 'union' support procs, so it doesn't matter.
As far as I understood, you're now worried that there will be an
incompatibility because we will fail to call the 'union' procedure in
cases where we previously called it? In other words, you fear that some
hypothetical opclass was handling the NULL values in some way that's
incompatible with this? I haven't thought terribly hard about this, but
I can't see a
Re: Missing update of all_hasnulls in BRIN opclasses
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 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
Re: Missing update of all_hasnulls in BRIN opclasses
Thanks Justin! I've applied all the fixes you proposed, and (hopefully)
improved a couple other comments.
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.
There's also a separate patch with pageinspect tests, but only as a
demonstration of various (non)broken cases, not for commit. And then
also a bash script generating indexes with random data, randomized
summarization etc. - on unpatched systems this happens to fail in about
1/3 of the runs (at least for me). I haven't seen any failures with the
patches attached (on any branch).
As for the issue / fix, I don't think there's a better solution than
what the patch does - we need to distinguish empty / all-nulls ranges,
but we can't add a flag because of on-disk format / ABI. So using the
existing flags seems like the only option - I haven't heard any other
ideas so far, and I couldn't come up with any myself either.
I've also thought about alternative "encodings" into allnulls/hasnulls,
instead of treating (true,true) as "empty" - but none of that ended up
being any simpler, quite the opposite actually, as it would change what
the individual flags mean etc. So AFAICS this is the best / least
disruptive option.
I went over all the places touching these flags, to double check if any
of those needs some tweaks (similar to union_tuples, which I missed for
a long time). But I haven't found anything else, so I think this version
of the patches is complete.
As for assessing how many indexes are affected - in principle, any index
on columns with NULLs may be broken. But it only matters if the index is
used for IS NULL queries, other queries are not affected.
I also realized that this only affects insertion of individual tuples
into existing all-null summaries, not "bulk" summarization that sees all
values at once. This happens because in this case add_values_to_range
sets hasnulls=true for the first (NULL) value, and then calls the
addValue procedure for the second (non-NULL) one, which resets the
allnulls flag to false.
But when inserting individual rows, we first set hasnulls=true, but
brin_form_tuple ignores that because of allnulls=true. And then when
inserting the second row, we start with hasnulls=false again, and the
opclass quietly resets the allnulls flag.
I guess this further reduces the number of broken indexes, especially
for data sets with small null_frac, or for append-only (or -mostly)
tables where most of the summarization is bulk.
I still feel a bit uneasy about this, but I think the patch is solid.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 033286cff9733c24fdc7c3f774d947c9f1072aa0 Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Sun, 8 Jan 2023 23:42:41 +0100
Subject: [PATCH] extra tests
---
contrib/pageinspect/Makefile| 2 +-
contrib/pageinspect/expected/brin-fails.out | 152
contrib/pageinspect/sql/brin-fails.sql | 85 +++
3 files changed, 238 insertions(+), 1 deletion(-)
create mode 100644 contrib/pageinspect/expected/brin-fails.out
create mode 100644 contrib/pageinspect/sql/brin-fails.sql
diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index 95e030b396..69a28a6d3d 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -22,7 +22,7 @@ DATA = pageinspect--1.11--1.12.sql pageinspect--1.10--1.11.sql \
pageinspect--1.0--1.1.sql
PGFILEDESC = "pageinspect - functions to inspect contents of database pages"
-REGRESS = page btree brin gin gist hash checksum oldextversions
+REGRESS = page btree brin gin gist hash checksum oldextversions brin-fails
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/contrib/pageinspect/expected/brin-fails.out b/contrib/pageinspect/expected/brin-fails.out
new file mode 100644
index 00..a12c761fc8
--- /dev/null
+++ b/contrib/pageinspect/expected/brin-fails.out
@@ -0,0 +1,152 @@
+create table t (a int);
+create extension pageinspect;
+-- works
+drop index if exists t_a_idx;
+NOTICE: index "t_a_idx" does not exist, skipping
+truncate t;
+insert into t values (null), (1);
+create index on t using brin (a);
+select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass);
+ itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | value
++++--+--+-+--
+ 1 | 0 | 1 | f| t| f
Re: Missing update of all_hasnulls in BRIN opclasses
On Fri, Dec 30, 2022 at 01:18:36AM +0100, Tomas Vondra wrote:
> + * Does the range already has NULL values? Either of the flags
> can
should say: "already have NULL values"
> + * If we had NULLS, and the opclass didn't set allnulls=true,
> set
> + * the hasnulls so that we know there are NULL values.
You could remove "the" before "hasnulls".
Or say "clear hasnulls so that.."
> @@ -585,6 +587,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.
Does "if allnulls" mean "if allnulls is true" ?
It's a bit unclear.
> + */
> + dtup->bt_columns[keyno].bv_hasnulls = hasnulls[keyno];
> +
> if (allnulls[keyno])
> {
> valueno += brdesc->bd_info[keyno]->oi_nstored;
--
Justin
Re: Missing update of all_hasnulls in BRIN opclasses
Hi,
here's an improved and cleaned-up version of the fix.
I removed brinbugs.sql from pageinspect, because it seems enough to have
the other tests (I added brinbugs first, before realizing those exist).
This also means meson.build is fine and there are no tests doing CREATE
EXTENSION concurrently etc.
I decided to go with the 0003 approach, which stores summaries for empty
ranges. That seems to be less intrusive (it's more like what we do now),
and works better for tables with a lot of bulk deletes. It means we can
have ranges with allnulls=hasnulls=true, which wasn't the case before,
but I don't see why this should break e.g. custom opclasses (if it does,
it probably means the opclass is wrong).
Finally, I realized union_tuples needs to be tweaked to deal with empty
ranges properly. The changes are fairly limited, though.
I plan to push this into master right at the beginning of January, and
then backpatch a couple days later.
I still feel a bit uneasy about tweaking this, but I don't think there's
a better way than reusing the existing flags.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 10c03ef1b41f69b54db761b16f8bb1e0642d815b Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Thu, 29 Dec 2022 22:41:36 +0100
Subject: [PATCH] Fix handling of NULLs in BRIN summaries
BRIN did not properly distinguish empty (new) and all-NULL ranges. All
ranges were initialized with all_nulls=true and opclasses simply reset
this to false when adding the first non-NULL value. This fails if the
first value in the range is NULL, and there are no other NULLs in the
range - we forget the range contains a NULL value.
This happens because the "all_nulls" flag is used for two separate
purposes - to mark empty ranges (not representing any rows yet) and
ranges containing only NULL values. The opclass can't know which of
those cases is it.
The opclass might also set has_nulls=true when resetting the all_nulls
flag - that would make it correct, but the indexes would be useless for
IS NULL conditions as all ranges start with all_nulls=true (and so all
ranges would have one of those flags set to true).
Ideally we'd introduce a new "is_empty" flag marking empty summaries,
but that would break ABI and/or on-disk format, depending on where we
add the flag. Considering we need to backpatch this, that doesn't seem
particularly great.
So instead we use an "impossible" combination of both flags (all_nulls
and has_nulls) set to true to mark "empty" ranges. It'd be better to
have a single flag for the whole index tuple (and not per-summary one),
because "range is empty" applies to all ranges in a multi-column index,
but this is where the existing flags are.
We could skip storing index tuples with this combination, but then we'd
have to always read/process this range - even if there are no rows, it
would still require reading the pages etc. So we store them, but ignore
them when building the bitmap.
---
src/backend/access/brin/brin.c| 78 ++-
src/backend/access/brin/brin_tuple.c | 11 ++-
...summarization-and-inprogress-insertion.out | 8 +-
...ummarization-and-inprogress-insertion.spec | 1 +
4 files changed, 89 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 7e386250ae..c7bf8963fa 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -591,6 +591,17 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
bval = >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 have
+ * scan keys, and we know there's nothing to match).
+ */
+ if (bval->bv_allnulls && bval->bv_hasnulls)
+ {
+ 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
@@ -1608,11 +1619,32 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b)
if (opcinfo->oi_regular_nulls)
{
- /* Adjust "hasnulls". */
+ /*
+ * If B is empty (represents no rows), ignore it and just keep
+ * A as is (might be empty etc.).
+ */
+ if (col_b->bv_allnulls && col_b->bv_hasnulls)
+continue;
+
+ /*
+ * Adjust "hasnulls".
+ *
+ * It may happen A has allnulls=true, and we should reset it. We
+ * need to copy the values from B first, which happens later.
+ * We know the next condition can't trigger, because B is not
+ * empty so only one of the flags is true.
+ */
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 there are no values in B, there's nothing left to do.
+ *
+ * Note this is mutually exclusive with the preceding condition.
+ * We have skipped "empty" B,
Re: Missing update of all_hasnulls in BRIN opclasses
On Mon, Nov 28, 2022 at 01:13:14AM +0100, Tomas Vondra wrote: > Opinions? Considering this will need to be backpatches, it'd be good to > get some feedback on the approach. I think it's fine, but it would be > unfortunate to fix one issue but break BRIN in a different way. > --- a/contrib/pageinspect/Makefile > +++ b/contrib/pageinspect/Makefile > @@ -22,7 +22,7 @@ DATA = pageinspect--1.10--1.11.sql \ > pageinspect--1.0--1.1.sql > PGFILEDESC = "pageinspect - functions to inspect contents of database pages" > > -REGRESS = page btree brin gin gist hash checksum oldextversions > +REGRESS = page btree brin gin gist hash checksum oldextversions brinbugs I can't comment on the patch itself, but: These changes to ./Makefile will also need to be made in ./meson.build. Also (per cirrusci), the test sometimes fail since two parallel tests are doing "CREATE EXTENSION".
Re: Missing update of all_hasnulls in BRIN opclasses
Here's an improved version of the fix I posted about a month ago.
0001
Adds tests demonstrating the issue, as before. I realized there's an
isolation test in src/test/module/brin that can demonstrate this, so I
modified it too, not just the pageinspect test as before.
0002
Uses the combination of all_nulls/has_nulls to identify "empty" range,
and does not store them to disk. I however realized not storing "empty"
ranges is probably not desirable. Imagine a table with a "gap" (e.g. due
to a batch DELETE) of pages with no rows:
create table x (a int) with (fillfactor = 10);
insert into x select i from generate_series(1,1000) s(i);
delete from x where a < 1000;
create index on x using brin(a) with (pages_per_range=1);
Any bitmap index scan using this index would have to scan all those
empty ranges, because there are no summaries.
0003
Still uses the all_nulls/has_nulls flags to identify empty ranges, but
stores them - and then we check the combination in bringetbitmap() to
skip those ranges as not matching any scan keys.
This also restores some of the existing behavior - for example creating
a BRIN index on entirely empty table (no pages at all) still allocates a
48kB index (3 index pages, 3 fsm pages). Seems a bit strange, but it's
an existing behavior.
As explained before, I've considered adding an new flag to one of the
BRIN structs - BrinMemTuple or BrinValues. But we can't add as last
field to BrinMemTuple because there already is FLEXIBLE_ARRAY_MEMBER,
and adding a field to BrinValues would change stride of the bt_columns
array. So this would break ABI, making this not backpatchable.
Furthermore, if we want to store summaries for empty ranges (which is
what 0003 does), we need to store the flag in the BRIN index tuple. And
we can't change the on-disk representation in backbranches, so encoding
this in the existing tuple seems like the only way.
So using the combination of all_nulls/has_nulls flag seems like the only
viable option, unfortunately.
Opinions? Considering this will need to be backpatches, it'd be good to
get some feedback on the approach. I think it's fine, but it would be
unfortunate to fix one issue but break BRIN in a different way.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom dea5e7aa821ddf745e509371f33bf1953ff6e853 Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Sat, 22 Oct 2022 12:47:33 +0200
Subject: [PATCH 1/3] pageinspect brinbugs test
Introduce a brinbugs.sql test suite into pageinspect, demonstrating the
issue with forgetting about initial NULL values. Ultimately this should
be added to the exisging brin.sql suite.
Furthermore, this tweaks an existing isolation test, originally intended
to test concurrent inserts and summarization, to also test this - it's
enough to ensure the first value added to the table is NULL.
---
contrib/pageinspect/Makefile | 2 +-
contrib/pageinspect/expected/brinbugs.out | 222 ++
contrib/pageinspect/sql/brinbugs.sql | 114 +
...summarization-and-inprogress-insertion.out | 6 +-
...ummarization-and-inprogress-insertion.spec | 1 +
5 files changed, 341 insertions(+), 4 deletions(-)
create mode 100644 contrib/pageinspect/expected/brinbugs.out
create mode 100644 contrib/pageinspect/sql/brinbugs.sql
diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index ad5a3ac5112..67eb02b78fd 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -22,7 +22,7 @@ DATA = pageinspect--1.10--1.11.sql \
pageinspect--1.0--1.1.sql
PGFILEDESC = "pageinspect - functions to inspect contents of database pages"
-REGRESS = page btree brin gin gist hash checksum oldextversions
+REGRESS = page btree brin gin gist hash checksum oldextversions brinbugs
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/contrib/pageinspect/expected/brinbugs.out b/contrib/pageinspect/expected/brinbugs.out
new file mode 100644
index 000..23843caa138
--- /dev/null
+++ b/contrib/pageinspect/expected/brinbugs.out
@@ -0,0 +1,222 @@
+create extension pageinspect;
+create table t (a int, b int);
+create index on t using brin (a, b);
+-- both columns should have has_nulls=false and [1,1] range
+truncate t;
+insert into t values (1,1);
+vacuum full t;
+select * from brin_page_items(get_raw_page('t_a_b_idx', 2), 't_a_b_idx'::regclass);
+ itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | value
++++--+--+-+--
+ 1 | 0 | 1 | f| f| f | {1 .. 1}
+ 1 | 0 | 2 | f| f| f | {1 .. 1}
+(2 rows)
+
+-- first column should have all_nulls=true, second has_nulls=false and [1,1] range
+truncate t;
+insert into t values (null, 1);
+vacuum full t;
+select * from brin_page_items(get_raw_page('t_a_b_idx', 2), 't_a_b_idx'::regclass);
+
Re: Missing update of all_hasnulls in BRIN opclasses
Hi, Tomas: For 0002-fixup-brin-has_nulls-20221022.patch : + first_row = (bval->bv_hasnulls && bval->bv_allnulls); + + if (bval->bv_hasnulls && bval->bv_allnulls) It seems the if condition can be changed to `if (first_row)` which is more readable. Chhers
Re: Missing update of all_hasnulls in BRIN opclasses
On 10/22/22 10:00, Alvaro Herrera wrote:
> On 2022-Oct-22, Tomas Vondra wrote:
>
>> I wonder how to do this in a back-patchable way - we can't add
>> parameters to the opclass procedure, and the other solution seems to be
>> storing it right in the BrinMemTuple, somehow. But that's likely an ABI
>> break :-(
>
> Hmm, I don't see the ABI incompatibility. BrinMemTuple is an in-memory
> structure, so you can add new members at the end of the struct and it
> will pose no problems to existing code.
>
But we're not passing BrinMemTuple to the opclass procedures - we're
passing a pointer to BrinValues, so we'd have to add the flag there. And
we're storing an array of those, so adding a field may shift the array
even if you add it at the end. Not sure if that's OK or not.
>> The only solution I can think of is actually passing it using all_nulls
>> and has_nulls - we could set both flags to true (which we never do now)
>> and teach the opclass that it signifies "empty" (and thus not to update
>> has_nulls after resetting all_nulls).
>>
>> Something like the attached (I haven't added any more tests, not sure
>> what would those look like - I can't think of a query testing this,
>> although maybe we could check how the flags change using pageinspect).
>
> I'll try to have a look at these patches tomorrow or on Monday.
>
I was experimenting with this a bit more, and unfortunately the latest
patch is still a few bricks shy - it did fix this particular issue, but
there were other cases that remained/got broken. See the first patch,
that adds a bunch of pageinspect tests testing different combinations.
After thinking about it a bit more, I think we can't quite fix this at
the opclass level, so the yesterday's patches are wrong. Instead, this
should be fixed in values_add_to_range() - the whole trick is we need to
remember the range was empty at the beginning, and only set the flag
when allnulls is false.
The reworked patch does that. And we can use the same logic (both flags
set mean no tuples were added to the range) when building the index, a
separate flag is not needed.
This slightly affects existing regression tests, because we won't create
any ranges for empty table (now we created one, because we initialized a
tuple in brinbuild and then wrote it to disk). This means that
brin_summarize_range now returns 0, but I think that's fine.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 5456cf819426d3f90c004f673dfc863903e568a1 Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Sat, 22 Oct 2022 12:47:33 +0200
Subject: [PATCH 1/2] pageinspect brinbugs test
---
contrib/pageinspect/Makefile | 2 +-
contrib/pageinspect/expected/brinbugs.out | 222 ++
contrib/pageinspect/sql/brinbugs.sql | 114 +++
3 files changed, 337 insertions(+), 1 deletion(-)
create mode 100644 contrib/pageinspect/expected/brinbugs.out
create mode 100644 contrib/pageinspect/sql/brinbugs.sql
diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index 5c0736564ab..92305e981f7 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -21,7 +21,7 @@ DATA = pageinspect--1.9--1.10.sql pageinspect--1.8--1.9.sql \
pageinspect--1.0--1.1.sql
PGFILEDESC = "pageinspect - functions to inspect contents of database pages"
-REGRESS = page btree brin gin gist hash checksum oldextversions
+REGRESS = page btree brin gin gist hash checksum oldextversions brinbugs
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/contrib/pageinspect/expected/brinbugs.out b/contrib/pageinspect/expected/brinbugs.out
new file mode 100644
index 000..23843caa138
--- /dev/null
+++ b/contrib/pageinspect/expected/brinbugs.out
@@ -0,0 +1,222 @@
+create extension pageinspect;
+create table t (a int, b int);
+create index on t using brin (a, b);
+-- both columns should have has_nulls=false and [1,1] range
+truncate t;
+insert into t values (1,1);
+vacuum full t;
+select * from brin_page_items(get_raw_page('t_a_b_idx', 2), 't_a_b_idx'::regclass);
+ itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | value
++++--+--+-+--
+ 1 | 0 | 1 | f| f| f | {1 .. 1}
+ 1 | 0 | 2 | f| f| f | {1 .. 1}
+(2 rows)
+
+-- first column should have all_nulls=true, second has_nulls=false and [1,1] range
+truncate t;
+insert into t values (null, 1);
+vacuum full t;
+select * from brin_page_items(get_raw_page('t_a_b_idx', 2), 't_a_b_idx'::regclass);
+ itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | value
++++--+--+-+--
+ 1 | 0 | 1 | t| f| f |
+ 1 | 0 | 2 | f| f| f | {1 .. 1}
+(2 rows)
+
+-- both columns
Re: Missing update of all_hasnulls in BRIN opclasses
On 2022-Oct-22, Tomas Vondra wrote: > I wonder how to do this in a back-patchable way - we can't add > parameters to the opclass procedure, and the other solution seems to be > storing it right in the BrinMemTuple, somehow. But that's likely an ABI > break :-( Hmm, I don't see the ABI incompatibility. BrinMemTuple is an in-memory structure, so you can add new members at the end of the struct and it will pose no problems to existing code. > The only solution I can think of is actually passing it using all_nulls > and has_nulls - we could set both flags to true (which we never do now) > and teach the opclass that it signifies "empty" (and thus not to update > has_nulls after resetting all_nulls). > > Something like the attached (I haven't added any more tests, not sure > what would those look like - I can't think of a query testing this, > although maybe we could check how the flags change using pageinspect). I'll try to have a look at these patches tomorrow or on Monday. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "I suspect most samba developers are already technically insane... Of course, since many of them are Australians, you can't tell." (L. Torvalds)
Re: Missing update of all_hasnulls in BRIN opclasses
On 10/21/22 18:44, Tomas Vondra wrote: > > ... > >> Apart from that, this patch looks good. >> Sadly, I don't think we can fix it like this :-( The problem is that all ranges start with all_nulls=true, because the new range gets initialized by brin_memtuple_initialize() like that. But this happens for *every* range before we even start processing the rows. So this way all the ranges would end up with has_nulls=true, making that flag pretty useless. Actually, even just doing "truncate" on the table creates such all-nulls range for the first range, and serializes it to disk. I wondered why we even write such tuples for "empty" ranges to disk, for example after "TRUNCATE" - the table is empty by definition, so how come we write all-nulls brin summary for the first range? For example brininsert() checks if the brin tuple was modified and needs to be written back, but brinbuild() just ignores that, and initializes (and writes) writes the tuple to disk anyway. I think we should not do that - there should be a flag in BrinBuildState, tracking if the BRIN tuple was modified, and we should only write it if it's true. That means we should never get an on-disk summary representing nothing. That doesn't fix the issue, though, because we still need to pass the memtuple tuple to the add_value opclass procedure, and whether it sets the has_nulls flag depends on whether it's a new tuple representing no other rows (in which case has_nulls remains false) or whether it was read from disk (in which case it needs to be flipped to 'true'). But the opclass has no way to tell the difference at the moment - it just gets the BrinMemTuple. So we'd have to extend this, somehow. I wonder how to do this in a back-patchable way - we can't add parameters to the opclass procedure, and the other solution seems to be storing it right in the BrinMemTuple, somehow. But that's likely an ABI break :-( The only solution I can think of is actually passing it using all_nulls and has_nulls - we could set both flags to true (which we never do now) and teach the opclass that it signifies "empty" (and thus not to update has_nulls after resetting all_nulls). Something like the attached (I haven't added any more tests, not sure what would those look like - I can't think of a query testing this, although maybe we could check how the flags change using pageinspect). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL CompanyFrom a99fd6a737cec24bb4063e99a241ff3e04c6ebb8 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Thu, 20 Oct 2022 19:55:23 +0200 Subject: [PATCH 1/9] fixup: brin has_nulls --- src/backend/access/brin/brin_bloom.c| 1 + src/backend/access/brin/brin_inclusion.c| 1 + src/backend/access/brin/brin_minmax.c | 1 + src/backend/access/brin/brin_minmax_multi.c | 1 + 4 files changed, 4 insertions(+) diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.c index 6b0af7267d5..60315450b41 100644 --- a/src/backend/access/brin/brin_bloom.c +++ b/src/backend/access/brin/brin_bloom.c @@ -539,6 +539,7 @@ brin_bloom_add_value(PG_FUNCTION_ARGS) BloomGetFalsePositiveRate(opts)); column->bv_values[0] = PointerGetDatum(filter); column->bv_allnulls = false; + column->bv_hasnulls = true; updated = true; } else diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c index 4b02d374f23..e0f44d3e62c 100644 --- a/src/backend/access/brin/brin_inclusion.c +++ b/src/backend/access/brin/brin_inclusion.c @@ -164,6 +164,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS) column->bv_values[INCLUSION_UNMERGEABLE] = BoolGetDatum(false); column->bv_values[INCLUSION_CONTAINS_EMPTY] = BoolGetDatum(false); column->bv_allnulls = false; + column->bv_hasnulls = true; new = true; } diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c index 9e8a8e056cc..8a5661a8952 100644 --- a/src/backend/access/brin/brin_minmax.c +++ b/src/backend/access/brin/brin_minmax.c @@ -90,6 +90,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS) column->bv_values[0] = datumCopy(newval, attr->attbyval, attr->attlen); column->bv_values[1] = datumCopy(newval, attr->attbyval, attr->attlen); column->bv_allnulls = false; + column->bv_hasnulls = true; PG_RETURN_BOOL(true); } diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c index 9a0bcf6698d..4e7119e2d78 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; + column->bv_hasnulls = true; modified = true; column->bv_mem_value = PointerGetDatum(ranges); -- 2.37.3 From 57e53d34f2f7bba91fcc0de6f4eff551669554fb Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sat, 22 Oct 2022 02:26:48
Re: Missing update of all_hasnulls in BRIN opclasses
On 10/21/22 17:50, Matthias van de Meent wrote: > On Fri, 21 Oct 2022 at 17:24, Tomas Vondra > wrote: >> >> Hi, >> >> While working on some BRIN code, I discovered a bug in handling NULL >> values - when inserting a non-NULL value into a NULL-only range, we >> reset the all_nulls flag but don't update the has_nulls flag. And >> because of that we then fail to return the range for IS NULL ranges. > > Ah, that's bad. > Yeah, I guess we'll need to inform the users to consider rebuilding BRIN indexes on NULL-able columns. > One question though: doesn't (shouldn't?) column->bv_allnulls already > imply column->bv_hasnulls? The column has nulls if all of the values > are null, right? Or is the description of the field deceptive, and > does bv_hasnulls actually mean "has nulls bitmap"? > What null bitmap do you mean? We're talking about summary for a page range - IIRC we translate this to nullbitmap for a BRIN tuple, but there may be multiple columns, and "has nulls bitmap" is an aggregate over all of them. Yeah, maybe it'd make sense to also have has_nulls=true whenever all_nulls=true, and maybe it'd be simpler because it'd be enough to check just one flag in consistent function etc. But we still need to track 2 different states - "has nulls" and "has summary". In any case, this ship sailed long ago - at least for the existing opclasses. >> Attached is a patch fixing this by properly updating the has_nulls flag. > > One comment on the patch: > >> +SET enable_seqscan = off; >> + [...] >> +SET enable_seqscan = off; > > Looks like duplicated SETs. Should that last one be RESET instead? > Yeah, should have been RESET. > Apart from that, this patch looks good. > Thanks! -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Missing update of all_hasnulls in BRIN opclasses
On Fri, 21 Oct 2022 at 17:24, Tomas Vondra wrote: > > Hi, > > While working on some BRIN code, I discovered a bug in handling NULL > values - when inserting a non-NULL value into a NULL-only range, we > reset the all_nulls flag but don't update the has_nulls flag. And > because of that we then fail to return the range for IS NULL ranges. Ah, that's bad. One question though: doesn't (shouldn't?) column->bv_allnulls already imply column->bv_hasnulls? The column has nulls if all of the values are null, right? Or is the description of the field deceptive, and does bv_hasnulls actually mean "has nulls bitmap"? > Attached is a patch fixing this by properly updating the has_nulls flag. One comment on the patch: > +SET enable_seqscan = off; > + [...] > +SET enable_seqscan = off; Looks like duplicated SETs. Should that last one be RESET instead? Apart from that, this patch looks good. - Matthias
