On Sun, Oct 25, 2020 at 7:13 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> 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.
I finish looking at the rest of the patches. I don't have much to
say, it all looks good and I quite like how much useless code you got
rid of!