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.