Fix several recently introduced issues around handling new relation forks

Enterprise / PostgreSQL - Andres Freund [] - 30 April 2019 02:28 EDT

Most of these stem from d25f519107 "tableam: relation creation, VACUUM FULL/CLUSTER, SET TABLESPACE.".

1) To pass data to the relation_set_new_filenode() RelationSetNewRelfilenode() was made to update RelationData.rd_rel directly. That's not OK however, as it makes the relcache entries temporarily inconsistent. Which among other scenarios is a problem if a REINDEX targets an index on pg_class - the CatalogTupleUpdate() in RelationSetNewRelfilenode(). Presumably that was introduced because other places in the code do so - while those aren't "good practice" they don't appear to be actively buggy (e.g. because system tables may not be targeted).

I (Andres) should have caught this while reviewing and signficantly evolving the code in that commit, mea culpa.

Fix that by instead passing in the new RelFileNode as separate argument to relation_set_new_filenode() and rely on the relcache to update the catalog entry. Also revert that the RelationMapUpdateMap() call was changed to immediate, and undo some other more unnecessary changes.

2) Document that the relation_set_new_filenode cannot rely on the whole relcache entry to be valid. It might be worthwhile to refactor the code to never have to rely on that, but given the way heap_create() is currently coded, that'd be a large change.

3) ATExecSetTableSpace() shouldn't do FlushRelationBuffers() itself. A table AM might not use shared buffers at all. Move to index_copy_data() and heapam_relation_copy_data().

4) heapam_relation_set_new_filenode() previously sometimes accessed rel->rd_rel->relpersistence rather than the `persistence` argument. Code movement mistake.

5) Previously heapam_relation_set_new_filenode() re-opened the smgr relation to create the init for, if necesary. Instead have RelationCreateStorage() return the SMgrRelation and use it to create the init fork.

6) Add a note about the danger of modifying the relcache directly to ATExecSetTableSpace() - it's currently not a bug because there's a check ERRORing for catalog tables.

Regression tests and assertion improvements that together trigger the bug described in 1) will be added in a later commit, as there is a related bug on all branches.

5c1560606d Fix several recently introduced issues around handling new relation forks.
src/backend/access/heap/heapam_handler.c | 35 +++++++++++++-------
src/backend/catalog/heap.c | 5 +--
src/backend/catalog/storage.c | 12 +++++--
src/backend/commands/tablecmds.c | 26 +++++++++------
src/backend/utils/cache/relcache.c | 55 +++++++++++++++++---------------
src/include/access/tableam.h | 25 +++++++++++----
src/include/catalog/storage.h | 2 +-
src/include/catalog/storage_xlog.h | 2 +-
8 files changed, 101 insertions(+), 61 deletions(-)


  • Share