Hi, Thanks for the review!
On Mon, Feb 8, 2021 at 12:14 AM Zhihong Yu <z...@yugabyte.com> wrote:
>
> Hi,
> For index_has_deprecated_collation(),
>
> + object.objectSubId = 0;
>
> The objectSubId field is not accessed by
> do_check_index_has_deprecated_collation(). Does it need to be assigned ?
Indeed it's not strictly necessary I think, but it makes things
cleaner and future proof, and that's how things are already done
nearby. So I think it's better to keep it this way.
> For RelationGetIndexListFiltered(), it seems when (options &
> REINDEXOPT_COLL_NOT_CURRENT) == 0, the full_list would be returned.
> This can be checked prior to entering the foreach loop.
That's already the case with this test:
/* Fast exit if no filtering was asked, or if the list if empty. */
if (!reindexHasFilter(options) || full_list == NIL)
return full_list;
knowing that
#define reindexHasFilter(x) ((x & REINDEXOPT_COLL_NOT_CURRENT) != 0)
The code as-is written to be extensible with possibly other filters
(e.g. specific library or specific version). Feedback so far seems to
indicate that it may be overkill and only filtering indexes with
deprecated collation is enough. I'll simplify this code in a future
version, getting rid of reindexHasFilter, unless someone thinks more
filter is a good idea.
For now I'm attaching a rebased version, there was a conflict with the
recent patch to add the missing_ok parameter to
get_collation_version_for_oid()
v4-0001-Add-a-new-COLLATION-option-to-REINDEX.patch
Description: Binary data
