On 2/20/23 19:15, Matthias van de Meent wrote:
> Hi,
>
> On Sun, 19 Feb 2023 at 16:04, Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> Hi,
>>
>> On 2/19/23 02:03, Matthias van de Meent wrote:
>>> On Thu, 16 Jun 2022 at 15:05, Tomas Vondra
>>> <tomas.vondra@enterprisedb.com> wrote:
>>>>
>>>> I've pushed the revert. Let's try again for PG16.
>>>
>>> As we discussed in person at the developer meeting, here's a patch to
>>> try again for PG16.
>>>
>>> It combines the committed patches with my fix, and adds some
>>> additional comments and polish. I am confident the code is correct,
>>> but not that it is clean (see the commit message of the patch for
>>> details).
>>>
>>
>> Thanks for the patch. I took a quick look, and I agree it seems correct,
>> and fairly clean too.
>
> Thanks. Based on feedback, attached is v2 of the patch, with as
> significant changes:
>
> - We don't store the columns we mention in predicates of summarized
> indexes in the hotblocking column anymore, they are stored in the
> summarized columns bitmap instead. This further reduces the chance of
> failiing to apply HOT with summarizing indexes.
Interesting idea. I need to think about the correctness, but AFAICS it
should work. Do we have any tests covering such cases?
I see both v1 and v2 had exactly this
src/test/regress/expected/stats.out | 110 ++++++++++++++++++
src/test/regress/sql/stats.sql | 82 ++++++++++++-
so I guess there are no new tests testing this for BRIN with predicates.
We should probably add some ...
> - The heaptuple header bit for summarized update in inserted tuples is
> replaced with passing an out parameter. This simplifies the logic and
> decreases chances of accidentally storing incorrect data.
>
OK.
0002 proposes a minor RelationGetIndexPredicate() tweak, getting rid of
the repeated if/else branches. Feel free to discard, if you think the v2
approach is better.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company