On Tue, 14 Mar 2023 at 14:49, Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
>
>
> On 3/8/23 23:31, Matthias van de Meent wrote:
> > On Wed, 22 Feb 2023 at 14:14, Matthias van de Meent
> >
> > 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.
Sure
> 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?
I don't like this double negation, as it adds significant parsing
complexity to the statement. If I'd have gone with a single Assert()
statement, I'd have used the following:
Assert((!onlySummarized) || HeapTupleIsHeapOnly(heapTuple));
because in the code section above that the HOT + !onlySummarized case
is an early exit.
> 4) A couple comments and minor tweaks.
> 5) Undoing a couple unnecessary changes (whitespace, ...).
> 6) Proper formatting of TU_UpdateIndexes enum.
Allright
> + *
> + * XXX Why do we assign explicit values to the members, instead of just letting
> + * it up to the enum (just like for TM_Result)?
This was from the v15 beta window, to reduce the difference between
bool and TU_UpdateIndexes. With pg16, that can be dropped.
>
> 7) Comment in RelationGetIndexAttrBitmap() is misleading, as it still
> references hotblockingattrs, even though it may update summarizedattrs
> in some cases.
How about
Since we have covering indexes with non-key columns, we must
handle them accurately here. Non-key columns must be added into
the hotblocking or summarizing attrs bitmap, since they are in
the index, and update shouldn't miss them.
instead for that section?
> If you agree with these changes, I'll get it committed.
Yes, thanks!
Kind regards,
Matthias van de Meent
