public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 3/8] Process all scan keys in existing BRIN opclasses
2+ messages / 2 participants
[nested] [flat]
* [PATCH 3/8] Process all scan keys in existing BRIN opclasses
@ 2021-03-04 23:45 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Tomas Vondra @ 2021-03-04 23:45 UTC (permalink / raw)
This modifies the existing BRIN opclasses (minmax, inclusion) although
those don't really benefit from this change. The primary purpose of this
is to allow more advanced opclases in the future.
---
src/backend/access/brin/brin_inclusion.c | 140 ++++++++++++++++-------
src/backend/access/brin/brin_minmax.c | 92 ++++++++++++---
src/include/catalog/pg_proc.dat | 4 +-
3 files changed, 177 insertions(+), 59 deletions(-)
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 12e5bddd1f..a260074c91 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -85,6 +85,8 @@ static FmgrInfo *inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno,
uint16 procnum);
static FmgrInfo *inclusion_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno,
Oid subtype, uint16 strategynum);
+static bool inclusion_consistent_key(BrinDesc *bdesc, BrinValues *column,
+ ScanKey key, Oid colloid);
/*
@@ -251,6 +253,10 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
/*
* BRIN inclusion consistent function
*
+ * We inspect the IS NULL scan keys first, which allows us to make a decision
+ * without looking at the contents of the page range. Only when the page range
+ * matches all those keys, we check the regular scan keys.
+ *
* All of the strategies are optional.
*/
Datum
@@ -258,24 +264,31 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
{
BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
- ScanKey key = (ScanKey) PG_GETARG_POINTER(2);
- Oid colloid = PG_GET_COLLATION(),
- subtype;
- Datum unionval;
- AttrNumber attno;
- Datum query;
- FmgrInfo *finfo;
- Datum result;
-
- Assert(key->sk_attno == column->bv_attno);
+ ScanKey *keys = (ScanKey *) PG_GETARG_POINTER(2);
+ int nkeys = PG_GETARG_INT32(3);
+ Oid colloid = PG_GET_COLLATION();
+ int keyno;
+ bool has_regular_keys = false;
- /* Handle IS NULL/IS NOT NULL tests. */
- if (key->sk_flags & SK_ISNULL)
+ /* Handle IS NULL/IS NOT NULL tests */
+ for (keyno = 0; keyno < nkeys; keyno++)
{
+ ScanKey key = keys[keyno];
+
+ Assert(key->sk_attno == column->bv_attno);
+
+ /* Skip regular scan keys (and remember that we have some). */
+ if ((!key->sk_flags & SK_ISNULL))
+ {
+ has_regular_keys = true;
+ continue;
+ }
+
if (key->sk_flags & SK_SEARCHNULL)
{
if (column->bv_allnulls || column->bv_hasnulls)
- PG_RETURN_BOOL(true);
+ continue; /* this key is fine, continue */
+
PG_RETURN_BOOL(false);
}
@@ -284,7 +297,12 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
* only nulls.
*/
if (key->sk_flags & SK_SEARCHNOTNULL)
- PG_RETURN_BOOL(!column->bv_allnulls);
+ {
+ if (column->bv_allnulls)
+ PG_RETURN_BOOL(false);
+
+ continue;
+ }
/*
* Neither IS NULL nor IS NOT NULL was used; assume all indexable
@@ -293,7 +311,14 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(false);
}
- /* If it is all nulls, it cannot possibly be consistent. */
+ /* If there are no regular keys, the page range is considered consistent. */
+ if (!has_regular_keys)
+ PG_RETURN_BOOL(true);
+
+ /*
+ * If is all nulls, it cannot possibly be consistent (at this point we
+ * know there are at least some regular scan keys).
+ */
if (column->bv_allnulls)
PG_RETURN_BOOL(false);
@@ -301,10 +326,45 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
if (DatumGetBool(column->bv_values[INCLUSION_UNMERGEABLE]))
PG_RETURN_BOOL(true);
- attno = key->sk_attno;
- subtype = key->sk_subtype;
- query = key->sk_argument;
- unionval = column->bv_values[INCLUSION_UNION];
+ /* Check that the range is consistent with all regular scan keys. */
+ for (keyno = 0; keyno < nkeys; keyno++)
+ {
+ ScanKey key = keys[keyno];
+
+ /* Skip IS NULL/IS NOT NULL keys (already handled above). */
+ if (key->sk_flags & SK_ISNULL)
+ continue;
+
+ /*
+ * When there are multiple scan keys, failure to meet the criteria for
+ * a single one of them is enough to discard the range as a whole, so
+ * break out of the loop as soon as a false return value is obtained.
+ */
+ if (!inclusion_consistent_key(bdesc, column, key, colloid))
+ PG_RETURN_BOOL(false);
+ }
+
+ PG_RETURN_BOOL(true);
+}
+
+/*
+ * inclusion_consistent_key
+ * Determine if the range is consistent with a single scan key.
+ */
+static bool
+inclusion_consistent_key(BrinDesc *bdesc, BrinValues *column, ScanKey key,
+ Oid colloid)
+{
+ FmgrInfo *finfo;
+ AttrNumber attno = key->sk_attno;
+ Oid subtype = key->sk_subtype;
+ Datum query = key->sk_argument;
+ Datum unionval = column->bv_values[INCLUSION_UNION];
+ Datum result;
+
+ /* This should be called only for regular keys, not for IS [NOT] NULL. */
+ Assert(!(key->sk_flags & SK_ISNULL));
+
switch (key->sk_strategy)
{
/*
@@ -324,49 +384,49 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTOverRightStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_BOOL(!DatumGetBool(result));
+ return !DatumGetBool(result);
case RTOverLeftStrategyNumber:
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTRightStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_BOOL(!DatumGetBool(result));
+ return !DatumGetBool(result);
case RTOverRightStrategyNumber:
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTLeftStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_BOOL(!DatumGetBool(result));
+ return !DatumGetBool(result);
case RTRightStrategyNumber:
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTOverLeftStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_BOOL(!DatumGetBool(result));
+ return !DatumGetBool(result);
case RTBelowStrategyNumber:
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTOverAboveStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_BOOL(!DatumGetBool(result));
+ return !DatumGetBool(result);
case RTOverBelowStrategyNumber:
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTAboveStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_BOOL(!DatumGetBool(result));
+ return !DatumGetBool(result);
case RTOverAboveStrategyNumber:
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTBelowStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_BOOL(!DatumGetBool(result));
+ return !DatumGetBool(result);
case RTAboveStrategyNumber:
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTOverBelowStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_BOOL(!DatumGetBool(result));
+ return !DatumGetBool(result);
/*
* Overlap and contains strategies
@@ -384,7 +444,7 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
key->sk_strategy);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_DATUM(result);
+ return DatumGetBool(result);
/*
* Contained by strategies
@@ -404,9 +464,9 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
RTOverlapStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
if (DatumGetBool(result))
- PG_RETURN_BOOL(true);
+ return true;
- PG_RETURN_DATUM(column->bv_values[INCLUSION_CONTAINS_EMPTY]);
+ return DatumGetBool(column->bv_values[INCLUSION_CONTAINS_EMPTY]);
/*
* Adjacent strategy
@@ -423,12 +483,12 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
RTOverlapStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
if (DatumGetBool(result))
- PG_RETURN_BOOL(true);
+ return true;
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTAdjacentStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_DATUM(result);
+ return DatumGetBool(result);
/*
* Basic comparison strategies
@@ -458,9 +518,9 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
RTRightStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
if (!DatumGetBool(result))
- PG_RETURN_BOOL(true);
+ return true;
- PG_RETURN_DATUM(column->bv_values[INCLUSION_CONTAINS_EMPTY]);
+ return DatumGetBool(column->bv_values[INCLUSION_CONTAINS_EMPTY]);
case RTSameStrategyNumber:
case RTEqualStrategyNumber:
@@ -468,30 +528,30 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
RTContainsStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
if (DatumGetBool(result))
- PG_RETURN_BOOL(true);
+ return true;
- PG_RETURN_DATUM(column->bv_values[INCLUSION_CONTAINS_EMPTY]);
+ return DatumGetBool(column->bv_values[INCLUSION_CONTAINS_EMPTY]);
case RTGreaterEqualStrategyNumber:
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTLeftStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
if (!DatumGetBool(result))
- PG_RETURN_BOOL(true);
+ return true;
- PG_RETURN_DATUM(column->bv_values[INCLUSION_CONTAINS_EMPTY]);
+ return DatumGetBool(column->bv_values[INCLUSION_CONTAINS_EMPTY]);
case RTGreaterStrategyNumber:
/* no need to check for empty elements */
finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype,
RTLeftStrategyNumber);
result = FunctionCall2Coll(finfo, colloid, unionval, query);
- PG_RETURN_BOOL(!DatumGetBool(result));
+ return !DatumGetBool(result);
default:
/* shouldn't happen */
elog(ERROR, "invalid strategy number %d", key->sk_strategy);
- PG_RETURN_BOOL(false);
+ return false;
}
}
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 2ffbd9bf0d..e116084a02 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -30,6 +30,8 @@ typedef struct MinmaxOpaque
static FmgrInfo *minmax_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno,
Oid subtype, uint16 strategynum);
+static bool minmax_consistent_key(BrinDesc *bdesc, BrinValues *column,
+ ScanKey key, Oid colloid);
Datum
@@ -140,29 +142,41 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
* Given an index tuple corresponding to a certain page range and a scan key,
* return whether the scan key is consistent with the index tuple's min/max
* values. Return true if so, false otherwise.
+ *
+ * We inspect the IS NULL scan keys first, which allows us to make a decision
+ * without looking at the contents of the page range. Only when the page range
+ * matches all those keys, we check the regular scan keys.
*/
Datum
brin_minmax_consistent(PG_FUNCTION_ARGS)
{
BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
- ScanKey key = (ScanKey) PG_GETARG_POINTER(2);
- Oid colloid = PG_GET_COLLATION(),
- subtype;
- AttrNumber attno;
- Datum value;
- Datum matches;
- FmgrInfo *finfo;
-
- Assert(key->sk_attno == column->bv_attno);
+ ScanKey *keys = (ScanKey *) PG_GETARG_POINTER(2);
+ int nkeys = PG_GETARG_INT32(3);
+ Oid colloid = PG_GET_COLLATION();
+ int keyno;
+ bool has_regular_keys = false;
/* handle IS NULL/IS NOT NULL tests */
- if (key->sk_flags & SK_ISNULL)
+ for (keyno = 0; keyno < nkeys; keyno++)
{
+ ScanKey key = keys[keyno];
+
+ Assert(key->sk_attno == column->bv_attno);
+
+ /* Skip regular scan keys (and remember that we have some). */
+ if ((!key->sk_flags & SK_ISNULL))
+ {
+ has_regular_keys = true;
+ continue;
+ }
+
if (key->sk_flags & SK_SEARCHNULL)
{
if (column->bv_allnulls || column->bv_hasnulls)
- PG_RETURN_BOOL(true);
+ continue; /* this key is fine, continue */
+
PG_RETURN_BOOL(false);
}
@@ -171,7 +185,12 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
* only nulls.
*/
if (key->sk_flags & SK_SEARCHNOTNULL)
- PG_RETURN_BOOL(!column->bv_allnulls);
+ {
+ if (column->bv_allnulls)
+ PG_RETURN_BOOL(false);
+
+ continue;
+ }
/*
* Neither IS NULL nor IS NOT NULL was used; assume all indexable
@@ -180,13 +199,52 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(false);
}
- /* if the range is all empty, it cannot possibly be consistent */
+ /* If there are no regular keys, the page range is considered consistent. */
+ if (!has_regular_keys)
+ PG_RETURN_BOOL(true);
+
+ /*
+ * If is all nulls, it cannot possibly be consistent (at this point we
+ * know there are at least some regular scan keys).
+ */
if (column->bv_allnulls)
PG_RETURN_BOOL(false);
- attno = key->sk_attno;
- subtype = key->sk_subtype;
- value = key->sk_argument;
+ /* Check that the range is consistent with all scan keys. */
+ for (keyno = 0; keyno < nkeys; keyno++)
+ {
+ ScanKey key = keys[keyno];
+
+ /* ignore IS NULL/IS NOT NULL tests handled above */
+ if (key->sk_flags & SK_ISNULL)
+ continue;
+
+ /*
+ * When there are multiple scan keys, failure to meet the criteria for
+ * a single one of them is enough to discard the range as a whole, so
+ * break out of the loop as soon as a false return value is obtained.
+ */
+ if (!minmax_consistent_key(bdesc, column, key, colloid))
+ PG_RETURN_DATUM(false);;
+ }
+
+ PG_RETURN_DATUM(true);
+}
+
+/*
+ * minmax_consistent_key
+ * Determine if the range is consistent with a single scan key.
+ */
+static bool
+minmax_consistent_key(BrinDesc *bdesc, BrinValues *column, ScanKey key,
+ Oid colloid)
+{
+ FmgrInfo *finfo;
+ AttrNumber attno = key->sk_attno;
+ Oid subtype = key->sk_subtype;
+ Datum value = key->sk_argument;
+ Datum matches;
+
switch (key->sk_strategy)
{
case BTLessStrategyNumber:
@@ -229,7 +287,7 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
break;
}
- PG_RETURN_DATUM(matches);
+ return DatumGetBool(matches);
}
/*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 506689d8ac..ad11a2b66f 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8206,7 +8206,7 @@
prosrc => 'brin_minmax_add_value' },
{ oid => '3385', descr => 'BRIN minmax support',
proname => 'brin_minmax_consistent', prorettype => 'bool',
- proargtypes => 'internal internal internal',
+ proargtypes => 'internal internal internal int4',
prosrc => 'brin_minmax_consistent' },
{ oid => '3386', descr => 'BRIN minmax support',
proname => 'brin_minmax_union', prorettype => 'bool',
@@ -8222,7 +8222,7 @@
prosrc => 'brin_inclusion_add_value' },
{ oid => '4107', descr => 'BRIN inclusion support',
proname => 'brin_inclusion_consistent', prorettype => 'bool',
- proargtypes => 'internal internal internal',
+ proargtypes => 'internal internal internal int4',
prosrc => 'brin_inclusion_consistent' },
{ oid => '4108', descr => 'BRIN inclusion support',
proname => 'brin_inclusion_union', prorettype => 'bool',
--
2.26.2
--------------556A1DC61262AF15641DB204
Content-Type: text/x-patch; charset=UTF-8;
name="0004-Move-IS-NOT-NULL-handling-from-BRIN-suppor-20210308b.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0004-Move-IS-NOT-NULL-handling-from-BRIN-suppor-20210308b.pa";
filename*1="tch"
^ permalink raw reply [nested|flat] 2+ messages in thread
* Memory leak in incremental sort re-scan
@ 2023-06-15 11:48 Laurenz Albe <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Laurenz Albe @ 2023-06-15 11:48 UTC (permalink / raw)
To: [email protected]
ExecIncrementalSort() calls tuplesort_begin_common(), which creates the "TupleSort main"
and "TupleSort sort" memory contexts, and ExecEndIncrementalSort() calls tuplesort_end(),
which destroys them.
But ExecReScanIncrementalSort() only resets the memory contexts. Since the next call to
ExecIncrementalSort() will create them again, we end up leaking these contexts for every
re-scan.
Here is a reproducer with the regression test database:
SET enable_sort = off;
SET enable_hashjoin = off;
SET enable_mergejoin = off;
SET enable_material = off;
SELECT t.unique2, t2.r
FROM tenk1 AS t
JOIN (SELECT unique1,
row_number() OVER (ORDER BY hundred, thousand) AS r
FROM tenk1
OFFSET 0) AS t2
ON t.unique1 + 0 = t2.unique1
WHERE t.unique1 < 1000;
The execution plan:
Nested Loop
Join Filter: ((t.unique1 + 0) = tenk1.unique1)
-> Bitmap Heap Scan on tenk1 t
Recheck Cond: (unique1 < 1000)
-> Bitmap Index Scan on tenk1_unique1
Index Cond: (unique1 < 1000)
-> WindowAgg
-> Incremental Sort
Sort Key: tenk1.hundred, tenk1.thousand
Presorted Key: tenk1.hundred
-> Index Scan using tenk1_hundred on tenk1
A memory context dump at the end of the execution looks like this:
ExecutorState: 262144 total in 6 blocks; 74136 free (29 chunks); 188008 used
TupleSort main: 32832 total in 2 blocks; 7320 free (0 chunks); 25512 used
TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0 chunks); 208 used
TupleSort main: 32832 total in 2 blocks; 7256 free (0 chunks); 25576 used
TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0 chunks); 208 used
TupleSort main: 32832 total in 2 blocks; 7320 free (0 chunks); 25512 used
TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0 chunks); 208 used
[many more]
1903 more child contexts containing 93452928 total in 7597 blocks; 44073240 free (0 chunks); 49379688 used
The following patch fixes the problem for me:
--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -1145,21 +1145,16 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
node->execution_status = INCSORT_LOADFULLSORT;
/*
- * If we've set up either of the sort states yet, we need to reset them.
- * We could end them and null out the pointers, but there's no reason to
- * repay the setup cost, and because ExecIncrementalSort guards presorted
- * column functions by checking to see if the full sort state has been
- * initialized yet, setting the sort states to null here might actually
- * cause a leak.
+ * Release tuplesort resources.
*/
if (node->fullsort_state != NULL)
{
- tuplesort_reset(node->fullsort_state);
+ tuplesort_end(node->fullsort_state);
node->fullsort_state = NULL;
}
if (node->prefixsort_state != NULL)
{
- tuplesort_reset(node->prefixsort_state);
+ tuplesort_end(node->prefixsort_state);
node->prefixsort_state = NULL;
}
The original comment hints that this might mot be the correct thing to do...
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2023-06-15 11:48 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-04 23:45 [PATCH 3/8] Process all scan keys in existing BRIN opclasses Tomas Vondra <[email protected]>
2023-06-15 11:48 Memory leak in incremental sort re-scan Laurenz Albe <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox