Re: REINDEX INDEX results in a crash for an index of pg_class since 9.6

Hi,

On 2019-05-01 19:41:24 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres(at)anarazel(dot)de> writes:
> >> On 2019-05-01 10:06:03 -0700, Andres Freund wrote:
> >>> I'm not sure this is the right short-term answer. Why isn't it, for now,
> >>> sufficient to do what I suggested with RelationSetNewRelfilenode() not
> >>> doing the CommandCounterIncrement(), and reindex_index() then doing the
> >>> SetReindexProcessing() before a CommandCounterIncrement()? That's like
> >>> ~10 line code change, and a few more with comments.
>
> > That looks like a hack to me...
>
> > The main thing I'm worried about right now is that I realized that
> > our recovery from errors in this area is completely hosed, cf
> > https://www.postgresql.org/message-id/4541.1556736252@sss.pgh.pa.us
>
> OK, so per the other thread, it seems like the error recovery problem
> isn't going to affect this directly. However, I still don't like this
> proposal much; the reason being that it's a rather fundamental change
> in the API of RelationSetNewRelfilenode. This will certainly break
> any external callers of that function --- and silently, too.
>
> Admittedly, there might not be any outside callers, but I don't really
> like that assumption for something we're going to have to back-patch.

Couldn't we just address that by adding a new
RelationSetNewRelfilenodeInternal() that's then wrapped by
RelationSetNewRelfilenode() which just does
RelationSetNewRelfilenodeInternal();CCI();?

Doesn't have to be ...Internal(), could also be
RelationBeginSetNewRelfilenode() or such.

I'm not sure why you think using CCI() for this purpose is a hack? To me
the ability to have catalog changes only take effect when they're all
done, and the system is ready for them, is one of the core purposes of
the infrastructure?

> The solution I'm thinking of should have much more localized effects,
> basically just in reindex_index and RelationSetNewRelfilenode, which is
> why I like it better.

Well, as I said before, I think hiding the to-be-rebuilt index from the
list of indexes is dangerous too - if somebody added an actual
CatalogUpdate/Insert (rather than inplace_update) anywhere along the
index_build() path, we'd not get an assertion failure anymore, but just
an index without the new entry. And given the fragility with HOT hiding
that a lot of the time, that seems dangerous to me.

Greetings,

Andres Freund