Revert changes to CONCURRENTLY that "sped up" Xmin advance

This reverts commit d9d076222f5b "VACUUM: ignore indexing operations
with CONCURRENTLY".

These changes caused indexes created with the CONCURRENTLY option to
miss heap tuples that were HOT-updated and HOT-pruned during the index
creation.  Before these changes, HOT pruning would have been prevented
by the Xmin of the transaction creating the index, but because this
change was precisely to allow the Xmin to move forward ignoring that
backend, now other backends scanning the table can prune them.  This is
not a problem for VACUUM (which requires a lock that conflicts with a
CREATE INDEX CONCURRENTLY operation), but HOT-prune can definitely
occur.  In other words, Xmin advancement was sped up, but at the cost of
corrupting the resulting index.

Regrettably, this means that the new feature in PG14 that RIC/CIC on
very large tables no longer force VACUUM to retain very old tuples goes
away.  We might try to implement it again in a later release, but for
now the risk of indexes missing tuples is too high and there's no easy
fix.

Backpatch to 14, where this change appeared.

Reported-by: Peter Slavov <[email protected]>
Diagnosys-by: Andrey Borodin <[email protected]>
Diagnosys-by: Michael Paquier <[email protected]>
Diagnosys-by: Andres Freund <[email protected]>
Discussion: https://postgr.es/m/17485-396609c6925b982d%40postgresql.org

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 907324b93ec64cbed76281ad65a52d484bde6735..9ffcdc629e6d3e6645053ba95ac02eba7432e1fc 100644 (file)
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -861,8 +861,6 @@ Indexes:
    Like any long-running transaction, <command>CREATE INDEX</command> on a
    table can affect which tuples can be removed by concurrent
    <command>VACUUM</command> on any other table.
-   Excepted from this are operations with the <literal>CONCURRENTLY</literal>
-   option for indexes that are not partial and do not index any expressions.
   </para>
 
   <para>

diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 36cb4a455b72c2553f7d2f1cb9d35ae4221130fc..336ca24b3120ea8d085dbc082d22d4c74e1d4ec0 100644 (file)
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -475,8 +475,6 @@ Indexes:
     Like any long-running transaction, <command>REINDEX</command> on a table
     can affect which tuples can be removed by concurrent
     <command>VACUUM</command> on any other table.
-    Excepted from this are operations with the <literal>CONCURRENTLY</literal>
-    option for indexes that are not partial and do not index any expressions.
    </para>
 
    <para>

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index cd58c5faf02965c0ce1823c40e9008e6cab1a180..4da53a5b3fdce95d4457fab3adb74d1297f46b88 100644 (file)
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1659,13 +1659,7 @@ TransactionIdIsActive(TransactionId xid)
  * relations that's not required, since only backends in my own database could
  * ever see the tuples in them. Also, we can ignore concurrently running lazy
  * VACUUMs because (a) they must be working on other tables, and (b) they
- * don't need to do snapshot-based lookups.  Similarly, for the non-catalog
- * horizon, we can ignore CREATE INDEX CONCURRENTLY and REINDEX CONCURRENTLY
- * when they are working on non-partial, non-expressional indexes, for the
- * same reasons and because they can't run in transaction blocks.  (They are
- * not possible to ignore for catalogs, because CIC and RC do some catalog
- * operations.)  Do note that this means that CIC and RC must use a lock level
- * that conflicts with VACUUM.
+ * don't need to do snapshot-based lookups.
  *
  * This also computes a horizon used to truncate pg_subtrans. For that
  * backends in all databases have to be considered, and concurrently running
@@ -1715,6 +1709,9 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
    bool        in_recovery = RecoveryInProgress();
    TransactionId *other_xids = ProcGlobal->xids;
 
+   /* inferred after ProcArrayLock is released */
+   h->catalog_oldest_nonremovable = InvalidTransactionId;
+
    LWLockAcquire(ProcArrayLock, LW_SHARED);
 
    h->latest_completed = ShmemVariableCache->latestCompletedXid;
@@ -1734,7 +1731,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 
        h->oldest_considered_running = initial;
        h->shared_oldest_nonremovable = initial;
-       h->catalog_oldest_nonremovable = initial;
        h->data_oldest_nonremovable = initial;
 
        /*
@@ -1833,25 +1829,11 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
            (statusFlags & PROC_AFFECTS_ALL_HORIZONS) ||
            in_recovery)
        {
-           /*
-            * We can ignore this backend if it's running CREATE INDEX
-            * CONCURRENTLY or REINDEX CONCURRENTLY on a "safe" index -- but
-            * only on vacuums of user-defined tables.
-            */
-           if (!(statusFlags & PROC_IN_SAFE_IC))
-               h->data_oldest_nonremovable =
-                   TransactionIdOlder(h->data_oldest_nonremovable, xmin);
-
-           /* Catalog tables need to consider all backends in this db */
-           h->catalog_oldest_nonremovable =
-               TransactionIdOlder(h->catalog_oldest_nonremovable, xmin);
+           h->data_oldest_nonremovable =
+               TransactionIdOlder(h->data_oldest_nonremovable, xmin);
        }
    }
 
-   /* catalog horizon should never be later than data */
-   Assert(TransactionIdPrecedesOrEquals(h->catalog_oldest_nonremovable,
-                                        h->data_oldest_nonremovable));
-
    /*
     * If in recovery fetch oldest xid in KnownAssignedXids, will be applied
     * after lock is released.
@@ -1873,8 +1855,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
            TransactionIdOlder(h->shared_oldest_nonremovable, kaxmin);
        h->data_oldest_nonremovable =
            TransactionIdOlder(h->data_oldest_nonremovable, kaxmin);
-       h->catalog_oldest_nonremovable =
-           TransactionIdOlder(h->catalog_oldest_nonremovable, kaxmin);
        /* temp relations cannot be accessed in recovery */
    }
    else
@@ -1901,9 +1881,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
        h->data_oldest_nonremovable =
            TransactionIdRetreatedBy(h->data_oldest_nonremovable,
                                     vacuum_defer_cleanup_age);
-       h->catalog_oldest_nonremovable =
-           TransactionIdRetreatedBy(h->catalog_oldest_nonremovable,
-                                    vacuum_defer_cleanup_age);
        /* defer doesn't apply to temp relations */
    }
 
@@ -1926,9 +1903,7 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
    h->shared_oldest_nonremovable =
        TransactionIdOlder(h->shared_oldest_nonremovable,
                           h->slot_catalog_xmin);
-   h->catalog_oldest_nonremovable =
-       TransactionIdOlder(h->catalog_oldest_nonremovable,
-                          h->slot_xmin);
+   h->catalog_oldest_nonremovable = h->data_oldest_nonremovable;
    h->catalog_oldest_nonremovable =
        TransactionIdOlder(h->catalog_oldest_nonremovable,
                           h->slot_catalog_xmin);