Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

On 3/8/23 23:31, Matthias van de Meent wrote:
> On Wed, 22 Feb 2023 at 14:14, Matthias van de Meent
> <boekewurm+postgres(at)gmail(dot)com> wrote:
>>
>> On Wed, 22 Feb 2023 at 13:15, Tomas Vondra
>> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>>>
>>> On 2/20/23 19:15, Matthias van de Meent wrote:
>>>> 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?
>>
>> There is a test that checks that an update to the predicated column
>> does update the index (on table brin_hot_2). However, the description
>> was out of date, so I've updated that in v4.
>>
>>>> - 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.
>>
>> I agree that this is better, it's included in v4 of the patch, as attached.
>
> I think that the v4 patch solves all comments up to now; and
> considering that most of this patch was committed but then reverted
> due to an issue in v15, and that said issue is fixed in this patch,
> I'm marking this as ready for committer.
>
> Tomas, would you be up for that?
>

Thanks for the patch. I started looking at it yesterday, and I think
it's 99% RFC. I think it's correct and I only have some minor comments,
(see the 0002 patch):

1) There were still a couple minor wording issues in the sgml docs.

2) bikeshedding: I added a bunch of "()" to various conditions, I think
it makes it clearer.

3) This seems a bit weird way to write a conditional Assert:

if (onlySummarized)
Assert(HeapTupleIsHeapOnly(heapTuple));

better to do a composed Assert(!(onlySummarized && !...)) or something?

4) A couple comments and minor tweaks.

5) Undoing a couple unnecessary changes (whitespace, ...).

6) Proper formatting of TU_UpdateIndexes enum.

7) Comment in RelationGetIndexAttrBitmap() is misleading, as it still
references hotblockingattrs, even though it may update summarizedattrs
in some cases.

If you agree with these changes, I'll get it committed.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company