Horton

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

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>, Postgres hackers <pgsql-hackers@lists.postgresql.org>
Date: 2019-04-25T21:03:09Z
Lists: pgsql-hackers 
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

Commits

  1. Remove reindex_catalog test from test schedules.

  2. Remove RelationSetIndexList().

  3. Fix reindexing of pg_class indexes some more.

  4. Run catalog reindexing test from 3dbb317d32 serially, to avoid deadlocks.

  5. Fix potential assertion failure when reindexing a pg_class index.

  6. Fix several recently introduced issues around handling new relation forks.

  7. Avoid VACUUM FULL altogether in initdb.

  8. Introduce replication progress tracking infrastructure.

  9. initdb: remove unnecessary VACUUM FULL