In rebuild_relation(), don't access an already-closed relcache entry.

This reliably fails with -DRELCACHE_FORCE_RELEASE, as reported by
Andrew Dunstan, and could sometimes fail in normal operation, resulting
in a wrong persistence value being used for the transient table.
It's not immediately clear to me what effects that might have beyond
the risk of a crash while accessing OldHeap->rd_rel->relpersistence,
but it's probably not good.

Bug introduced by commit f41872d0c, and made substantially worse by
commit 85b506bbf, which added a second such access significantly
later than the heap_close.  I doubt the first reference could fail
in a production scenario, but the second one definitely could.

Discussion: https://postgr.es/m/7b52f900-0579-cda9-ae2e-de5da17090e6@2ndQuadrant.com

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 330385ba0a69183007187c4f517f18a9bf294724..ef1abf34abbe095368125345d5fc0ed188a8cec3 100644 (file)
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -557,6 +557,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
    Oid         tableOid = RelationGetRelid(OldHeap);
    Oid         tableSpace = OldHeap->rd_rel->reltablespace;
    Oid         OIDNewHeap;
+   char        relpersistence;
    bool        is_system_catalog;
    bool        swap_toast_by_content;
    TransactionId frozenXid;
@@ -566,7 +567,8 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
    if (OidIsValid(indexOid))
        mark_index_clustered(OldHeap, indexOid, true);
 
-   /* Remember if it's a system catalog */
+   /* Remember info about rel before closing OldHeap */
+   relpersistence = OldHeap->rd_rel->relpersistence;
    is_system_catalog = IsSystemRelation(OldHeap);
 
    /* Close relcache entry, but keep lock until transaction commit */
@@ -574,7 +576,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
 
    /* Create the transient table that will receive the re-ordered data */
    OIDNewHeap = make_new_heap(tableOid, tableSpace,
-                              OldHeap->rd_rel->relpersistence,
+                              relpersistence,
                               AccessExclusiveLock);
 
    /* Copy the heap data into the new table in the desired order */
@@ -588,7 +590,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
    finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
                     swap_toast_by_content, false, true,
                     frozenXid, cutoffMulti,
-                    OldHeap->rd_rel->relpersistence);
+                    relpersistence);
 }