From: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Cc: | Justin Pryzby <pryzby(at)telsasoft(dot)com>, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Missing update of all_hasnulls in BRIN opclasses |
Date: | 2023-04-24 21:20:32 |
Message-ID: | 491ba86b-6294-37bb-fe00-c91e829dfc82@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Melanie Plageman | 2023-04-24 21:37:48 | Re: pg_stat_io not tracking smgrwriteback() is confusing |
Previous Message | Andres Freund | 2023-04-24 21:14:32 | Re: pg_stat_io not tracking smgrwriteback() is confusing |