Re: Collation versioning

On Fri, Nov 8, 2019 at 2:24 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>
> On Thu, Nov 7, 2019 at 10:20 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> >
> > I didn't do anything for the spurious warning when running a reindex,
> > and kept original approach of pg_depend catalog.
>
> I see three options:
>
> 1. Change all or some of index_open(), relation_open(),
> RelationIdGetRelation(), RelationBuildDesc() and
> RelationInitIndexAccessInfo() to take some kind of flag so we can say
> NO_COLLATION_VERSION_CHECK_PLEASE, and then have ReindexIndex() pass
> that flag down when opening it for the purpose of rebuilding it.
> 2. Use a global state to suppress these warnings while opening that
> index. Perhaps ReindexIndex() would call RelCacheWarnings(false)
> before index_open(), and use PG_FINALLY to make sure it restores
> warnings with RelCacheWarnings(true). (This is a less-code-churn
> bad-programming-style version of #1.)
> 3. Move the place that we run the collation version check. Instead
> of doing it in RelationInitIndexAccessInfo() so that it runs when the
> relation cache first loads the index, put it into a new routine
> RelationCheckValidity() and call that from ... I don't know, some
> other place that runs whenever we access indexes but not when we
> rebuild them.
>
> 3's probably a better idea, if you can find a reasonable place to call
> it from. I'm thinking some time around the time the executor acquires
> locks, but using a flag in the relcache entry to make sure it doesn't
> run the check again if there were no warnings last time (so one
> successful version check turns the extra work off for the rest of this
> relcache entry's lifetime). I think it'd be a feature, not a bug, if
> it gave you the warning every single time you executed a query using
> an index that has a mismatch... but a practical alternative would be
> to check only once per index and that probably makes more sense.

I tried to dig into approach #3. I think that trying to perform this
check when the executor acquires locks won't work well with at least
with partitioned table. I instead tried to handle that in
get_relation_info(), adding a flag in the relcache to emit each
warning only once per backend lifetime, and this seems to work quite
well.

I think that on top of that, we should make sure that non-full vacuum
and analyze also emit such warnings, so that autovacuum can spam the
logs too.