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
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
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'
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
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 pos
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().
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 ran
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 dat
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'
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
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
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
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 t
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
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
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
> a
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 whe
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 rewor
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 mo
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 ha
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
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/pageinsp
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
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
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
>
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 incompatibi
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 happe
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
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 fai
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.
Reproducing this is t
30 matches
Mail list logo
