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);
 }