public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 3/8] Disallow compressed data inside container types
6+ messages / 3 participants
[nested] [flat]
* [PATCH 3/8] Disallow compressed data inside container types
@ 2021-03-04 11:03 Dilip Kumar <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Dilip Kumar @ 2021-03-04 11:03 UTC (permalink / raw)
Currently, we have a general rule that Datums of container types
(rows, arrays, ranges, etc) must not contain any external TOAST
pointers. But the rule for the compressed data is not defined
and no specific rule is followed e.g. while constructing the array
we decompress the compressed field but while constructing the row
in some cases we don't decompress the compressed data whereas in
the other cases we only decompress while flattening the external
toast pointers. This patch make a general rule for the compressed
data i.e. we don't allow the compressed data in the container type.
Dilip Kumar based on idea from Robert Haas
---
src/backend/access/common/heaptuple.c | 9 +--
src/backend/access/heap/heaptoast.c | 4 +-
src/backend/executor/execExprInterp.c | 6 +-
src/backend/executor/execTuples.c | 4 --
src/backend/utils/adt/expandedrecord.c | 76 +++++++++-----------------
src/backend/utils/adt/jsonfuncs.c | 3 +-
src/include/funcapi.h | 4 +-
src/pl/plpgsql/src/pl_exec.c | 3 +-
8 files changed, 38 insertions(+), 71 deletions(-)
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index c36c283253..eb9f016dfa 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -984,15 +984,12 @@ Datum
heap_copy_tuple_as_datum(HeapTuple tuple, TupleDesc tupleDesc)
{
/*
- * If the tuple contains any external TOAST pointers, we have to inline
- * those fields to meet the conventions for composite-type Datums.
+ * We have to inline any external/compressed data to meet the conventions
+ * for composite-type Datums.
*/
- if (HeapTupleHasExternal(tuple))
- return toast_flatten_tuple_to_datum(tuple->t_data,
+ return toast_flatten_tuple_to_datum(tuple->t_data,
tuple->t_len,
tupleDesc);
- else
- return heap_copy_tuple_as_raw_datum(tuple, tupleDesc);
}
/* ----------------
diff --git a/src/backend/access/heap/heaptoast.c b/src/backend/access/heap/heaptoast.c
index 55bbe1d584..b09462348b 100644
--- a/src/backend/access/heap/heaptoast.c
+++ b/src/backend/access/heap/heaptoast.c
@@ -589,9 +589,9 @@ toast_build_flattened_tuple(TupleDesc tupleDesc,
struct varlena *new_value;
new_value = (struct varlena *) DatumGetPointer(new_values[i]);
- if (VARATT_IS_EXTERNAL(new_value))
+ if (VARATT_IS_EXTERNAL(new_value) || VARATT_IS_COMPRESSED(new_value))
{
- new_value = detoast_external_attr(new_value);
+ new_value = detoast_attr(new_value);
new_values[i] = PointerGetDatum(new_value);
freeable_values[num_to_free++] = (Pointer) new_value;
}
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index c3754acca4..71e6f41fee 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2845,8 +2845,7 @@ ExecEvalRow(ExprState *state, ExprEvalStep *op)
{
Form_pg_attribute attr = TupleDescAttr(op->d.row.tupdesc, i);
- if (op->d.row.elemnulls[i] || attr->attlen != -1 ||
- !VARATT_IS_EXTERNAL(DatumGetPointer(op->d.row.elemvalues[i])))
+ if (op->d.row.elemnulls[i] || attr->attlen != -1)
continue;
op->d.row.elemvalues[i] =
@@ -3103,8 +3102,7 @@ ExecEvalFieldStoreForm(ExprState *state, ExprEvalStep *op, ExprContext *econtext
{
Form_pg_attribute attr = TupleDescAttr(*op->d.fieldstore.argdesc, i);
- if (op->d.fieldstore.nulls[i] || attr->attlen != -1 ||
- !VARATT_IS_EXTERNAL(DatumGetPointer(op->d.fieldstore.values[i])))
+ if (op->d.fieldstore.nulls[i] || attr->attlen != -1)
continue;
op->d.fieldstore.values[i] = PointerGetDatum(
PG_DETOAST_DATUM_PACKED(op->d.fieldstore.values[i]));
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index 73c35df9c9..f11546468e 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -2207,10 +2207,6 @@ HeapTupleHeaderGetDatum(HeapTupleHeader tuple)
Datum result;
TupleDesc tupDesc;
- /* No work if there are no external TOAST pointers in the tuple */
- if (!HeapTupleHeaderHasExternal(tuple))
- return PointerGetDatum(tuple);
-
/* Use the type data saved by heap_form_tuple to look up the rowtype */
tupDesc = lookup_rowtype_tupdesc(HeapTupleHeaderGetTypeId(tuple),
HeapTupleHeaderGetTypMod(tuple));
diff --git a/src/backend/utils/adt/expandedrecord.c b/src/backend/utils/adt/expandedrecord.c
index e19491ecf7..3cbc256671 100644
--- a/src/backend/utils/adt/expandedrecord.c
+++ b/src/backend/utils/adt/expandedrecord.c
@@ -673,14 +673,6 @@ ER_get_flat_size(ExpandedObjectHeader *eohptr)
erh->er_typmod = tupdesc->tdtypmod;
}
- /*
- * If we have a valid flattened value without out-of-line fields, we can
- * just use it as-is.
- */
- if (erh->flags & ER_FLAG_FVALUE_VALID &&
- !(erh->flags & ER_FLAG_HAVE_EXTERNAL))
- return erh->fvalue->t_len;
-
/* If we have a cached size value, believe that */
if (erh->flat_size)
return erh->flat_size;
@@ -693,38 +685,36 @@ ER_get_flat_size(ExpandedObjectHeader *eohptr)
tupdesc = erh->er_tupdesc;
/*
- * Composite datums mustn't contain any out-of-line values.
+ * Composite datums mustn't contain any out-of-line/compressed values.
*/
- if (erh->flags & ER_FLAG_HAVE_EXTERNAL)
+ for (i = 0; i < erh->nfields; i++)
{
- for (i = 0; i < erh->nfields; i++)
- {
- Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
+ Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
- if (!erh->dnulls[i] &&
- !attr->attbyval && attr->attlen == -1 &&
- VARATT_IS_EXTERNAL(DatumGetPointer(erh->dvalues[i])))
- {
- /*
- * expanded_record_set_field_internal can do the actual work
- * of detoasting. It needn't recheck domain constraints.
- */
- expanded_record_set_field_internal(erh, i + 1,
- erh->dvalues[i], false,
- true,
- false);
- }
+ if (!erh->dnulls[i] &&
+ !attr->attbyval && attr->attlen == -1 &&
+ (VARATT_IS_EXTERNAL(DatumGetPointer(erh->dvalues[i])) ||
+ VARATT_IS_COMPRESSED(DatumGetPointer(erh->dvalues[i]))))
+ {
+ /*
+ * expanded_record_set_field_internal can do the actual work
+ * of detoasting. It needn't recheck domain constraints.
+ */
+ expanded_record_set_field_internal(erh, i + 1,
+ erh->dvalues[i], false,
+ true,
+ false);
}
-
- /*
- * We have now removed all external field values, so we can clear the
- * flag about them. This won't cause ER_flatten_into() to mistakenly
- * take the fast path, since expanded_record_set_field() will have
- * cleared ER_FLAG_FVALUE_VALID.
- */
- erh->flags &= ~ER_FLAG_HAVE_EXTERNAL;
}
+ /*
+ * We have now removed all external field values, so we can clear the
+ * flag about them. This won't cause ER_flatten_into() to mistakenly
+ * take the fast path, since expanded_record_set_field() will have
+ * cleared ER_FLAG_FVALUE_VALID.
+ */
+ erh->flags &= ~ER_FLAG_HAVE_EXTERNAL;
+
/* Test if we currently have any null values */
hasnull = false;
for (i = 0; i < erh->nfields; i++)
@@ -770,19 +760,6 @@ ER_flatten_into(ExpandedObjectHeader *eohptr,
Assert(erh->er_magic == ER_MAGIC);
- /* Easy if we have a valid flattened value without out-of-line fields */
- if (erh->flags & ER_FLAG_FVALUE_VALID &&
- !(erh->flags & ER_FLAG_HAVE_EXTERNAL))
- {
- Assert(allocated_size == erh->fvalue->t_len);
- memcpy(tuphdr, erh->fvalue->t_data, allocated_size);
- /* The original flattened value might not have datum header fields */
- HeapTupleHeaderSetDatumLength(tuphdr, allocated_size);
- HeapTupleHeaderSetTypeId(tuphdr, erh->er_typeid);
- HeapTupleHeaderSetTypMod(tuphdr, erh->er_typmod);
- return;
- }
-
/* Else allocation should match previous get_flat_size result */
Assert(allocated_size == erh->flat_size);
@@ -1155,11 +1132,12 @@ expanded_record_set_field_internal(ExpandedRecordHeader *erh, int fnumber,
if (expand_external)
{
if (attr->attlen == -1 &&
- VARATT_IS_EXTERNAL(DatumGetPointer(newValue)))
+ (VARATT_IS_EXTERNAL(DatumGetPointer(newValue)) ||
+ VARATT_IS_COMPRESSED(DatumGetPointer(newValue))))
{
/* Detoasting should be done in short-lived context. */
oldcxt = MemoryContextSwitchTo(get_short_term_cxt(erh));
- newValue = PointerGetDatum(detoast_external_attr((struct varlena *) DatumGetPointer(newValue)));
+ newValue = PointerGetDatum(detoast_attr((struct varlena *) DatumGetPointer(newValue)));
MemoryContextSwitchTo(oldcxt);
}
else
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index c3d464f42b..821aa8fbdb 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -3388,8 +3388,7 @@ populate_record(TupleDesc tupdesc,
&field,
&nulls[i]);
- if (!nulls[i] && att->attlen == -1 &&
- VARATT_IS_EXTERNAL(DatumGetPointer(values[i])))
+ if (!nulls[i] && att->attlen == -1)
values[i] = PointerGetDatum(PG_DETOAST_DATUM_PACKED(values[i]));
}
diff --git a/src/include/funcapi.h b/src/include/funcapi.h
index 8ba7ae211f..c869012873 100644
--- a/src/include/funcapi.h
+++ b/src/include/funcapi.h
@@ -208,10 +208,10 @@ extern TupleDesc build_function_result_tupdesc_t(HeapTuple procTuple);
* Macro declarations/inline functions:
* HeapTupleHeaderGetRawDatum(HeapTupleHeader tuple) - same as
* HeapTupleHeaderGetDatum but the input tuple should not contain
- * external varlena
+ * external/compressed varlena
* HeapTupleGetDatum(HeapTuple tuple) - convert a HeapTuple to a Datum.
* HeapTupleGetRawDatum(HeapTuple tuple) - same as HeapTupleGetDatum
- * but the input tuple should not contain external varlena
+ * but the input tuple should not contain external/compressed varlena
*
* Obsolete routines and macros:
* TupleDesc RelationNameGetTupleDesc(const char *relname) - Use to get a
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index fd073767bc..0519253cbe 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -7300,8 +7300,7 @@ make_tuple_from_row(PLpgSQL_execstate *estate,
&dvalues[i], &nulls[i]);
if (fieldtypeid != TupleDescAttr(tupdesc, i)->atttypid)
return NULL;
- if (!nulls[i] && TupleDescAttr(tupdesc, i)->attlen == -1 &&
- VARATT_IS_EXTERNAL(DatumGetPointer(dvalues[i])))
+ if (!nulls[i] && TupleDescAttr(tupdesc, i)->attlen == -1)
dvalues[i] = PointerGetDatum(PG_DETOAST_DATUM_PACKED(dvalues[i]));
/* XXX should we insist on typmod match, too? */
}
--
2.17.0
--C94crkcyjafcjHxo
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0004-Built-in-compression-method.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Expand applicability of aggregate's sortop optimization
@ 2024-07-17 03:28 Andrei Lepikhov <[email protected]>
2024-07-17 09:33 ` Re: Expand applicability of aggregate's sortop optimization Matthias van de Meent <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andrei Lepikhov @ 2024-07-17 03:28 UTC (permalink / raw)
To: Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>; +Cc: David Rowley <[email protected]>; [email protected]
On 5/8/24 17:13, Matthias van de Meent wrote:
> As you may know, aggregates like SELECT MIN(unique1) FROM tenk1; are
> rewritten as SELECT unique1 FROM tenk1 ORDER BY unique1 USING < LIMIT
> 1; by using the optional sortop field in the aggregator.
> However, this optimization is disabled for clauses that in itself have
> an ORDER BY clause such as `MIN(unique1 ORDER BY <anything>), because
> <anything> can cause reordering of distinguisable values like 1.0 and
> 1.00, which then causes measurable differences in the output. In the
> general case, that's a good reason to not apply this optimization, but
> in some cases, we could still apply the index optimization.
Thanks for the job! I guess you could be more brave and push down also
FILTER statement.
>
> One of those cases is fixed in the attached patch: if we order by the
> same column that we're aggregating, using the same order class as the
> aggregate's sort operator (i.e. the aggregate's sortop is in the same
> btree opclass as the ORDER BY's sort operator), then we can still use
> the index operation: The sort behaviour isn't changed, thus we can
> apply the optimization.
>
> PFA the small patch that implements this.
>
> Note that we can't blindly accept just any ordering by the same
> column: If we had an opclass that sorted numeric values by the length
> of the significant/mantissa, then that'd provide a different (and
> distinct) ordering from that which is expected by the normal
> min()/max() aggregates for numeric, which could cause us to return
> arguably incorrect results for the aggregate expression.
As I see, the code:
aggsortop = fetch_agg_sort_op(aggref->aggfnoid);
if (!OidIsValid(aggsortop))
return false; /* not a MIN/MAX aggregate */
used twice and can be evaluated earlier to avoid duplicated code.
Also, I'm unsure about the necessity of looking through the btree
classes. Maybe just to check the commutator to the sortop, like in the
diff attached? Or could you provide an example to support your approach?
--
regards, Andrei Lepikhov
Attachments:
[text/x-patch] rewritten_patch.diff (6.4K, ../../[email protected]/2-rewritten_patch.diff)
download | inline diff:
diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c
index afb5445b77..e9e95e2b2a 100644
--- a/src/backend/optimizer/plan/planagg.c
+++ b/src/backend/optimizer/plan/planagg.c
@@ -253,6 +253,20 @@ can_minmax_aggs(PlannerInfo *root, List **context)
if (list_length(aggref->args) != 1)
return false; /* it couldn't be MIN/MAX */
+ /*
+ * We might implement the optimization when a FILTER clause is present
+ * by adding the filter to the quals of the generated subquery. For
+ * now, just punt.
+ */
+ if (aggref->aggfilter != NULL)
+ return false;
+
+ curTarget = (TargetEntry *) linitial(aggref->args);
+
+ aggsortop = fetch_agg_sort_op(aggref->aggfnoid);
+ if (!OidIsValid(aggsortop))
+ return false; /* not a MIN/MAX aggregate */
+
/*
* ORDER BY is usually irrelevant for MIN/MAX, but it can change the
* outcome if the aggsortop's operator class recognizes non-identical
@@ -267,22 +281,37 @@ can_minmax_aggs(PlannerInfo *root, List **context)
* quickly.
*/
if (aggref->aggorder != NIL)
- return false;
- /* note: we do not care if DISTINCT is mentioned ... */
-
- /*
- * We might implement the optimization when a FILTER clause is present
- * by adding the filter to the quals of the generated subquery. For
- * now, just punt.
- */
- if (aggref->aggfilter != NULL)
- return false;
+ {
+ SortGroupClause *orderClause;
+
+ /*
+ * If the order clause is the same column as the one we're
+ * aggregating, we can still use the index: It is undefined which
+ * value is MIN() or MAX(), as well as which value is first or
+ * last when sorted. So, we can still use the index IFF the
+ * aggregated expression equals the expression used in the
+ * ordering operation.
+ */
+
+ /*
+ * We only accept a single argument to min/max aggregates,
+ * orderings that have more clauses won't provide correct results.
+ */
+ if (list_length(aggref->aggorder) > 1)
+ return false;
+
+ orderClause = castNode(SortGroupClause, linitial(aggref->aggorder));
+
+ if (orderClause->tleSortGroupRef != curTarget->ressortgroupref)
+ return false;
+
+ if (orderClause->sortop != aggsortop &&
+ orderClause->sortop != get_commutator(aggsortop))
+ return false;
+ }
- aggsortop = fetch_agg_sort_op(aggref->aggfnoid);
- if (!OidIsValid(aggsortop))
- return false; /* not a MIN/MAX aggregate */
+ /* note: we do not care if DISTINCT is mentioned ... */
- curTarget = (TargetEntry *) linitial(aggref->args);
if (contain_mutable_functions((Node *) curTarget->expr))
return false; /* not potentially indexable */
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index a5596ab210..5d37510d64 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1003,6 +1003,71 @@ select max(unique1) from tenk1 where unique1 > 42;
9999
(1 row)
+-- When sorting on the column that's being aggregated, indexes can also be
+-- used, but only when the aggregate's operator has the same ordering behavior
+-- as the ORDER BY-clause, i.e. if it is in the same btree opclass as the one
+-- chosen for the ORDER BY clause.
+explain (costs off)
+ select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1;
+ QUERY PLAN
+------------------------------------------------------------
+ Result
+ InitPlan 1
+ -> Limit
+ -> Index Only Scan using tenk1_unique1 on tenk1
+ Index Cond: (unique1 IS NOT NULL)
+(5 rows)
+
+select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1;
+ min
+-----
+ 0
+(1 row)
+
+explain (costs off)
+ select max(unique1 ORDER BY unique1 USING <) from tenk1;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Result
+ InitPlan 1
+ -> Limit
+ -> Index Only Scan Backward using tenk1_unique1 on tenk1
+ Index Cond: (unique1 IS NOT NULL)
+(5 rows)
+
+select max(unique1 ORDER BY unique1 USING <) from tenk1;
+ max
+------
+ 9999
+(1 row)
+
+explain (costs off)
+ select max(unique1 ORDER BY tenthous) from tenk1;
+ QUERY PLAN
+-------------------------------
+ Aggregate
+ -> Sort
+ Sort Key: tenthous
+ -> Seq Scan on tenk1
+(4 rows)
+
+select max(unique1 ORDER BY tenthous) from tenk1;
+ max
+------
+ 9999
+(1 row)
+
+-- But even then, the index can't be used if we order by multiple columns.
+explain (costs off)
+ select max(unique1 ORDER BY unique1, tenthous) from tenk1;
+ QUERY PLAN
+-------------------------------------
+ Aggregate
+ -> Sort
+ Sort Key: unique1, tenthous
+ -> Seq Scan on tenk1
+(4 rows)
+
-- the planner may choose a generic aggregate here if parallel query is
-- enabled, since that plan will be parallel safe and the "optimized"
-- plan, which has almost identical cost, will not be. we want to test
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index ca6d1bcfb7..5cdf336fc0 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -368,6 +368,24 @@ explain (costs off)
select max(unique1) from tenk1 where unique1 > 42;
select max(unique1) from tenk1 where unique1 > 42;
+-- When sorting on the column that's being aggregated, indexes can also be
+-- used, but only when the aggregate's operator has the same ordering behavior
+-- as the ORDER BY-clause, i.e. if it is in the same btree opclass as the one
+-- chosen for the ORDER BY clause.
+explain (costs off)
+ select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1;
+select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1;
+explain (costs off)
+ select max(unique1 ORDER BY unique1 USING <) from tenk1;
+select max(unique1 ORDER BY unique1 USING <) from tenk1;
+explain (costs off)
+ select max(unique1 ORDER BY tenthous) from tenk1;
+select max(unique1 ORDER BY tenthous) from tenk1;
+
+-- But even then, the index can't be used if we order by multiple columns.
+explain (costs off)
+ select max(unique1 ORDER BY unique1, tenthous) from tenk1;
+
-- the planner may choose a generic aggregate here if parallel query is
-- enabled, since that plan will be parallel safe and the "optimized"
-- plan, which has almost identical cost, will not be. we want to test
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Expand applicability of aggregate's sortop optimization
2024-07-17 03:28 Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[email protected]>
@ 2024-07-17 09:33 ` Matthias van de Meent <[email protected]>
2024-07-17 14:09 ` Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Matthias van de Meent @ 2024-07-17 09:33 UTC (permalink / raw)
To: Andrei Lepikhov <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; David Rowley <[email protected]>; [email protected]
On Wed, 17 Jul 2024 at 05:29, Andrei Lepikhov <[email protected]> wrote:
>
> On 5/8/24 17:13, Matthias van de Meent wrote:
> > As you may know, aggregates like SELECT MIN(unique1) FROM tenk1; are
> > rewritten as SELECT unique1 FROM tenk1 ORDER BY unique1 USING < LIMIT
> > 1; by using the optional sortop field in the aggregator.
> > However, this optimization is disabled for clauses that in itself have
> > an ORDER BY clause such as `MIN(unique1 ORDER BY <anything>), because
> > <anything> can cause reordering of distinguisable values like 1.0 and
> > 1.00, which then causes measurable differences in the output. In the
> > general case, that's a good reason to not apply this optimization, but
> > in some cases, we could still apply the index optimization.
>
> Thanks for the job! I guess you could be more brave and push down also
> FILTER statement.
While probably not impossible, I wasn't planning on changing this code
with new optimizations; just expanding the applicability of the
current optimizations.
Note that the "aggfilter" clause was not new, but moved up in the code
to make sure we use this local information to bail out (if applicable)
before trying to use the catalogs for bail-out information.
> >
> > One of those cases is fixed in the attached patch: if we order by the
> > same column that we're aggregating, using the same order class as the
> > aggregate's sort operator (i.e. the aggregate's sortop is in the same
> > btree opclass as the ORDER BY's sort operator), then we can still use
> > the index operation: The sort behaviour isn't changed, thus we can
> > apply the optimization.
> >
> > PFA the small patch that implements this.
> >
> > Note that we can't blindly accept just any ordering by the same
> > column: If we had an opclass that sorted numeric values by the length
> > of the significant/mantissa, then that'd provide a different (and
> > distinct) ordering from that which is expected by the normal
> > min()/max() aggregates for numeric, which could cause us to return
> > arguably incorrect results for the aggregate expression.
>
> As I see, the code:
> aggsortop = fetch_agg_sort_op(aggref->aggfnoid);
> if (!OidIsValid(aggsortop))
> return false; /* not a MIN/MAX aggregate */
>
> used twice and can be evaluated earlier to avoid duplicated code.
The code is structured like this to make sure we only start accessing
catalogs once we know that all other reasons to bail out from this
optimization indicate we can apply the opimization. You'll notice that
I've tried to put the cheapest checks that only use caller-supplied
information first, and catalog accesses only after that.
If the fetch_agg_sort_op clause would be deduplicated, it would either
increase code complexity to handle both aggref->aggorder paths, or it
would increase the cost of planning MAX(a ORDER BY b) because of the
newly added catalog access.
> Also, I'm unsure about the necessity of looking through the btree
> classes. Maybe just to check the commutator to the sortop, like in the
> diff attached? Or could you provide an example to support your approach?
I think it could work, but I'd be hesitant to rely on that, as
commutator registration is optional (useful, but never required for
btree operator classes' operators). Looking at the btree operator
class, which is the definition of sortability in PostgreSQL, seems
more suitable and correct.
Kind regards,
Matthias van de Meent
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Expand applicability of aggregate's sortop optimization
2024-07-17 03:28 Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[email protected]>
2024-07-17 09:33 ` Re: Expand applicability of aggregate's sortop optimization Matthias van de Meent <[email protected]>
@ 2024-07-17 14:09 ` Andrei Lepikhov <[email protected]>
2024-07-18 12:49 ` Re: Expand applicability of aggregate's sortop optimization Matthias van de Meent <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andrei Lepikhov @ 2024-07-17 14:09 UTC (permalink / raw)
To: Matthias van de Meent <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; David Rowley <[email protected]>; [email protected]
On 17/7/2024 16:33, Matthias van de Meent wrote:
> On Wed, 17 Jul 2024 at 05:29, Andrei Lepikhov <[email protected]> wrote:
>> Thanks for the job! I guess you could be more brave and push down also
>> FILTER statement.
>
> While probably not impossible, I wasn't planning on changing this code
> with new optimizations; just expanding the applicability of the
> current optimizations.
Got it>> As I see, the code:
>> aggsortop = fetch_agg_sort_op(aggref->aggfnoid);
>> if (!OidIsValid(aggsortop))
>> return false; /* not a MIN/MAX aggregate */
>>
>> used twice and can be evaluated earlier to avoid duplicated code.
>
> The code is structured like this to make sure we only start accessing
> catalogs once we know that all other reasons to bail out from this
> optimization indicate we can apply the opimization. You'll notice that
> I've tried to put the cheapest checks that only use caller-supplied
> information first, and catalog accesses only after that.
>
> If the fetch_agg_sort_op clause would be deduplicated, it would either
> increase code complexity to handle both aggref->aggorder paths, or it
> would increase the cost of planning MAX(a ORDER BY b) because of the
> newly added catalog access.
IMO it looks like a micro optimisation. But I agree, it is more about
code style - let the committer decide what is better.>> Also, I'm unsure
about the necessity of looking through the btree
>> classes. Maybe just to check the commutator to the sortop, like in the
>> diff attached? Or could you provide an example to support your approach?
>
> I think it could work, but I'd be hesitant to rely on that, as
> commutator registration is optional (useful, but never required for
> btree operator classes' operators). Looking at the btree operator
> class, which is the definition of sortability in PostgreSQL, seems
> more suitable and correct.
Hm, I dubious about that. Can you provide an example which my variant
will not pass but your does that correctly?
--
regards, Andrei Lepikhov
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Expand applicability of aggregate's sortop optimization
2024-07-17 03:28 Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[email protected]>
2024-07-17 09:33 ` Re: Expand applicability of aggregate's sortop optimization Matthias van de Meent <[email protected]>
2024-07-17 14:09 ` Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[email protected]>
@ 2024-07-18 12:49 ` Matthias van de Meent <[email protected]>
2024-09-03 06:25 ` Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Matthias van de Meent @ 2024-07-18 12:49 UTC (permalink / raw)
To: Andrei Lepikhov <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; David Rowley <[email protected]>; [email protected]
On Wed, 17 Jul 2024 at 16:09, Andrei Lepikhov <[email protected]> wrote:
>
> On 17/7/2024 16:33, Matthias van de Meent wrote:
> > On Wed, 17 Jul 2024 at 05:29, Andrei Lepikhov <[email protected]> wrote:
> >> Thanks for the job! I guess you could be more brave and push down also
> >> FILTER statement.
> >
> > While probably not impossible, I wasn't planning on changing this code
> > with new optimizations; just expanding the applicability of the
> > current optimizations.
> Got it>> As I see, the code:
> >> aggsortop = fetch_agg_sort_op(aggref->aggfnoid);
> >> if (!OidIsValid(aggsortop))
> >> return false; /* not a MIN/MAX aggregate */
> >>
> >> used twice and can be evaluated earlier to avoid duplicated code.
> >
> > The code is structured like this to make sure we only start accessing
> > catalogs once we know that all other reasons to bail out from this
> > optimization indicate we can apply the opimization. You'll notice that
> > I've tried to put the cheapest checks that only use caller-supplied
> > information first, and catalog accesses only after that.
> >
> > If the fetch_agg_sort_op clause would be deduplicated, it would either
> > increase code complexity to handle both aggref->aggorder paths, or it
> > would increase the cost of planning MAX(a ORDER BY b) because of the
> > newly added catalog access.
> IMO it looks like a micro optimisation. But I agree, it is more about
> code style - let the committer decide what is better.>> Also, I'm unsure
> about the necessity of looking through the btree
> >> classes. Maybe just to check the commutator to the sortop, like in the
> >> diff attached? Or could you provide an example to support your approach?
> >
> > I think it could work, but I'd be hesitant to rely on that, as
> > commutator registration is optional (useful, but never required for
> > btree operator classes' operators). Looking at the btree operator
> > class, which is the definition of sortability in PostgreSQL, seems
> > more suitable and correct.
> Hm, I dubious about that. Can you provide an example which my variant
> will not pass but your does that correctly?
Here is one:
"""
CREATE OPERATOR @@> (
function=int4gt, leftarg=int4, rightarg=int4
); CREATE OPERATOR @@>= (
function=int4ge, leftarg=int4, rightarg=int4
); CREATE OPERATOR @@= (
function=int4eq, leftarg=int4, rightarg=int4
); CREATE OPERATOR @@<= (
function=int4le, leftarg=int4, rightarg=int4
); CREATE OPERATOR @@< (
function=int4lt, leftarg=int4, rightarg=int4
);
CREATE OPERATOR CLASS my_int_ops
FOR TYPE int
USING btree AS
OPERATOR 1 @<@,
OPERATOR 2 @<=@,
OPERATOR 3 @=@,
OPERATOR 4 @>=@,
OPERATOR 5 @>@,
FUNCTION 1 btint4cmp;
CREATE AGGREGATE my_int_max (
BASETYPE = int4,
SFUNC = int4larger,
STYPE = int4,
SORTOP = @>@
);
CREATE TABLE my_table AS
SELECT id::int4 FROM generate_series(1, 10000) id;
CREATE INDEX ON my_table (id my_int_ops);
SELECT my_int_max(id ORDER BY id USING @<@ ) from my_table;
"""
Because the @<@ and @>@ operators are not registered as commutative,
it couldn't apply the optimization in your patch, while the btree
operator check does allow it to apply the optimization.
Aside: Arguably, checking for commutator operators would not be
incorrect when looking at it from "applied operators" point of view,
but if that commutative operator isn't registered as opposite ordering
of the same btree opclass, then we'd probably break some assumptions
of some aggregate's sortop - it could be registered with another
opclass, and that could cause us to select a different btree opclass
(thus: ordering) than is indicated to be required by the aggregate;
the thing we're trying to protect against here.
Kind regards,
Matthias van de Meent
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Expand applicability of aggregate's sortop optimization
2024-07-17 03:28 Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[email protected]>
2024-07-17 09:33 ` Re: Expand applicability of aggregate's sortop optimization Matthias van de Meent <[email protected]>
2024-07-17 14:09 ` Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[email protected]>
2024-07-18 12:49 ` Re: Expand applicability of aggregate's sortop optimization Matthias van de Meent <[email protected]>
@ 2024-09-03 06:25 ` Andrei Lepikhov <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Andrei Lepikhov @ 2024-09-03 06:25 UTC (permalink / raw)
To: Matthias van de Meent <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; David Rowley <[email protected]>; [email protected]
On 18/7/2024 14:49, Matthias van de Meent wrote:
> Aside: Arguably, checking for commutator operators would not be
> incorrect when looking at it from "applied operators" point of view,
> but if that commutative operator isn't registered as opposite ordering
> of the same btree opclass, then we'd probably break some assumptions
> of some aggregate's sortop - it could be registered with another
> opclass, and that could cause us to select a different btree opclass
> (thus: ordering) than is indicated to be required by the aggregate;
> the thing we're trying to protect against here.
Hi,
This thread stands idle. At the same time, the general idea of this
patch and the idea of utilising prosupport functions look promising. Are
you going to develop this feature further?
--
regards, Andrei Lepikhov
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-09-03 06:25 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-04 11:03 [PATCH 3/8] Disallow compressed data inside container types Dilip Kumar <[email protected]>
2024-07-17 03:28 Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[email protected]>
2024-07-17 09:33 ` Re: Expand applicability of aggregate's sortop optimization Matthias van de Meent <[email protected]>
2024-07-17 14:09 ` Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[email protected]>
2024-07-18 12:49 ` Re: Expand applicability of aggregate's sortop optimization Matthias van de Meent <[email protected]>
2024-09-03 06:25 ` Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[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