On 2018/10/30 4:48, Tom Lane wrote:
> I wrote:
>> Alvaro Herrera <alvhe...@2ndquadrant.com> writes:
>>> How about modifying SysScanDescData to have a memory context member,
>>> which is created by systable_beginscan and destroyed by endscan?
>
>> I think it would still have problems, in that it would affect in which
>> context index_getnext delivers its output. We could reasonably make
>> these sorts of changes in places where the entire index_beginscan/
>> index_getnext/index_endscan call structure is in one place, but that
>> doesn't apply for the systable functions.
>
> Actually, after looking more closely, index_getnext doesn't return a
> palloc'd object anyway, or at least not one that the caller is responsible
> for managing. So maybe that could work.
Instead of SysScanDescData, could we add one to IndexScanData? It's
somewhat clear that catalog scans don't leak much, but user index scans
can, as seen in this case.
In this particular case, I see that it's IndexCheckExclusion() that
invokes check_unique_or_exclusion_constraint() repeatedly for each heap
tuple after finishing building index on the heap. The latter performs
index_beginscan/endscan() for every heap tuple, but doesn't bother to
release the memory allocated for IndexScanDesc and its members.
I've tried to fix that with the attached patches.
0001 adds the ability for the callers of index_beginscan to specify a
memory context. index_beginscan_internals switches to that context before
calling ambeginscan and stores into a new field xs_scan_cxt of
IndexScanData, but maybe storing it is unnecessary.
0002 builds on that and adds the ability for the callers of
check_exclusion_or_unique_constraints to specify a memory context. Using
that, it fixes the leak by teaching IndexCheckExclusion and
ExecIndexInsertTuples to pass a memory context that's reset before calling
check_exclusion_or_unique_constraints() for the next tuple.
The following example shows that the patch works.
With HEAD:
create table foo (a int4range);
alter table foo add exclude using spgist (a with &&);
-- this consumes about 1GB
insert into foo select int4range(i,i+1) from generate_series(1, 100000) i;
alter table foo drop constraint foo_a_excl;
-- this too
alter table foo add exclude using spgist (a with &&);
Patched:
create table foo (a int4range);
alter table foo add exclude using spgist (a with &&);
-- memory consumption doesn't grow above 37MB
insert into foo select int4range(i,i+1) from generate_series(1, 100000) i;
alter table foo drop constraint foo_a_excl;
-- ditto
alter table foo add exclude using spgist (a with &&);
Thoughts?
Thanks,
Amit
>From 39900ceb569fc32946eb4106ae5db4cf0386f95a Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 30 Oct 2018 16:56:17 +0900
Subject: [PATCH v1 1/2] Allow IndexScanDesc allocation in caller-specified
context
---
src/backend/access/index/genam.c | 6 ++++--
src/backend/access/index/indexam.c | 21 +++++++++++++++------
src/backend/commands/cluster.c | 3 ++-
src/backend/executor/execIndexing.c | 3 ++-
src/backend/executor/execReplication.c | 2 +-
src/backend/executor/nodeIndexonlyscan.c | 3 ++-
src/backend/executor/nodeIndexscan.c | 6 ++++--
src/backend/utils/adt/selfuncs.c | 4 ++--
src/include/access/genam.h | 3 ++-
src/include/access/relscan.h | 6 ++++++
10 files changed, 40 insertions(+), 17 deletions(-)
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 9d08775687..0744b00d15 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -370,7 +370,8 @@ systable_beginscan(Relation heapRelation,
}
sysscan->iscan = index_beginscan(heapRelation, irel,
-
snapshot, nkeys, 0);
+
snapshot, nkeys, 0,
+
CurrentMemoryContext);
index_rescan(sysscan->iscan, key, nkeys, NULL, 0);
sysscan->scan = NULL;
}
@@ -572,7 +573,8 @@ systable_beginscan_ordered(Relation heapRelation,
}
sysscan->iscan = index_beginscan(heapRelation, indexRelation,
-
snapshot, nkeys, 0);
+
snapshot, nkeys, 0,
+
CurrentMemoryContext);
index_rescan(sysscan->iscan, key, nkeys, NULL, 0);
sysscan->scan = NULL;
diff --git a/src/backend/access/index/indexam.c
b/src/backend/access/index/indexam.c
index eade540ef5..b17ad9417b 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -125,7 +125,8 @@ do { \
static IndexScanDesc index_beginscan_internal(Relation indexRelation,
int nkeys, int norderbys,
Snapshot snapshot,
- ParallelIndexScanDesc pscan,
bool temp_snap);
+ ParallelIndexScanDesc pscan,
bool temp_snap,
+ MemoryContext scan_cxt);
/* ----------------------------------------------------------------
@@ -222,11 +223,13 @@ IndexScanDesc
index_beginscan(Relation heapRelation,
Relation indexRelation,
Snapshot snapshot,
- int nkeys, int norderbys)
+ int nkeys, int norderbys,
+ MemoryContext scan_cxt)
{
IndexScanDesc scan;
- scan = index_beginscan_internal(indexRelation, nkeys, norderbys,
snapshot, NULL, false);
+ scan = index_beginscan_internal(indexRelation, nkeys, norderbys,
snapshot,
+ NULL,
false, scan_cxt);
/*
* Save additional parameters into the scandesc. Everything else was
set
@@ -251,7 +254,8 @@ index_beginscan_bitmap(Relation indexRelation,
{
IndexScanDesc scan;
- scan = index_beginscan_internal(indexRelation, nkeys, 0, snapshot,
NULL, false);
+ scan = index_beginscan_internal(indexRelation, nkeys, 0, snapshot,
+ NULL,
false, CurrentMemoryContext);
/*
* Save additional parameters into the scandesc. Everything else was
set
@@ -268,9 +272,11 @@ index_beginscan_bitmap(Relation indexRelation,
static IndexScanDesc
index_beginscan_internal(Relation indexRelation,
int nkeys, int norderbys,
Snapshot snapshot,
- ParallelIndexScanDesc pscan,
bool temp_snap)
+ ParallelIndexScanDesc pscan,
bool temp_snap,
+ MemoryContext scan_cxt)
{
IndexScanDesc scan;
+ MemoryContext oldcxt;
RELATION_CHECKS;
CHECK_REL_PROCEDURE(ambeginscan);
@@ -286,11 +292,14 @@ index_beginscan_internal(Relation indexRelation,
/*
* Tell the AM to open a scan.
*/
+ oldcxt = MemoryContextSwitchTo(scan_cxt);
scan = indexRelation->rd_amroutine->ambeginscan(indexRelation, nkeys,
norderbys);
/* Initialize information for parallel scan. */
scan->parallel_scan = pscan;
scan->xs_temp_snap = temp_snap;
+ scan->xs_scan_cxt = scan_cxt;
+ MemoryContextSwitchTo(oldcxt);
return scan;
}
@@ -504,7 +513,7 @@ index_beginscan_parallel(Relation heaprel, Relation
indexrel, int nkeys,
snapshot = RestoreSnapshot(pscan->ps_snapshot_data);
RegisterSnapshot(snapshot);
scan = index_beginscan_internal(indexrel, nkeys, norderbys, snapshot,
- pscan,
true);
+ pscan,
true, CurrentMemoryContext);
/*
* Save additional parameters into the scandesc. Everything else was
set
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 68be470977..2c5c23a145 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -926,7 +926,8 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid
OIDOldIndex, bool verbose,
if (OldIndex != NULL && !use_sort)
{
heapScan = NULL;
- indexScan = index_beginscan(OldHeap, OldIndex, SnapshotAny, 0,
0);
+ indexScan = index_beginscan(OldHeap, OldIndex, SnapshotAny, 0,
0,
+
CurrentMemoryContext);
index_rescan(indexScan, NULL, 0, NULL, 0);
}
else
diff --git a/src/backend/executor/execIndexing.c
b/src/backend/executor/execIndexing.c
index 9927ad70e6..74d15e6b43 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -719,7 +719,8 @@ check_exclusion_or_unique_constraint(Relation heap,
Relation index,
retry:
conflict = false;
found_self = false;
- index_scan = index_beginscan(heap, index, &DirtySnapshot, indnkeyatts,
0);
+ index_scan = index_beginscan(heap, index, &DirtySnapshot, indnkeyatts,
0,
+
CurrentMemoryContext);
index_rescan(index_scan, scankeys, indnkeyatts, NULL, 0);
while ((tup = index_getnext(index_scan,
diff --git a/src/backend/executor/execReplication.c
b/src/backend/executor/execReplication.c
index 25ba93e03c..432785c7d1 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -132,7 +132,7 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
InitDirtySnapshot(snap);
scan = index_beginscan(rel, idxrel, &snap,
IndexRelationGetNumberOfKeyAttributes(idxrel),
- 0);
+ 0, CurrentMemoryContext);
/* Build scan key. */
build_replindex_scan_key(skey, rel, idxrel, searchslot);
diff --git a/src/backend/executor/nodeIndexonlyscan.c
b/src/backend/executor/nodeIndexonlyscan.c
index daedf342f7..8b654a9eee 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -91,7 +91,8 @@ IndexOnlyNext(IndexOnlyScanState *node)
node->ioss_RelationDesc,
estate->es_snapshot,
node->ioss_NumScanKeys,
-
node->ioss_NumOrderByKeys);
+
node->ioss_NumOrderByKeys,
+
CurrentMemoryContext);
node->ioss_ScanDesc = scandesc;
diff --git a/src/backend/executor/nodeIndexscan.c
b/src/backend/executor/nodeIndexscan.c
index ba7821b0e2..750b455e66 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -114,7 +114,8 @@ IndexNext(IndexScanState *node)
node->iss_RelationDesc,
estate->es_snapshot,
node->iss_NumScanKeys,
-
node->iss_NumOrderByKeys);
+
node->iss_NumOrderByKeys,
+
CurrentMemoryContext);
node->iss_ScanDesc = scandesc;
@@ -220,7 +221,8 @@ IndexNextWithReorder(IndexScanState *node)
node->iss_RelationDesc,
estate->es_snapshot,
node->iss_NumScanKeys,
-
node->iss_NumOrderByKeys);
+
node->iss_NumOrderByKeys,
+
CurrentMemoryContext);
node->iss_ScanDesc = scandesc;
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index e0ece74bb9..16f7cc3641 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -5582,7 +5582,7 @@ get_actual_variable_range(PlannerInfo *root,
VariableStatData *vardata,
*/
index_scan = index_beginscan(heapRel, indexRel,
&SnapshotNonVacuumable,
-
1, 0);
+
1, 0, CurrentMemoryContext);
index_rescan(index_scan, scankeys, 1, NULL, 0);
/* Fetch first tuple in sortop's direction */
@@ -5615,7 +5615,7 @@ get_actual_variable_range(PlannerInfo *root,
VariableStatData *vardata,
{
index_scan = index_beginscan(heapRel, indexRel,
&SnapshotNonVacuumable,
-
1, 0);
+
1, 0, CurrentMemoryContext);
index_rescan(index_scan, scankeys, 1, NULL, 0);
/* Fetch first tuple in reverse direction */
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index 534fac7bf2..676eb2b752 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -140,7 +140,8 @@ extern bool index_insert(Relation indexRelation,
extern IndexScanDesc index_beginscan(Relation heapRelation,
Relation indexRelation,
Snapshot snapshot,
- int nkeys, int norderbys);
+ int nkeys, int norderbys,
+ MemoryContext scan_cxt);
extern IndexScanDesc index_beginscan_bitmap(Relation indexRelation,
Snapshot snapshot,
int nkeys);
diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h
index e5289b8aa7..169435d746 100644
--- a/src/include/access/relscan.h
+++ b/src/include/access/relscan.h
@@ -139,6 +139,12 @@ typedef struct IndexScanDescData
/* parallel index scan information, in shared memory */
ParallelIndexScanDesc parallel_scan;
+
+ /*
+ * The context under which memory for this struct and its members are
+ * allocated.
+ */
+ MemoryContext xs_scan_cxt;
} IndexScanDescData;
/* Generic structure for parallel scans */
--
2.11.0
>From 9252ae65eee9b9718bd62e5804a7783b4b6b9000 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 30 Oct 2018 17:53:07 +0900
Subject: [PATCH v1 2/2] Add a MemoryContext arg to
check_exclusion_or_unique_constraint
check_exclusion_or_unique_constraint performs an index scan and some
index AMs may use huge amounts of memory that might end up getting
allocated in a possibly long-lived context. A given caller may call
check_exclusion_or_unique_constraint repeatedly as it's scanning a
heap, for example, where it's desirable to free the memory for each
new tuple.
With that in place, teach IndexCheckExclusion and
ExecIndexInsertTuples to pass a per-tuple context down to
check_exclusion_or_unique_constraint.
---
src/backend/catalog/index.c | 12 +++++++++++-
src/backend/commands/constraint.c | 2 +-
src/backend/executor/execIndexing.c | 21 ++++++++++++++-------
src/include/executor/executor.h | 3 ++-
4 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5885899c9b..ccafceb73a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2983,6 +2983,7 @@ IndexCheckExclusion(Relation heapRelation,
EState *estate;
ExprContext *econtext;
Snapshot snapshot;
+ MemoryContext index_scan_cxt;
/*
* If we are reindexing the target index, mark it as no longer being
@@ -3017,11 +3018,20 @@ IndexCheckExclusion(Relation heapRelation,
true, /*
buffer access strategy OK */
true); /*
syncscan OK */
+ /*
+ * This context contains the allocations needed to scan the index
+ * for a given heap tuple, reset for every tuple.
+ */
+ index_scan_cxt = AllocSetContextCreate(CurrentMemoryContext,
+
"IndexCheckExclusion_cxt",
+
ALLOCSET_DEFAULT_SIZES);
+
while ((heapTuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
CHECK_FOR_INTERRUPTS();
MemoryContextReset(econtext->ecxt_per_tuple_memory);
+ MemoryContextReset(index_scan_cxt);
/* Set up for predicate or expression evaluation */
ExecStoreHeapTuple(heapTuple, slot, false);
@@ -3050,7 +3060,7 @@ IndexCheckExclusion(Relation heapRelation,
check_exclusion_constraint(heapRelation,
indexRelation, indexInfo,
&(heapTuple->t_self), values, isnull,
- estate,
true);
+ estate,
true, index_scan_cxt);
}
heap_endscan(scan);
diff --git a/src/backend/commands/constraint.c
b/src/backend/commands/constraint.c
index f472355b48..a4b1e4c112 100644
--- a/src/backend/commands/constraint.c
+++ b/src/backend/commands/constraint.c
@@ -178,7 +178,7 @@ unique_key_recheck(PG_FUNCTION_ARGS)
*/
check_exclusion_constraint(trigdata->tg_relation, indexRel,
indexInfo,
&tmptid,
values, isnull,
- estate,
false);
+ estate,
false, CurrentMemoryContext);
}
/*
diff --git a/src/backend/executor/execIndexing.c
b/src/backend/executor/execIndexing.c
index 74d15e6b43..cc1f5ffc39 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -129,7 +129,8 @@ static bool check_exclusion_or_unique_constraint(Relation
heap, Relation index,
EState
*estate, bool newIndex,
CEOUC_WAIT_MODE waitMode,
bool
errorOK,
-
ItemPointer conflictTid);
+
ItemPointer conflictTid,
+
MemoryContext index_scan_cxt);
static bool index_recheck_constraint(Relation index, Oid *constr_procs,
Datum *existing_values, bool
*existing_isnull,
@@ -410,6 +411,7 @@ ExecInsertIndexTuples(TupleTableSlot *slot,
{
bool violationOK;
CEOUC_WAIT_MODE waitMode;
+ ExprContext *econtext = GetPerTupleExprContext(estate);
if (applyNoDupErr)
{
@@ -432,7 +434,8 @@ ExecInsertIndexTuples(TupleTableSlot *slot,
indexRelation, indexInfo,
tupleid, values, isnull,
estate, false,
-
waitMode, violationOK, NULL);
+
waitMode, violationOK, NULL,
+
econtext->ecxt_per_tuple_memory);
}
if ((checkUnique == UNIQUE_CHECK_PARTIAL ||
@@ -582,7 +585,8 @@ ExecCheckIndexConstraints(TupleTableSlot *slot,
indexInfo, &invalidItemPtr,
values, isnull, estate, false,
CEOUC_WAIT, true,
-
conflictTid);
+
conflictTid,
+
CurrentMemoryContext);
if (!satisfiesConstraint)
return false;
}
@@ -643,7 +647,8 @@ check_exclusion_or_unique_constraint(Relation heap,
Relation index,
EState
*estate, bool newIndex,
CEOUC_WAIT_MODE waitMode,
bool
violationOK,
-
ItemPointer conflictTid)
+
ItemPointer conflictTid,
+
MemoryContext index_scan_cxt)
{
Oid *constr_procs;
uint16 *constr_strats;
@@ -720,7 +725,7 @@ retry:
conflict = false;
found_self = false;
index_scan = index_beginscan(heap, index, &DirtySnapshot, indnkeyatts,
0,
-
CurrentMemoryContext);
+
index_scan_cxt);
index_rescan(index_scan, scankeys, indnkeyatts, NULL, 0);
while ((tup = index_getnext(index_scan,
@@ -865,12 +870,14 @@ check_exclusion_constraint(Relation heap, Relation index,
IndexInfo *indexInfo,
ItemPointer tupleid,
Datum *values, bool *isnull,
- EState *estate, bool
newIndex)
+ EState *estate, bool
newIndex,
+ MemoryContext index_scan_cxt)
{
(void) check_exclusion_or_unique_constraint(heap, index, indexInfo,
tupleid,
values, isnull,
estate, newIndex,
-
CEOUC_WAIT, false, NULL);
+
CEOUC_WAIT, false, NULL,
+
index_scan_cxt);
}
/*
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index edf538365b..c5998d9885 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -556,7 +556,8 @@ extern void check_exclusion_constraint(Relation heap,
Relation index,
IndexInfo *indexInfo,
ItemPointer tupleid,
Datum *values, bool *isnull,
- EState *estate, bool
newIndex);
+ EState *estate, bool
newIndex,
+ MemoryContext
index_scan_cxt);
/*
* prototypes from functions in execReplication.c
--
2.11.0
