On Sun, Oct 25, 2020 at 7:13 PM Julien Rouhaud <rjuju...@gmail.com> wrote: > > On Sun, Oct 25, 2020 at 10:36 AM Thomas Munro <thomas.mu...@gmail.com> wrote: > > > > On Fri, Oct 23, 2020 at 2:07 AM Julien Rouhaud <rjuju...@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!
