Re: old_snapshot_threshold vs indexes

On Tue, Aug 27, 2019 at 1:54 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> > OK I started writing a patch and realised there were a few ugly
> > problems that I was about to report here... but now I wonder if, based
> > on the comment for RelationHasUnloggedIndex(), we shouldn't just nuke
> > all this code. We don't actually support unlogged indexes on
> > permanent tables (there is no syntax to create them, and
> > RelationHasUnloggedIndex() will never return true in practice because
> > RelationNeedsWAL() will always return false first).
>
> Oh! That explains why the code coverage report shows clearly that
> RelationHasUnloggedIndex never returns true ;-)
>
> > This is a locking
> > protocol violation and a performance pessimisation in support of a
> > feature we don't have. If we add support for that in some future
> > release, we can figure out how to do it properly then, no?
>
> +1. That fix is also back-patchable, which adding fields to relcache
> entries would not be.

There is a fly in the ointment: REL9_6_STABLE's copy of
RelationHasUnloggedIndex() is hardcoded to return true for hash
indexes (see commit 2cc41acd8).

However, I now see that there isn't a buffer content lock deadlock
risk here after all, because we don't reach RelationHasUnloggedIndex()
if IsCatalogRelation(rel). That reminds me of commit 4fd05bb55b4. It
still doesn't seem like a great idea to be doing catalog access while
holding the buffer content lock, though.

So I think we need to leave 9.6 as is, and discuss how far back to
back-patch the attached. It could go back to 10, but perhaps we
should be cautious and push it to master only for now, if you agree
with my analysis of the deadlock thing.

> It's not really apparent to me that unlogged indexes on logged tables
> would ever be a useful combination, so I'm certainly willing to nuke
> poorly-thought-out code that putatively supports it. But perhaps
> we should add some comments to remind us that this area would need
> work if anyone ever wanted to support that. Not sure where.

It might make sense for some kind of in-memory index that is rebuilt
from the heap at startup, but then I doubt such a thing would have an
index relation with a relpersistence to check anyway.

--
Thomas Munro
https://enterprisedb.com