On Tue, 14 Mar 2023 at 14:49, Tomas Vondra <tomas.von...@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