public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] fix CREATE INDEX progress report with nested partitions
15+ messages / 5 participants
[nested] [flat]
* [PATCH] fix CREATE INDEX progress report with nested partitions
@ 2023-01-31 15:13 Ilya Gladyshev <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Ilya Gladyshev @ 2023-01-31 15:13 UTC (permalink / raw)
The progress reporting was added in v12 (ab0dfc961) but the original
patch didn't seem to consider the possibility of nested partitioning.
When called recursively, DefineIndex() would clobber the number of
completed partitions, and it was possible to end up with the TOTAL
counter greater than the DONE counter.
This clarifies/re-defines that the progress reporting counts both direct
and indirect children, but doesn't count intermediate partitioned tables:
- The TOTAL counter is set once at the start of the command.
- For indexes which are newly-built, the recursively-called
DefineIndex() increments the DONE counter.
- For pre-existing indexes which are ATTACHed rather than built,
DefineIndex() increments the DONE counter, unless the attached index is
partitioned, in which case progress report is not updated.
Author: Ilya Gladyshev
Reviewed-By: Justin Pryzby, Tomas Vondra, Dean Rasheed, Alvaro Herrera, Matthias van de Meent
Discussion: https://www.postgresql.org/message-id/flat/a15f904a70924ffa4ca25c3c744cff31e0e6e143.camel%40gmail.co...
---
doc/src/sgml/monitoring.sgml | 10 ++-
src/backend/bootstrap/bootparse.y | 2 +
src/backend/commands/indexcmds.c | 70 +++++++++++++++++--
src/backend/commands/tablecmds.c | 4 +-
src/backend/tcop/utility.c | 8 ++-
src/backend/utils/activity/backend_progress.c | 28 ++++++++
src/include/commands/defrem.h | 1 +
src/include/utils/backend_progress.h | 1 +
8 files changed, 115 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 6249bb50d02..3f891b75541 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -6901,7 +6901,10 @@ FROM pg_stat_get_backend_idset() AS backendid;
</para>
<para>
When creating an index on a partitioned table, this column is set to
- the total number of partitions on which the index is to be created.
+ the total number of partitions on which the index is to be created or attached.
+ In the case of intermediate partitioned tables, this includes both
+ direct and indirect partitions, but excludes the intermediate
+ partitioned tables themselves.
This field is <literal>0</literal> during a <literal>REINDEX</literal>.
</para></entry>
</row>
@@ -6912,7 +6915,10 @@ FROM pg_stat_get_backend_idset() AS backendid;
</para>
<para>
When creating an index on a partitioned table, this column is set to
- the number of partitions on which the index has been created.
+ the number of partitions on which the index has been created or attached.
+ In the case of intermediate partitioned tables, this includes both
+ direct and indirect partitions, but excludes the intermediate
+ partitioned tables themselves.
This field is <literal>0</literal> during a <literal>REINDEX</literal>.
</para></entry>
</row>
diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y
index 86804bb598e..81a1b7bfec3 100644
--- a/src/backend/bootstrap/bootparse.y
+++ b/src/backend/bootstrap/bootparse.y
@@ -306,6 +306,7 @@ Boot_DeclareIndexStmt:
$4,
InvalidOid,
InvalidOid,
+ -1,
false,
false,
false,
@@ -358,6 +359,7 @@ Boot_DeclareUniqueIndexStmt:
$5,
InvalidOid,
InvalidOid,
+ -1,
false,
false,
false,
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 16ec0b114e6..62a8f0d6aa2 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -130,6 +130,32 @@ typedef struct ReindexErrorInfo
char relkind;
} ReindexErrorInfo;
+
+/*
+ * Count the number of direct and indirect leaf partitions. Note that this
+ * also excludes foreign tables. This should be consistent with the loop in
+ * ProcessUtilitySlow().
+ * XXX: are the partitions already locked when this is called by other code paths ?
+ */
+static int
+count_leaf_partitions(Oid relid)
+{
+ int nleaves = 0;
+ List *childs = find_all_inheritors(relid, NoLock, NULL);
+ ListCell *lc;
+
+ foreach(lc, childs)
+ {
+ Oid partrelid = lfirst_oid(lc);
+
+ if (RELKIND_HAS_STORAGE(get_rel_relkind(partrelid)))
+ nleaves++;
+ }
+
+ list_free(childs);
+ return nleaves;
+}
+
/*
* CheckIndexCompatible
* Determine whether an existing index definition is compatible with a
@@ -518,6 +544,7 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
* case the caller had better have checked it earlier.
* 'skip_build': make the catalog entries but don't create the index files
* 'quiet': suppress the NOTICE chatter ordinarily provided for constraints.
+ * 'total_leaves': total number of leaf partitions (excluding foreign tables)
*
* Returns the object address of the created index.
*/
@@ -527,6 +554,7 @@ DefineIndex(Oid relationId,
Oid indexRelationId,
Oid parentIndexId,
Oid parentConstraintId,
+ int total_leaves,
bool is_alter_table,
bool check_rights,
bool check_not_in_use,
@@ -1219,8 +1247,22 @@ DefineIndex(Oid relationId,
Relation parentIndex;
TupleDesc parentDesc;
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
- nparts);
+ /*
+ * Set the total number of partitions at the start of the command;
+ * don't update it when being called recursively.
+ */
+ if (!OidIsValid(parentIndexId))
+ {
+ /*
+ * When called by ProcessUtilitySlow() the number of leaves is
+ * passed in as an optimization.
+ */
+ if (total_leaves < 0)
+ total_leaves = count_leaf_partitions(relationId);
+
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
+ total_leaves);
+ }
/* Make a local copy of partdesc->oids[], just for safety */
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
@@ -1250,6 +1292,7 @@ DefineIndex(Oid relationId,
{
Oid childRelid = part_oids[i];
Relation childrel;
+ char child_relkind;
Oid child_save_userid;
int child_save_sec_context;
int child_save_nestlevel;
@@ -1259,6 +1302,7 @@ DefineIndex(Oid relationId,
bool found = false;
childrel = table_open(childRelid, lockmode);
+ child_relkind = RelationGetForm(childrel)->relkind;
GetUserIdAndSecContext(&child_save_userid,
&child_save_sec_context);
@@ -1426,14 +1470,24 @@ DefineIndex(Oid relationId,
InvalidOid, /* no predefined OID */
indexRelationId, /* this is our child */
createdConstraintId,
+ -1, /* The progress isn't updated during recursion */
is_alter_table, check_rights, check_not_in_use,
skip_build, quiet);
SetUserIdAndSecContext(child_save_userid,
child_save_sec_context);
}
+ else
+ {
+ /*
+ * If a pre-existing index was attached, the progress
+ * report is updated here. But if the index is
+ * partitioned, we don't count its partitions, since
+ * that's expensive, and the ATTACH is fast anyway.
+ */
+ if (!RELKIND_HAS_PARTITIONS(child_relkind))
+ pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+ }
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- i + 1);
free_attrmap(attmap);
}
@@ -1484,9 +1538,15 @@ DefineIndex(Oid relationId,
/* Close the heap and we're done, in the non-concurrent case */
table_close(rel, NoLock);
- /* If this is the top-level index, we're done. */
+ /*
+ * If this is the top-level index, we're done. When called recursively
+ * for child tables, the done partition counter is incremented now,
+ * rather than in the caller.
+ */
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
+ else
+ pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
return address;
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3e2c5f797cd..2ef4491ec8e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1216,6 +1216,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
InvalidOid,
RelationGetRelid(idxRel),
constraintOid,
+ -1,
false, false, false, false, false);
index_close(idxRel, AccessShareLock);
@@ -8640,6 +8641,7 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
InvalidOid, /* no predefined OID */
InvalidOid, /* no parent index */
InvalidOid, /* no parent constraint */
+ -1,
true, /* is_alter_table */
check_rights,
false, /* check_not_in_use - we did it already */
@@ -18105,7 +18107,7 @@ AttachPartitionEnsureIndexes(Relation rel, Relation attachrel)
&conOid);
DefineIndex(RelationGetRelid(attachrel), stmt, InvalidOid,
RelationGetRelid(idxRel),
- conOid,
+ conOid, -1,
true, false, false, false, false);
}
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index c7d9d96b45d..c8317822a64 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1462,6 +1462,7 @@ ProcessUtilitySlow(ParseState *pstate,
Oid relid;
LOCKMODE lockmode;
bool is_alter_table;
+ int total_leaves = 0;
if (stmt->concurrent)
PreventInTransactionBlock(isTopLevel,
@@ -1502,7 +1503,8 @@ ProcessUtilitySlow(ParseState *pstate,
inheritors = find_all_inheritors(relid, lockmode, NULL);
foreach(lc, inheritors)
{
- char relkind = get_rel_relkind(lfirst_oid(lc));
+ Oid partrelid = lfirst_oid(lc);
+ char relkind = get_rel_relkind(partrelid);
if (relkind != RELKIND_RELATION &&
relkind != RELKIND_MATVIEW &&
@@ -1519,6 +1521,9 @@ ProcessUtilitySlow(ParseState *pstate,
stmt->relation->relname),
errdetail("Table \"%s\" contains partitions that are foreign tables.",
stmt->relation->relname)));
+
+ if (RELKIND_HAS_STORAGE(get_rel_relkind(partrelid)))
+ total_leaves++;
}
list_free(inheritors);
}
@@ -1545,6 +1550,7 @@ ProcessUtilitySlow(ParseState *pstate,
InvalidOid, /* no predefined OID */
InvalidOid, /* no parent index */
InvalidOid, /* no parent constraint */
+ total_leaves,
is_alter_table,
true, /* check_rights */
true, /* check_not_in_use */
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index d96af812b19..2a9994b98fd 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -58,6 +58,34 @@ pgstat_progress_update_param(int index, int64 val)
PGSTAT_END_WRITE_ACTIVITY(beentry);
}
+/*-----------
+ * pgstat_progress_incr_param() -
+ *
+ * Increment index'th member in st_progress_param[] of the current backend.
+ *-----------
+ */
+void
+pgstat_progress_incr_param(int index, int64 incr)
+{
+ volatile PgBackendStatus *beentry = MyBEEntry;
+ int64 val;
+
+ Assert(index >= 0 && index < PGSTAT_NUM_PROGRESS_PARAM);
+
+ if (!beentry || !pgstat_track_activities)
+ return;
+
+ /*
+ * Because no other process should write to this backend's own status, we
+ * can read its value from shared memory without needing to loop to ensure
+ * its consistency.
+ */
+ val = beentry->st_progress_param[index];
+ val += incr;
+
+ pgstat_progress_update_param(index, val);
+}
+
/*-----------
* pgstat_progress_update_multi_param() -
*
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 4f7f87fc62c..b774b3028ce 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -29,6 +29,7 @@ extern ObjectAddress DefineIndex(Oid relationId,
Oid indexRelationId,
Oid parentIndexId,
Oid parentConstraintId,
+ int total_leaves,
bool is_alter_table,
bool check_rights,
bool check_not_in_use,
diff --git a/src/include/utils/backend_progress.h b/src/include/utils/backend_progress.h
index 005e5d75ab6..a84752ade99 100644
--- a/src/include/utils/backend_progress.h
+++ b/src/include/utils/backend_progress.h
@@ -36,6 +36,7 @@ typedef enum ProgressCommandType
extern void pgstat_progress_start_command(ProgressCommandType cmdtype,
Oid relid);
extern void pgstat_progress_update_param(int index, int64 val);
+extern void pgstat_progress_incr_param(int index, int64 incr);
extern void pgstat_progress_update_multi_param(int nparam, const int *index,
const int64 *val);
extern void pgstat_progress_end_command(void);
--
2.34.1
--ifoK5vRJk3IPpQDo--
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
@ 2023-10-03 07:16 David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: David Rowley @ 2023-10-03 07:16 UTC (permalink / raw)
To: Richard Guo <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]
On Tue, 3 Oct 2023 at 09:11, David Rowley <[email protected]> wrote:
> I'm concerned that this patch will be too much overhead when creating
> paths when a PathKey's EquivalenceClass has a large number of members
> from partitioned tables.
I just tried out the patch to see how much it affects the performance
of the planner. I think we need to find a better way to strip off the
pathkeys for the columns that have been aggregated.
Setup:
create table lp (a int, b int) partition by list (a);
select 'create table lp'||x||' partition of lp for values in('||x||')'
from generate_series(0,999)x;
\gexec
\pset pager off
set enable_partitionwise_aggregate=1;
Benchmark query:
explain (summary on) select a,count(*) from lp group by a;
Master:
Planning Time: 23.945 ms
Planning Time: 23.887 ms
Planning Time: 23.927 ms
perf top:
7.39% libc.so.6 [.] __memmove_avx_unaligned_erms
6.98% [kernel] [k] clear_page_rep
5.69% postgres [.] bms_is_subset
5.07% postgres [.] fetch_upper_rel
4.41% postgres [.] bms_equal
Patched:
Planning Time: 41.410 ms
Planning Time: 41.474 ms
Planning Time: 41.488 ms
perf top:
19.02% postgres [.] bms_is_subset
6.91% postgres [.] find_ec_member_matching_expr
5.93% libc.so.6 [.] __memmove_avx_unaligned_erms
5.55% [kernel] [k] clear_page_rep
4.07% postgres [.] fetch_upper_rel
3.46% postgres [.] bms_equal
> I wondered if we should instead just check
> if the subpath's pathkeys match root->group_pathkeys and if they do
> set the AggPath's pathkeys to list_copy_head(subpath->pathkeys,
> root->num_groupby_pathkeys), that'll be much cheaper, but it just
> feels a bit too much like a special case.
I tried this approach (patch attached) and it does perform better than
the other patch:
create_agg_path_fix2.patch:
Planning Time: 24.357 ms
Planning Time: 24.293 ms
Planning Time: 24.259 ms
7.45% libc.so.6 [.] __memmove_avx_unaligned_erms
6.90% [kernel] [k] clear_page_rep
5.56% postgres [.] bms_is_subset
5.38% postgres [.] bms_equal
I wonder if the attached patch is too much of a special case fix. I
guess from the lack of complaints previously that there are no other
cases where we could possibly have pathkeys that belong to columns
that are aggregated. I've not gone to much effort to see if I can
craft a case that hits this without the ORDER BY/DISTINCT aggregate
optimisation, however.
David
Attachments:
[application/octet-stream] create_agg_path_fix2.patch (935B, ../../CAApHDvo7RzcQYw-gnkZr6QCijCqf8vJLkJ4XFk-KawvyAw109Q@mail.gmail.com/2-create_agg_path_fix2.patch)
download | inline diff:
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 211ba65389..2469f43819 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -3133,10 +3133,18 @@ create_agg_path(PlannerInfo *root,
pathnode->path.parallel_safe = rel->consider_parallel &&
subpath->parallel_safe;
pathnode->path.parallel_workers = subpath->parallel_workers;
+
if (aggstrategy == AGG_SORTED)
- pathnode->path.pathkeys = subpath->pathkeys; /* preserves order */
+ {
+ if (pathkeys_contained_in(subpath->pathkeys, root->group_pathkeys))
+ pathnode->path.pathkeys = list_copy_head(subpath->pathkeys,
+ root->num_groupby_pathkeys);
+ else
+ pathnode->path.pathkeys = subpath->pathkeys; /* preserves order */
+ }
else
pathnode->path.pathkeys = NIL; /* output is unordered */
+
pathnode->subpath = subpath;
pathnode->aggstrategy = aggstrategy;
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
@ 2023-10-05 06:26 ` David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: David Rowley @ 2023-10-05 06:26 UTC (permalink / raw)
To: Richard Guo <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]
On Tue, 3 Oct 2023 at 20:16, David Rowley <[email protected]> wrote:
> I wonder if the attached patch is too much of a special case fix. I
> guess from the lack of complaints previously that there are no other
> cases where we could possibly have pathkeys that belong to columns
> that are aggregated. I've not gone to much effort to see if I can
> craft a case that hits this without the ORDER BY/DISTINCT aggregate
> optimisation, however.
I spent more time on this today. I'd been wondering if there was any
reason why create_agg_path() would receive a subpath with pathkeys
that were anything but the PlannerInfo's group_pathkeys. I mean, how
could we do Group Aggregate if it wasn't? I wondered if grouping sets
might change that, but it seems the group_pathkeys will be set to the
initial grouping set.
Given that, it would seem it's safe to just trim off any pathkey that
was added to the group_pathkeys by
adjust_group_pathkeys_for_groupagg().
PlannerInfo.num_groupby_pathkeys marks the number of pathkeys that
existed in group_pathkeys before adjust_group_pathkeys_for_groupagg()
made any additions, so we can just trim the list length back to that.
I've done this in the attached patch. I also considered if it was
worth adding a regression test for this and I concluded that there are
better ways to test for this and considered if we should add some code
to createplan.c to check that all Path pathkeys have corresponding
items in the PathTarget. I've included an additional patch which adds
some code in USE_ASSERT_CHECKING builds to verify this. Without the
fix it's simple enough to trigger this with a query such as:
select two,count(distinct four) from tenk1 group by two order by two;
Without the fix the additional asserts cause the regression tests to
fail, but with the fix everything passes.
Justin's case is quite an obscure way to hit this as it requires
partitionwise aggregation plus a single partition so that the Append
is removed due to only having a single subplan in setrefs.c. If there
had been 2 partitions, then the AppendPath wouldn't have inherited the
subpath's pathkeys per code at the end of create_append_path().
So in short, I propose the attached fix without any regression tests
because I feel that any regression test would just mark that there was
a big in create_agg_path() and not really help with ensuring we don't
end up with some similar problem in the future.
I have some concerns that the assert_pathkeys_in_target() function
might be a little heavyweight for USE_ASSERT_CHECKING builds. So I'm
not proposing to commit that without further discussion.
Does anyone feel differently?
If not, I plan to push the attached
strip_aggregate_pathkeys_from_aggpaths_v2.patch early next week.
David
Attachments:
[application/octet-stream] strip_aggregated_pathkeys_from_aggpaths_v2.patch (1.3K, ../../CAApHDvqoMf_kxQKON+FLQh8DbMKe62R=OdrZoenb6xDYWmQVUQ@mail.gmail.com/2-strip_aggregated_pathkeys_from_aggpaths_v2.patch)
download | inline diff:
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 211ba65389..700cd0e16b 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -3133,10 +3133,25 @@ create_agg_path(PlannerInfo *root,
pathnode->path.parallel_safe = rel->consider_parallel &&
subpath->parallel_safe;
pathnode->path.parallel_workers = subpath->parallel_workers;
+
if (aggstrategy == AGG_SORTED)
- pathnode->path.pathkeys = subpath->pathkeys; /* preserves order */
+ {
+ /*
+ * Attempt to preserve the order of the subpath. Additional pathkeys may
+ * have been added in adjust_group_pathkeys_for_groupagg() to support ORDER
+ * BY / DISTINCT aggregates. Pathkeys added there belong to columns within
+ * the aggregate function, so we must strip these additional pathkeys off as
+ * those columns are unavailable above the aggregate node.
+ */
+ if (list_length(subpath->pathkeys) > root->num_groupby_pathkeys)
+ pathnode->path.pathkeys = list_copy_head(subpath->pathkeys,
+ root->num_groupby_pathkeys);
+ else
+ pathnode->path.pathkeys = subpath->pathkeys; /* preserves order */
+ }
else
pathnode->path.pathkeys = NIL; /* output is unordered */
+
pathnode->subpath = subpath;
pathnode->aggstrategy = aggstrategy;
[application/octet-stream] assert_pathkeys_in_target.patch (1.4K, ../../CAApHDvqoMf_kxQKON+FLQh8DbMKe62R=OdrZoenb6xDYWmQVUQ@mail.gmail.com/3-assert_pathkeys_in_target.patch)
download | inline diff:
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 34ca6d4ac2..44a0e2d058 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -378,6 +378,46 @@ create_plan(PlannerInfo *root, Path *best_path)
return plan;
}
+#ifdef USE_ASSERT_CHECKING
+static void
+assert_pathkeys_in_target(Path *path)
+{
+ List *pathkeys = path->pathkeys;
+ PathTarget *target = path->pathtarget;
+ Relids relids = path->parent->relids;
+ ListCell *lc;
+
+ foreach (lc, pathkeys)
+ {
+ ListCell *lc2;
+ PathKey *pathkey = lfirst_node(PathKey, lc);
+ bool found = false;
+ foreach (lc2, target->exprs)
+ {
+ Expr *expr = lfirst(lc2);
+
+ if (find_ec_member_matching_expr(pathkey->pk_eclass, expr, relids))
+ {
+ found = true;
+ break;
+ }
+
+ }
+ if (!found)
+ {
+ if (find_computable_ec_member(NULL,
+ pathkey->pk_eclass,
+ target->exprs,
+ relids,
+ false))
+ found = true;
+ }
+
+ Assert(found);
+ }
+}
+#endif
+
/*
* create_plan_recurse
* Recursive guts of create_plan().
@@ -390,6 +430,10 @@ create_plan_recurse(PlannerInfo *root, Path *best_path, int flags)
/* Guard against stack overflow due to overly complex plans */
check_stack_depth();
+#ifdef USE_ASSERT_CHECKING
+ assert_pathkeys_in_target(best_path);
+#endif
+
switch (best_path->pathtype)
{
case T_SeqScan:
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
@ 2023-10-08 10:52 ` Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Richard Guo @ 2023-10-08 10:52 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]
On Thu, Oct 5, 2023 at 2:26 PM David Rowley <[email protected]> wrote:
> So in short, I propose the attached fix without any regression tests
> because I feel that any regression test would just mark that there was
> a big in create_agg_path() and not really help with ensuring we don't
> end up with some similar problem in the future.
If the pathkeys that were added by adjust_group_pathkeys_for_groupagg()
are computable from the targetlist, it seems that we do not need to trim
them off, because prepare_sort_from_pathkeys() will add resjunk target
entries for them. But it's also no harm if we trim them off. So I
think the patch is a pretty safe fix. +1 to it.
> I have some concerns that the assert_pathkeys_in_target() function
> might be a little heavyweight for USE_ASSERT_CHECKING builds. So I'm
> not proposing to commit that without further discussion.
Yeah, it looks like some heavy to call assert_pathkeys_in_target() for
each path node. Can we run some benchmarks to see how much overhead it
would bring to USE_ASSERT_CHECKING build?
Thanks
Richard
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
@ 2023-10-08 23:42 ` David Rowley <[email protected]>
2023-10-09 02:08 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-09 04:13 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
0 siblings, 2 replies; 15+ messages in thread
From: David Rowley @ 2023-10-08 23:42 UTC (permalink / raw)
To: Richard Guo <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]
On Sun, 8 Oct 2023 at 23:52, Richard Guo <[email protected]> wrote:
> On Thu, Oct 5, 2023 at 2:26 PM David Rowley <[email protected]> wrote:
>>
>> So in short, I propose the attached fix without any regression tests
>> because I feel that any regression test would just mark that there was
>> a big in create_agg_path() and not really help with ensuring we don't
>> end up with some similar problem in the future.
>
>
> If the pathkeys that were added by adjust_group_pathkeys_for_groupagg()
> are computable from the targetlist, it seems that we do not need to trim
> them off, because prepare_sort_from_pathkeys() will add resjunk target
> entries for them. But it's also no harm if we trim them off. So I
> think the patch is a pretty safe fix. +1 to it.
hmm, I think one of us does not understand what is going on here. I
tried to explain in [1] why we *need* to strip off the pathkeys added
by adjust_group_pathkeys_for_groupagg().
Given the following example:
create table ab (a int,b int);
explain (costs off) select a,count(distinct b) from ab group by a;
QUERY PLAN
----------------------------
GroupAggregate
Group Key: a
-> Sort
Sort Key: a, b
-> Seq Scan on ab
(5 rows)
adjust_group_pathkeys_for_groupagg() will add the pathkey for the "b"
column and that results in the Sort node sorting on {a,b}. It's
simply not at all valid to have the GroupAggregate path claim that its
pathkeys are also (effectively) {a,b}" as "b" does not and cannot
legally exist after the aggregation takes place. We cannot put a
resjunk "b" in the targetlist of the GroupAggregate either as there
could be any number "b" values aggregated.
Can you explain why you think we can put a resjunk "b" in the target
list of the GroupAggregate in the above case?
>>
>> I have some concerns that the assert_pathkeys_in_target() function
>> might be a little heavyweight for USE_ASSERT_CHECKING builds. So I'm
>> not proposing to commit that without further discussion.
>
>
> Yeah, it looks like some heavy to call assert_pathkeys_in_target() for
> each path node. Can we run some benchmarks to see how much overhead it
> would bring to USE_ASSERT_CHECKING build?
I think it'll be easy to show that there is an overhead to it. It'll
be in the realm of the ~41ms patched vs ~24ms unpatched that I showed
in [2]. That's quite an extreme case.
Maybe it's worth checking the total planning time spent in a run of
the regression tests with and without the patch to see how much
overhead it adds to the "average case".
David
[1] https://postgr.es/m/CAApHDvpJJigQRW29TppTOPYp+Aui0mtd3MpfRxyKv=N-tB62jQ@mail.gmail.com
[2] https://postgr.es/m/CAApHDvo7RzcQYw-gnkZr6QCijCqf8vJLkJ4XFk-KawvyAw109Q@mail.gmail.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
@ 2023-10-09 02:08 ` Richard Guo <[email protected]>
1 sibling, 0 replies; 15+ messages in thread
From: Richard Guo @ 2023-10-09 02:08 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]
On Mon, Oct 9, 2023 at 7:42 AM David Rowley <[email protected]> wrote:
> On Sun, 8 Oct 2023 at 23:52, Richard Guo <[email protected]> wrote:
> > If the pathkeys that were added by adjust_group_pathkeys_for_groupagg()
> > are computable from the targetlist, it seems that we do not need to trim
> > them off, because prepare_sort_from_pathkeys() will add resjunk target
> > entries for them. But it's also no harm if we trim them off. So I
> > think the patch is a pretty safe fix. +1 to it.
>
> hmm, I think one of us does not understand what is going on here. I
> tried to explain in [1] why we *need* to strip off the pathkeys added
> by adjust_group_pathkeys_for_groupagg().
Sorry I didn't make myself clear. I understand why we need to trim off
the pathkeys added by adjust_group_pathkeys_for_groupagg(). What I
meant was that if the new added pathkeys are *computable* from the
existing target entries, then prepare_sort_from_pathkeys() will add
resjunk target entries for them, so there seems to be no problem even if
we do not trim them off. For example
explain (verbose, costs off)
select a, count(distinct a+1) from prt1 group by a order by a;
QUERY PLAN
------------------------------------------------------------------------------------
Result
Output: prt1.a, (count(DISTINCT ((prt1.a + 1))))
-> Merge Append
Sort Key: prt1.a, ((prt1.a + 1))
-> GroupAggregate
Output: prt1.a, count(DISTINCT ((prt1.a + 1))), ((prt1.a +
1))
Group Key: prt1.a
-> Sort
Output: prt1.a, ((prt1.a + 1))
Sort Key: prt1.a, ((prt1.a + 1))
-> Seq Scan on public.prt1_p1 prt1
Output: prt1.a, (prt1.a + 1)
...
Expression 'a+1' is *computable* from the existing entry 'a', so we just
add a new resjunk target entry for 'a+1', and there is no error planning
this query. But if we change 'a+1' to something that is not computable,
then we would have problems (without your fix), and the reason has been
well explained by your messages.
explain (verbose, costs off)
select a, count(distinct b) from prt1 group by a order by a;
ERROR: could not find pathkey item to sort
Having said that, I think it's the right thing to do to trim off the new
added pathkeys, even if they are *computable*. In the plan above, the
'(prt1.a + 1)' in GroupAggregate's targetlist and MergeAppend's
pathkeys are actually redundant. It's good to remove it.
> Can you explain why you think we can put a resjunk "b" in the target
> list of the GroupAggregate in the above case?
Hmm, I don't think we can do that, because 'b' is not *computable* from
the existing target entries, as I explained above.
Thanks
Richard
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
@ 2023-10-09 04:13 ` David Rowley <[email protected]>
2023-10-09 05:41 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2024-03-13 17:00 ` Re: pg16: XX000: could not find pathkey item to sort Alexander Lakhin <[email protected]>
1 sibling, 2 replies; 15+ messages in thread
From: David Rowley @ 2023-10-09 04:13 UTC (permalink / raw)
To: Richard Guo <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]
On Mon, 9 Oct 2023 at 12:42, David Rowley <[email protected]> wrote:
> Maybe it's worth checking the total planning time spent in a run of
> the regression tests with and without the patch to see how much
> overhead it adds to the "average case".
I've now pushed the patch that trims off the Pathkeys for the ORDER BY
/ DISTINCT aggregates.
As for the patch to verify the pathkeys during create plan, I patched
master with the attached plan_times.patch.txt and used the following
to check the time spent in the planner for 3 runs of make
installcheck.
$ for i in {1..3}; do pg_ctl start -D pgdata -l plantime.log >
/dev/null && cd pg_src && make installcheck > /dev/null && cd .. &&
grep "planning time in" plantime.log|sed -E -e 's/.*planning time in
(.*) nanoseconds/\1/'|awk '{nanoseconds += $1} END{print nanoseconds}'
&& pg_ctl stop -D pgdata > /dev/null && rm plantime.log; done
Master:
1855788104
1839655412
1740769066
Patched:
1917797221
1766606115
1881322655
Those results are a bit noisy. Perhaps a few more runs might yield
more consistency, but it seems that there's not too much overhead to
it. If I take the minimum value out of the 3 runs from each, it comes
to about 1.5% extra time spent in planning. Perhaps that's OK.
David
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 1e4dd27dba..3c713782f1 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -17,6 +17,7 @@
#include <limits.h>
#include <math.h>
+#include <time.h>
#include "access/genam.h"
#include "access/htup_details.h"
@@ -274,11 +275,22 @@ planner(Query *parse, const char *query_string, int cursorOptions,
ParamListInfo boundParams)
{
PlannedStmt *result;
+ struct timespec start, end;
+
+ clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
if (planner_hook)
result = (*planner_hook) (parse, query_string, cursorOptions, boundParams);
else
result = standard_planner(parse, query_string, cursorOptions, boundParams);
+
+ clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
+
+ elog(LOG,
+ "planning time in %f nanoseconds",
+ ((double) (end.tv_sec * 1000000000 + end.tv_nsec) -
+ (double) (start.tv_sec * 1000000000 + start.tv_nsec)));
+
return result;
}
Attachments:
[text/plain] plan_times.patch.txt (1014B, ../../CAApHDvqira92LLPbW37azL+fDEm7yvWni89YekB9G751W7nEBw@mail.gmail.com/2-plan_times.patch.txt)
download | inline diff:
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 1e4dd27dba..3c713782f1 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -17,6 +17,7 @@
#include <limits.h>
#include <math.h>
+#include <time.h>
#include "access/genam.h"
#include "access/htup_details.h"
@@ -274,11 +275,22 @@ planner(Query *parse, const char *query_string, int cursorOptions,
ParamListInfo boundParams)
{
PlannedStmt *result;
+ struct timespec start, end;
+
+ clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
if (planner_hook)
result = (*planner_hook) (parse, query_string, cursorOptions, boundParams);
else
result = standard_planner(parse, query_string, cursorOptions, boundParams);
+
+ clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
+
+ elog(LOG,
+ "planning time in %f nanoseconds",
+ ((double) (end.tv_sec * 1000000000 + end.tv_nsec) -
+ (double) (start.tv_sec * 1000000000 + start.tv_nsec)));
+
return result;
}
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-09 04:13 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
@ 2023-10-09 05:41 ` Richard Guo <[email protected]>
1 sibling, 0 replies; 15+ messages in thread
From: Richard Guo @ 2023-10-09 05:41 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]
On Mon, Oct 9, 2023 at 12:13 PM David Rowley <[email protected]> wrote:
> I've now pushed the patch that trims off the Pathkeys for the ORDER BY
> / DISTINCT aggregates.
Thanks for pushing!
> Those results are a bit noisy. Perhaps a few more runs might yield
> more consistency, but it seems that there's not too much overhead to
> it. If I take the minimum value out of the 3 runs from each, it comes
> to about 1.5% extra time spent in planning. Perhaps that's OK.
I agree that the overhead is acceptable, especially it only happens in
USE_ASSERT_CHECKING builds.
Thanks
Richard
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-09 04:13 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
@ 2024-03-13 17:00 ` Alexander Lakhin <[email protected]>
2024-03-13 23:00 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
1 sibling, 1 reply; 15+ messages in thread
From: Alexander Lakhin @ 2024-03-13 17:00 UTC (permalink / raw)
To: David Rowley <[email protected]>; Richard Guo <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]
Hello David,
09.10.2023 07:13, David Rowley wrote:
> On Mon, 9 Oct 2023 at 12:42, David Rowley <[email protected]> wrote:
>> Maybe it's worth checking the total planning time spent in a run of
>> the regression tests with and without the patch to see how much
>> overhead it adds to the "average case".
> I've now pushed the patch that trims off the Pathkeys for the ORDER BY
> / DISTINCT aggregates.
>
I've stumbled upon the same error, but this time it apparently has another
cause. It can be produced (on REL_16_STABLE and master) as follows:
CREATE TABLE t (a int, b int) PARTITION BY RANGE (a);
CREATE TABLE td PARTITION OF t DEFAULT;
CREATE TABLE tp1 PARTITION OF t FOR VALUES FROM (1) TO (2);
SET enable_partitionwise_aggregate = on;
SET parallel_setup_cost = 0;
SELECT a, sum(b order by b) FROM t GROUP BY a ORDER BY a;
ERROR: could not find pathkey item to sort
`git bisect` for this anomaly blames the same commit 1349d2790.
Best regards,
Alexander
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-09 04:13 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-13 17:00 ` Re: pg16: XX000: could not find pathkey item to sort Alexander Lakhin <[email protected]>
@ 2024-03-13 23:00 ` David Rowley <[email protected]>
2024-03-14 05:23 ` Re: pg16: XX000: could not find pathkey item to sort Ashutosh Bapat <[email protected]>
2024-03-14 22:58 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
0 siblings, 2 replies; 15+ messages in thread
From: David Rowley @ 2024-03-13 23:00 UTC (permalink / raw)
To: Alexander Lakhin <[email protected]>; Robert Haas <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: Richard Guo <[email protected]>; Justin Pryzby <[email protected]>; [email protected]
On Thu, 14 Mar 2024 at 06:00, Alexander Lakhin <[email protected]> wrote:
> I've stumbled upon the same error, but this time it apparently has another
> cause. It can be produced (on REL_16_STABLE and master) as follows:
> CREATE TABLE t (a int, b int) PARTITION BY RANGE (a);
> CREATE TABLE td PARTITION OF t DEFAULT;
> CREATE TABLE tp1 PARTITION OF t FOR VALUES FROM (1) TO (2);
> SET enable_partitionwise_aggregate = on;
> SET parallel_setup_cost = 0;
> SELECT a, sum(b order by b) FROM t GROUP BY a ORDER BY a;
>
> ERROR: could not find pathkey item to sort
>
> `git bisect` for this anomaly blames the same commit 1349d2790.
Thanks for finding and for the recreator script.
I've attached a patch which fixes the problem for me.
On debugging this I uncovered some other stuff that looks broken which
seems to caused by partition-wise aggregates. With your example
query, in get_useful_pathkeys_for_relation(), we call
relation_can_be_sorted_early() to check if the pathkey can be used as
a set of pathkeys in useful_pathkeys_list. The problem is that in
your query the 'rel' is the base relation belonging to the partitioned
table and relation_can_be_sorted_early() looks through the targetlist
for that relation and finds columns "a" and "b" in there. The problem
is "b" has been aggregated away as partial aggregation has taken place
due to the partition-wise aggregation. I believe whichever rel we
should be using there should have an Aggref in the target exprs rather
than the plain unaggregated column. I've added Robert and Ashutosh to
see what their thoughts are on this.
David
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 443ab08d75..eaba6ddc03 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -7320,6 +7320,15 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
{
ListCell *lc;
Path *cheapest_partial_path;
+ List *groupby_pathkeys;
+
+
+ /* trim off any pathkeys added for ORDER BY / DISTINCT aggregates */
+ if (list_length(root->group_pathkeys) > root->num_groupby_pathkeys)
+ groupby_pathkeys = list_copy_head(root->group_pathkeys,
+ root->num_groupby_pathkeys);
+ else
+ groupby_pathkeys = root->group_pathkeys;
/* Try Gather for unordered paths and Gather Merge for ordered ones. */
generate_useful_gather_paths(root, rel, true);
@@ -7334,7 +7343,7 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
int presorted_keys;
double total_groups;
- is_sorted = pathkeys_count_contained_in(root->group_pathkeys,
+ is_sorted = pathkeys_count_contained_in(groupby_pathkeys,
path->pathkeys,
&presorted_keys);
@@ -7366,7 +7375,7 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
path = (Path *) create_incremental_sort_path(root,
rel,
path,
- root->group_pathkeys,
+ groupby_pathkeys,
presorted_keys,
-1.0);
@@ -7375,7 +7384,7 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
rel,
path,
rel->reltarget,
- root->group_pathkeys,
+ groupby_pathkeys,
NULL,
&total_groups);
Attachments:
[text/plain] fix_groupby_pathkeys.patch (1.6K, ../../CAApHDvqEdHSMwZt5ngXOBK+rZ=WU5iJdkvf+yD7qQJVu606fNg@mail.gmail.com/2-fix_groupby_pathkeys.patch)
download | inline diff:
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 443ab08d75..eaba6ddc03 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -7320,6 +7320,15 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
{
ListCell *lc;
Path *cheapest_partial_path;
+ List *groupby_pathkeys;
+
+
+ /* trim off any pathkeys added for ORDER BY / DISTINCT aggregates */
+ if (list_length(root->group_pathkeys) > root->num_groupby_pathkeys)
+ groupby_pathkeys = list_copy_head(root->group_pathkeys,
+ root->num_groupby_pathkeys);
+ else
+ groupby_pathkeys = root->group_pathkeys;
/* Try Gather for unordered paths and Gather Merge for ordered ones. */
generate_useful_gather_paths(root, rel, true);
@@ -7334,7 +7343,7 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
int presorted_keys;
double total_groups;
- is_sorted = pathkeys_count_contained_in(root->group_pathkeys,
+ is_sorted = pathkeys_count_contained_in(groupby_pathkeys,
path->pathkeys,
&presorted_keys);
@@ -7366,7 +7375,7 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
path = (Path *) create_incremental_sort_path(root,
rel,
path,
- root->group_pathkeys,
+ groupby_pathkeys,
presorted_keys,
-1.0);
@@ -7375,7 +7384,7 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
rel,
path,
rel->reltarget,
- root->group_pathkeys,
+ groupby_pathkeys,
NULL,
&total_groups);
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-09 04:13 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-13 17:00 ` Re: pg16: XX000: could not find pathkey item to sort Alexander Lakhin <[email protected]>
2024-03-13 23:00 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
@ 2024-03-14 05:23 ` Ashutosh Bapat <[email protected]>
2024-03-14 10:15 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
1 sibling, 1 reply; 15+ messages in thread
From: Ashutosh Bapat @ 2024-03-14 05:23 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Robert Haas <[email protected]>; Richard Guo <[email protected]>; Justin Pryzby <[email protected]>; [email protected]
On Thu, Mar 14, 2024 at 4:30 AM David Rowley <[email protected]> wrote:
> On Thu, 14 Mar 2024 at 06:00, Alexander Lakhin <[email protected]>
> wrote:
> > I've stumbled upon the same error, but this time it apparently has
> another
> > cause. It can be produced (on REL_16_STABLE and master) as follows:
> > CREATE TABLE t (a int, b int) PARTITION BY RANGE (a);
> > CREATE TABLE td PARTITION OF t DEFAULT;
> > CREATE TABLE tp1 PARTITION OF t FOR VALUES FROM (1) TO (2);
> > SET enable_partitionwise_aggregate = on;
> > SET parallel_setup_cost = 0;
> > SELECT a, sum(b order by b) FROM t GROUP BY a ORDER BY a;
> >
> > ERROR: could not find pathkey item to sort
> >
> > `git bisect` for this anomaly blames the same commit 1349d2790.
>
> Thanks for finding and for the recreator script.
>
> I've attached a patch which fixes the problem for me.
>
> On debugging this I uncovered some other stuff that looks broken which
> seems to caused by partition-wise aggregates. With your example
> query, in get_useful_pathkeys_for_relation(), we call
> relation_can_be_sorted_early() to check if the pathkey can be used as
> a set of pathkeys in useful_pathkeys_list. The problem is that in
> your query the 'rel' is the base relation belonging to the partitioned
> table and relation_can_be_sorted_early() looks through the targetlist
> for that relation and finds columns "a" and "b" in there. The problem
> is "b" has been aggregated away as partial aggregation has taken place
> due to the partition-wise aggregation. I believe whichever rel we
> should be using there should have an Aggref in the target exprs rather
> than the plain unaggregated column. I've added Robert and Ashutosh to
> see what their thoughts are on this.
>
I don't understand why root->query_pathkeys has both a and b. "a" is there
because of GROUP BY and ORDER BY clause. But why "b"?
Under the debugger this is what I observed: generate_useful_gather_paths()
gets called twice, once for the base relation and second time for the upper
relation.
When it's called for base relation, it includes "a" and "b" both in the
useful pathkeys. The plan doesn't use sortedness on b. But I don't think
that's the problem of the relation used. It looks like root->query_pathkeys
containing "b" may be a problem.
When it's called for upper relation, the reltarget has "a" and Aggref() and
it includes only "a" in the useful pathkeys which is as per your
expectation.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-09 04:13 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-13 17:00 ` Re: pg16: XX000: could not find pathkey item to sort Alexander Lakhin <[email protected]>
2024-03-13 23:00 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-14 05:23 ` Re: pg16: XX000: could not find pathkey item to sort Ashutosh Bapat <[email protected]>
@ 2024-03-14 10:15 ` David Rowley <[email protected]>
2024-03-18 05:50 ` Re: pg16: XX000: could not find pathkey item to sort Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: David Rowley @ 2024-03-14 10:15 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Robert Haas <[email protected]>; Richard Guo <[email protected]>; Justin Pryzby <[email protected]>; [email protected]
On Thu, 14 Mar 2024 at 18:23, Ashutosh Bapat
<[email protected]> wrote:
> I don't understand why root->query_pathkeys has both a and b. "a" is there because of GROUP BY and ORDER BY clause. But why "b"?
So that the ORDER BY aggregate function can be evaluated without
nodeAgg.c having to perform the sort. See
adjust_group_pathkeys_for_groupagg().
David
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-09 04:13 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-13 17:00 ` Re: pg16: XX000: could not find pathkey item to sort Alexander Lakhin <[email protected]>
2024-03-13 23:00 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-14 05:23 ` Re: pg16: XX000: could not find pathkey item to sort Ashutosh Bapat <[email protected]>
2024-03-14 10:15 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
@ 2024-03-18 05:50 ` Ashutosh Bapat <[email protected]>
2024-03-20 01:28 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Ashutosh Bapat @ 2024-03-18 05:50 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Robert Haas <[email protected]>; Richard Guo <[email protected]>; Justin Pryzby <[email protected]>; [email protected]
On Thu, Mar 14, 2024 at 3:45 PM David Rowley <[email protected]> wrote:
> On Thu, 14 Mar 2024 at 18:23, Ashutosh Bapat
> <[email protected]> wrote:
> > I don't understand why root->query_pathkeys has both a and b. "a" is
> there because of GROUP BY and ORDER BY clause. But why "b"?
>
> So that the ORDER BY aggregate function can be evaluated without
> nodeAgg.c having to perform the sort. See
> adjust_group_pathkeys_for_groupagg().
>
Thanks. To me, it looks like we are gathering pathkeys, which if used to
sort the result of overall join, would avoid sorting in as many as
aggregates as possible.
relation_can_be_sorted_early() finds, pathkeys which if used to sort the
given relation, would help sorting the overall join. Contrary to what I
said earlier, it might help if the base relation is sorted on "a" and "b".
What I find weird is that the sorting is not pushed down to the partitions,
where it would help most.
#explain verbose SELECT a, sum(b order by b) FROM t GROUP BY a ORDER BY a;
QUERY PLAN
------------------------------------------------------------------------------------
GroupAggregate (cost=362.21..398.11 rows=200 width=12)
Output: t.a, sum(t.b ORDER BY t.b)
Group Key: t.a
-> Sort (cost=362.21..373.51 rows=4520 width=8)
Output: t.a, t.b
Sort Key: t.a, t.b
-> Append (cost=0.00..87.80 rows=4520 width=8)
-> Seq Scan on public.tp1 t_1 (cost=0.00..32.60 rows=2260
width=8)
Output: t_1.a, t_1.b
-> Seq Scan on public.td t_2 (cost=0.00..32.60 rows=2260
width=8)
Output: t_2.a, t_2.b
(11 rows)
and that's the case even without parallel plans
#explain verbose SELECT a, sum(b order by b) FROM t GROUP BY a ORDER BY a;
QUERY PLAN
------------------------------------------------------------------------------------
GroupAggregate (cost=362.21..398.11 rows=200 width=12)
Output: t.a, sum(t.b ORDER BY t.b)
Group Key: t.a
-> Sort (cost=362.21..373.51 rows=4520 width=8)
Output: t.a, t.b
Sort Key: t.a, t.b
-> Append (cost=0.00..87.80 rows=4520 width=8)
-> Seq Scan on public.tp1 t_1 (cost=0.00..32.60 rows=2260
width=8)
Output: t_1.a, t_1.b
-> Seq Scan on public.td t_2 (cost=0.00..32.60 rows=2260
width=8)
Output: t_2.a, t_2.b
(11 rows)
But it could be just because the corresponding plan was not found to be
optimal. May be because there isn't enough data in those tables.
If the problem you speculate is different from this one, I am not able to
see it. It might help give an example query or explain more.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-09 04:13 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-13 17:00 ` Re: pg16: XX000: could not find pathkey item to sort Alexander Lakhin <[email protected]>
2024-03-13 23:00 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-14 05:23 ` Re: pg16: XX000: could not find pathkey item to sort Ashutosh Bapat <[email protected]>
2024-03-14 10:15 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-18 05:50 ` Re: pg16: XX000: could not find pathkey item to sort Ashutosh Bapat <[email protected]>
@ 2024-03-20 01:28 ` David Rowley <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: David Rowley @ 2024-03-20 01:28 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Robert Haas <[email protected]>; Richard Guo <[email protected]>; Justin Pryzby <[email protected]>; [email protected]
On Mon, 18 Mar 2024 at 18:50, Ashutosh Bapat
<[email protected]> wrote:
> If the problem you speculate is different from this one, I am not able to see it. It might help give an example query or explain more.
I looked at this again and I might have been wrong about there being a
problem. I set a breakpoint in create_gather_merge_path() and
adjusted the startup and total cost to 1 when I saw the pathkeys
containing {a,b}. It turns out this is the non-partitionwise
aggregate path, and of course, the targetlist there does contain the
"b" column, so it's fine in that case that the pathkeys are {a,b}. I
had previously thought that this was for the partition-wise aggregate
plan, in which case the targetlist would contain a, sum(b order by b),
of which there's no single value of "b" that we can legally sort by.
Here's the full plan.
postgres=# explain verbose SELECT a, sum(b order by b) FROM t GROUP BY
a ORDER BY a;
QUERY PLAN
---------------------------------------------------------------------------------------------------
GroupAggregate (cost=1.00..25.60 rows=200 width=12)
Output: t.a, sum(t.b ORDER BY t.b)
Group Key: t.a
-> Gather Merge (cost=1.00..1.00 rows=4520 width=8)
Output: t.a, t.b
Workers Planned: 2
-> Sort (cost=158.36..163.07 rows=1882 width=8)
Output: t.a, t.b
Sort Key: t.a, t.b
-> Parallel Append (cost=0.00..56.00 rows=1882 width=8)
-> Parallel Seq Scan on public.tp1 t_1
(cost=0.00..23.29 rows=1329 width=8)
Output: t_1.a, t_1.b
-> Parallel Seq Scan on public.td t_2
(cost=0.00..23.29 rows=1329 width=8)
Output: t_2.a, t_2.b
(14 rows)
David
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pg16: XX000: could not find pathkey item to sort
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-09 04:13 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-13 17:00 ` Re: pg16: XX000: could not find pathkey item to sort Alexander Lakhin <[email protected]>
2024-03-13 23:00 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
@ 2024-03-14 22:58 ` David Rowley <[email protected]>
1 sibling, 0 replies; 15+ messages in thread
From: David Rowley @ 2024-03-14 22:58 UTC (permalink / raw)
To: Alexander Lakhin <[email protected]>; Robert Haas <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: Richard Guo <[email protected]>; Justin Pryzby <[email protected]>; [email protected]
On Thu, 14 Mar 2024 at 12:00, David Rowley <[email protected]> wrote:
> I've attached a patch which fixes the problem for me.
I've pushed the patch to fix gather_grouping_paths(). The issue with
the RelOptInfo having the incorrect PathTarget->exprs after the
partial phase of partition-wise aggregate remains.
David
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2024-03-20 01:28 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-01-31 15:13 [PATCH] fix CREATE INDEX progress report with nested partitions Ilya Gladyshev <[email protected]>
2023-10-03 07:16 Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-05 06:26 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-08 10:52 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-08 23:42 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-09 02:08 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2023-10-09 04:13 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2023-10-09 05:41 ` Re: pg16: XX000: could not find pathkey item to sort Richard Guo <[email protected]>
2024-03-13 17:00 ` Re: pg16: XX000: could not find pathkey item to sort Alexander Lakhin <[email protected]>
2024-03-13 23:00 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-14 05:23 ` Re: pg16: XX000: could not find pathkey item to sort Ashutosh Bapat <[email protected]>
2024-03-14 10:15 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-18 05:50 ` Re: pg16: XX000: could not find pathkey item to sort Ashutosh Bapat <[email protected]>
2024-03-20 01:28 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[email protected]>
2024-03-14 22:58 ` Re: pg16: XX000: could not find pathkey item to sort David Rowley <[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