Re: REINDEX backend filtering

On Mon, Mar 15, 2021 at 10:56:50AM -0700, Mark Dilger wrote:
>
>
> > On Mar 15, 2021, at 10:50 AM, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> >
> > On Mon, Mar 15, 2021 at 10:40:25AM -0700, Mark Dilger wrote:
> >> I'm saying that your patch seems to call down to get_collation_actual_version() via get_collation_version_for_oid() from your new function do_check_index_has_outdated_collation(), but I'm not seeing how that gets exercised.
> >
> > It's a little bit late here so sorry if I'm missing something.
> >
> > do_check_index_has_outdated_collation() is called from
> > index_has_outdated_collation() which is called from
> > index_has_outdated_dependency() which is called from
> > RelationGetIndexListFiltered(), and that function is called when passing the
> > OUTDATED option to REINDEX (and reindexdb --outdated). So this is exercised
> > with added tests for both matching and non matching collation version.
>
> Ok, fair enough. I was thinking about the case where the collation actually returns a different version number because it (the C library providing the collation) got updated, but I think you've answered already that you are not planning to test that case, only the case where pg_depend is modified to have a bogus version number.

This infrastructure is supposed to detect that the collation library *used to*
return a different version before it was updated. And that's exactly what
we're testing by manually updating the refobjversion.

> It seems a bit odd to me that a feature intended to handle cases where collations are updated is not tested via having a collation be updated during the test. It leaves open the possibility that something differs between the test and reindexed run after real world collation updates. But that's for the committer who picks up your patch to decide, and perhaps it is unfair to make your patch depend on addressing that issue.

Why is that odd? We're testing that we're correctly storing the collation
version during index creating and correctly detecting a mismatch. Having a
fake collation provider to return a fake version number won't add any more
coverage unless I'm missing something.

It's similar to how we test the various corruption scenario. AFAIK we're not
providing custom drivers to write corrupted data but we're simply simulating a
corruption overwriting some blocks.