On Thu, Dec 31, 2020 at 11:01 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> Happy New Year.
Happy New Year.
> For v12-0001-Pass-down-logically-unchanged-index-hint.patch
>
> + if (hasexpression)
> + return false;
> +
> + return true;
>
> The above can be written as return !hasexpression;
> The negation is due to the return value from index_unchanged_by_update_var_walker() is inverse of index unchanged.
> If you align the meaning of return value from index_unchanged_by_update_var_walker() the same way as
index_unchanged_by_update(),negation is not needed.
I don't think that that represents an improvement. The negation seems
clearer to me because we're negating the *absence* of something that
we search for more or less linearly (a modified column from the
index). This happens when determining whether to do an extra thing
(provide the "logically unchanged" hint to this particular
index/aminsert() call). To me, the negation reflects that high level
structure.
> For struct xl_btree_delete:
>
> + /* DELETED TARGET OFFSET NUMBERS FOLLOW */
> + /* UPDATED TARGET OFFSET NUMBERS FOLLOW */
> + /* UPDATED TUPLES METADATA (xl_btree_update) ARRAY FOLLOWS */
>
> I guess the comment is for illustration purposes. Maybe you can write the comment in lower case.
The comment is written like this (in higher case) to be consistent
with an existing convention. If this was a green field situation I
suppose I might not write it that way, but that's not how I assess
these things. I always try to give the existing convention the benefit
of the doubt. In this case I don't think that it matters very much,
and so I'm inclined to stick with the existing style.
> +#define BOTTOMUP_FAVORABLE_STRIDE 3
>
> Adding a comment on why the number 3 is chosen would be helpful for people to understand the code.
Noted - I will add something about that to the next revision.
--
Peter Geoghegan