• Andres Freund's avatar
    Fix several recently introduced issues around handling new relation forks. · 5c156060
    Andres Freund authored
    Most of these stem from d25f5191 "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.
    
    Reported-By: Michael Paquier
    Diagnosed-By: Tom Lane and Andres Freund
    Author: Andres Freund
    Reviewed-By: Tom Lane
    Discussion: https://postgr.es/m/20190418011430.GA19133@paquier.xyz
    5c156060
storage_xlog.h 1.56 KB