On Sun, Oct 25, 2020 at 10:36 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Fri, Oct 23, 2020 at 2:07 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > While reviewing the changes, I found a couple of minor issues
> > (inherited from the previous versions). It's missing a
> > free_objects_addresses in recordDependencyOnCollations, and there's a
> > small typo. Inline diff:
>
> Thanks, fixed.
>
> I spent the past few days trying to simplify this patch further.
> Here's what I came up with:
Thanks!
>
> 1. Drop the function dependencyExists() and related code, which in
> earlier versions tried to avoid creating duplicate pg_depends rows by
> merging with existing rows. This was rather complicated, and there
> are not many duplicates anyway, and it's easier to suppress duplicate
> warnings at warning time (see do_collation_version_check()). (I'm not
> against working harder to make pg_depend rows unique, but it's not
> required for this and I didn't like that way of doing it.)
I didn't review all the changes yet, so I'll probably post a deeper
review tomorrow. I'm not opposed to this new approach, as it indeed
saves a lot of code. However, looking at
do_collation_version_check(), it seems that you're saving the
collation in context->checked_calls even if it didn't raise a WARNING.
Since you can now have indexes with dependencies on a same collation
with both version tracked and untracked (see for instance
icuidx00_val_pattern_where in the regression tests), can't this hide
corruption warning reports if the untracked version is found first?
That can be easily fixed, so no objection to that approach of course.