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

Hi,

On 2019-04-25 16:02:03 -0400, Tom Lane wrote:
> > Currently the only thing that table_relation_set_new_filenode() accesses
> > that already is updated is the RelFileNode. I wonder if we shouldn't
> > change the API so that table_relation_set_new_filenode() will get a
> > relcache entry *without* any updates passed in, then internally does
> > GetNewRelFileNode() (if so desired by the AM), and returns the new rnode
> > via a new out parameter.
>
> That could work. The important API spec is then that the relcache entry
> reflects the *previous* state of the relation, and is not to be modified
> by the tableam call.

Right.

I was wondering if we should just pass in the pg_class tuple as an "out"
argument, instead of pointers to relfilnode/relfrozenxid/relminmxid.

> > We don't currently allow that, but as far as I can see the current
> > coding of ATExecSetTableSpace() also has bad problems with system
> > catalog updates. It copies the data and *then* does
> > CatalogTupleUpdate(), but *witout* updating the reclache - which ijust
> > would cause the update to be lost.
>
> Well, I imagine it's expecting the subsequent CCI to update the relcache
> entry, which I think is correct behavior in this worldview. We're
> basically trying to make the relcache state follow transaction/command
> boundary semantics.

My point was that given the current coding the code in
ATExecSetTableSpace() would make changes to the *old* relfilenode, after
having already copied the contents to the new relfilenode. Which means
that if ATExecSetTableSpace() is ever used on pg_class or one of it's
indexes, it'd just loose those changes, afaict.

> > I could come up with a patch for that if you want me to.
>
> I'm happy to let you take a whack at it if you want.

I'll give it a whack (after writing one more email on a separate but
loosely related topic).

Greetings,

Andres Freund