Fix memory leak in indexUnchanged hint mechanism.

Commit 9dc718bd added a "logically unchanged by UPDATE" hinting
mechanism, which is currently used within nbtree indexes only (see
commit d168b666).  This mechanism determined whether or not the incoming
item is a logically unchanged duplicate (a duplicate needed only for
MVCC versioning purposes) once per row updated per non-HOT update.  This
approach led to memory leaks which were noticeable with an UPDATE
statement that updated sufficiently many rows, at least on tables that
happen to have an expression index.

On HEAD, fix the issue by adding a cache to the executor's per-index
IndexInfo struct.

Take a different approach on Postgres 14 to avoid an ABI break: simply
pass down the hint to all indexes unconditionally with non-HOT UPDATEs.
This is deemed acceptable because the hint is currently interpreted
within btinsert() as "perform a bottom-up index deletion pass if and
when the only alternative is splitting the leaf page -- prefer to delete
any LP_DEAD-set items first".  nbtree must always treat the hint as a
noisy signal about what might work, as a strategy of last resort, with
costs imposed on non-HOT updaters.  (The same thing might not be true
within another index AM that applies the hint, which is why the original
behavior is preserved on HEAD.)

Author: Peter Geoghegan <pg@bowt.ie>
Reported-By: Klaudie Willis <Klaudie.Willis@protonmail.com>
Diagnosed-By: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/261065.1639497535@sss.pgh.pa.us
Backpatch: 14-, where the hinting mechanism was added.

diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 917bb3577e77e5a996e126e0ba9ffc22875fba47..3c27cb1e71befbe59825a5a587fb99d88031045f 100644 (file)
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -302,6 +302,8 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
    indexInfo->ii_OpclassOptions = NULL;
    indexInfo->ii_Unique = true;
    indexInfo->ii_ReadyForInserts = true;
+   indexInfo->ii_CheckedUnchanged = false;
+   indexInfo->ii_IndexUnchanged = false;
    indexInfo->ii_Concurrent = false;
    indexInfo->ii_BrokenHotChain = false;
    indexInfo->ii_ParallelWorkers = 0;

diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 22eda9449f91f7bb023c10016650d5c69005a65f..0cb0b8f1111060900c263a136f9ea30b38439081 100644 (file)
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -935,19 +935,32 @@ static bool
 index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate,
                          IndexInfo *indexInfo, Relation indexRelation)
 {
-   Bitmapset  *updatedCols = ExecGetUpdatedCols(resultRelInfo, estate);
-   Bitmapset  *extraUpdatedCols = ExecGetExtraUpdatedCols(resultRelInfo, estate);
+   Bitmapset  *updatedCols;
+   Bitmapset  *extraUpdatedCols;
    Bitmapset  *allUpdatedCols;
    bool        hasexpression = false;
    List       *idxExprs;
 
+   /*
+    * Check cache first
+    */
+   if (indexInfo->ii_CheckedUnchanged)
+       return indexInfo->ii_IndexUnchanged;
+   indexInfo->ii_CheckedUnchanged = true;
+
    /*
     * Check for indexed attribute overlap with updated columns.
     *
     * Only do this for key columns.  A change to a non-key column within an
     * INCLUDE index should not be counted here.  Non-key column values are
     * opaque payload state to the index AM, a little like an extra table TID.
+    *
+    * Note that row-level BEFORE triggers won't affect our behavior, since
+    * they don't affect the updatedCols bitmaps generally.  It doesn't seem
+    * worth the trouble of checking which attributes were changed directly.
     */
+   updatedCols = ExecGetUpdatedCols(resultRelInfo, estate);
+   extraUpdatedCols = ExecGetExtraUpdatedCols(resultRelInfo, estate);
    for (int attr = 0; attr < indexInfo->ii_NumIndexKeyAttrs; attr++)
    {
        int         keycol = indexInfo->ii_IndexAttrNumbers[attr];
@@ -968,6 +981,7 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate,
                          extraUpdatedCols))
        {
            /* Changed key column -- don't hint for this index */
+           indexInfo->ii_IndexUnchanged = false;
            return false;
        }
    }
@@ -981,7 +995,10 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate,
     * shows that the index as a whole is logically unchanged by UPDATE.
     */
    if (!hasexpression)
+   {
+       indexInfo->ii_IndexUnchanged = true;
        return true;
+   }
 
    /*
     * Need to pass only one bms to expression_tree_walker helper function.
@@ -1008,8 +1025,18 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate,
        bms_free(allUpdatedCols);
 
    if (hasexpression)
+   {
+       indexInfo->ii_IndexUnchanged = false;
        return false;
+   }
 
+   /*
+    * Deliberately don't consider index predicates.  We should even give the
+    * hint when result rel's "updated tuple" has no corresponding index
+    * tuple, which is possible with a partial index (provided the usual
+    * conditions are met).
+    */
+   indexInfo->ii_IndexUnchanged = true;
    return true;
 }
 

diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index dace4f7e2dfe2e50cc1b7031a13011f842a33adc..822395625b6bc4c0b5224ccf8f93d2fc554af035 100644 (file)
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -751,6 +751,8 @@ makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions,
    Assert(n->ii_NumIndexKeyAttrs <= n->ii_NumIndexAttrs);
    n->ii_Unique = unique;
    n->ii_ReadyForInserts = isready;
+   n->ii_CheckedUnchanged = false;
+   n->ii_IndexUnchanged = false;
    n->ii_Concurrent = concurrent;
 
    /* expressions */

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 8429a9c55dfc58018ef427e47fe1361b14d83cfa..4ea8735dd81c1bcdc5ca4a18c53486eeb3f2e843 100644 (file)
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -142,6 +142,8 @@ typedef struct ExprState
  *     Unique              is it a unique index?
  *     OpclassOptions      opclass-specific options, or NULL if none
  *     ReadyForInserts     is it valid for inserts?
+ *     CheckedUnchanged    IndexUnchanged status determined yet?
+ *     IndexUnchanged      aminsert hint, cached for retail inserts
  *     Concurrent          are we doing a concurrent index build?
  *     BrokenHotChain      did we detect any broken HOT chains?
  *     ParallelWorkers     # of workers requested (excludes leader)
@@ -172,6 +174,8 @@ typedef struct IndexInfo
    Datum      *ii_OpclassOptions;  /* array with one entry per column */
    bool        ii_Unique;
    bool        ii_ReadyForInserts;
+   bool        ii_CheckedUnchanged;
+   bool        ii_IndexUnchanged;
    bool        ii_Concurrent;
    bool        ii_BrokenHotChain;
    int         ii_ParallelWorkers;