pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedDefer selection of asynchronous subplans until the executor initialization stage
33+ messages / 6 participants
[nested] [flat]
* Defer selection of asynchronous subplans until the executor initialization stage
@ 2021-05-11 03:45 Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 33+ messages in thread
From: Andrey Lepikhov @ 2021-05-11 03:45 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 7/5/21 21:05, Etsuro Fujita wrote:
> I think it would be better to start a new thread for this, and add the
> patch to the next CF so that it doesn’t get lost.
Current implementation of async append choose asynchronous subplans at
the phase of an append plan creation. This is safe approach, but we
loose some optimizations, such of flattening trivial subqueries and
can't execute some simple queries asynchronously. For example:
EXPLAIN (ANALYZE, TIMING OFF, SUMMARY OFF, COSTS OFF)
(SELECT * FROM f1 WHERE a < 10) UNION ALL
(SELECT * FROM f2 WHERE a < 10);
But, as I could understand, we can choose these subplans later, at the
init append phase when all optimizations already passed.
In attachment - implementation of the proposed approach.
Initial script for the example see in the parent thread [1].
[1]
https://www.postgresql.org/message-id/a38bb206-8340-9528-5ef6-37de2d5cb1a3%40postgrespro.ru
--
regards,
Andrey Lepikhov
Postgres Professional
From 395b1d62389cf40520a4afd87c11301aa2b17df2 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Tue, 11 May 2021 08:43:03 +0500
Subject: [PATCH] Defer selection of asynchronous subplans to the executor
initial phase.
---
contrib/postgres_fdw/postgres_fdw.c | 10 +++++++++-
src/backend/executor/execAmi.c | 7 +++----
src/backend/executor/nodeAppend.c | 19 +++++++++++++++++++
src/backend/nodes/copyfuncs.c | 1 -
src/backend/nodes/outfuncs.c | 1 -
src/backend/nodes/readfuncs.c | 1 -
src/backend/optimizer/plan/createplan.c | 17 +----------------
src/include/nodes/plannodes.h | 1 -
src/include/optimizer/planmain.h | 1 +
9 files changed, 33 insertions(+), 25 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 4ff58d9c27..3e151a6790 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1245,6 +1245,7 @@ postgresGetForeignPlan(PlannerInfo *root,
bool has_final_sort = false;
bool has_limit = false;
ListCell *lc;
+ ForeignScan *fsplan;
/*
* Get FDW private data created by postgresGetForeignUpperPaths(), if any.
@@ -1429,7 +1430,7 @@ postgresGetForeignPlan(PlannerInfo *root,
* field of the finished plan node; we can't keep them in private state
* because then they wouldn't be subject to later planner processing.
*/
- return make_foreignscan(tlist,
+ fsplan = make_foreignscan(tlist,
local_exprs,
scan_relid,
params_list,
@@ -1437,6 +1438,13 @@ postgresGetForeignPlan(PlannerInfo *root,
fdw_scan_tlist,
fdw_recheck_quals,
outer_plan);
+
+ /* If appropriate, consider participation in async operations */
+ fsplan->scan.plan.async_capable = (enable_async_append &&
+ best_path->path.pathkeys == NIL &&
+ !fsplan->scan.plan.parallel_safe &&
+ is_async_capable_path((Path *)best_path));
+ return fsplan;
}
/*
diff --git a/src/backend/executor/execAmi.c b/src/backend/executor/execAmi.c
index b3726a54f3..4e70f4eb54 100644
--- a/src/backend/executor/execAmi.c
+++ b/src/backend/executor/execAmi.c
@@ -524,6 +524,9 @@ ExecSupportsBackwardScan(Plan *node)
if (node->parallel_aware)
return false;
+ if (node->async_capable)
+ return false;
+
switch (nodeTag(node))
{
case T_Result:
@@ -536,10 +539,6 @@ ExecSupportsBackwardScan(Plan *node)
{
ListCell *l;
- /* With async, tuples may be interleaved, so can't back up. */
- if (((Append *) node)->nasyncplans > 0)
- return false;
-
foreach(l, ((Append *) node)->appendplans)
{
if (!ExecSupportsBackwardScan((Plan *) lfirst(l)))
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index 3c1f12adaf..363cf9f4a5 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -117,6 +117,8 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
int firstvalid;
int i,
j;
+ ListCell *l;
+ bool consider_async = false;
/* check for unsupported flags */
Assert(!(eflags & EXEC_FLAG_MARK));
@@ -197,6 +199,23 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
appendplanstates = (PlanState **) palloc(nplans *
sizeof(PlanState *));
+ /* If appropriate, consider async append */
+ consider_async = (list_length(node->appendplans) > 1);
+
+ if (!consider_async)
+ {
+ foreach(l, node->appendplans)
+ {
+ Plan *subplan = (Plan *) lfirst(l);
+
+ /* Check to see if subplan can be executed asynchronously */
+ if (subplan->async_capable)
+ {
+ subplan->async_capable = false;
+ }
+ }
+ }
+
/*
* call ExecInitNode on each of the valid plans to be executed and save
* the results into the appendplanstates array.
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 90770a89b0..a44185d7fc 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -243,7 +243,6 @@ _copyAppend(const Append *from)
*/
COPY_BITMAPSET_FIELD(apprelids);
COPY_NODE_FIELD(appendplans);
- COPY_SCALAR_FIELD(nasyncplans);
COPY_SCALAR_FIELD(first_partial_plan);
COPY_NODE_FIELD(part_prune_info);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 8da8b14f0e..cd5dbce76a 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -433,7 +433,6 @@ _outAppend(StringInfo str, const Append *node)
WRITE_BITMAPSET_FIELD(apprelids);
WRITE_NODE_FIELD(appendplans);
- WRITE_INT_FIELD(nasyncplans);
WRITE_INT_FIELD(first_partial_plan);
WRITE_NODE_FIELD(part_prune_info);
}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 3772ea07df..3f5879951b 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1717,7 +1717,6 @@ _readAppend(void)
READ_BITMAPSET_FIELD(apprelids);
READ_NODE_FIELD(appendplans);
- READ_INT_FIELD(nasyncplans);
READ_INT_FIELD(first_partial_plan);
READ_NODE_FIELD(part_prune_info);
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 7003238d76..b1f2a493f0 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -82,7 +82,6 @@ static List *get_gating_quals(PlannerInfo *root, List *quals);
static Plan *create_gating_plan(PlannerInfo *root, Path *path, Plan *plan,
List *gating_quals);
static Plan *create_join_plan(PlannerInfo *root, JoinPath *best_path);
-static bool is_async_capable_path(Path *path);
static Plan *create_append_plan(PlannerInfo *root, AppendPath *best_path,
int flags);
static Plan *create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path,
@@ -1097,7 +1096,7 @@ create_join_plan(PlannerInfo *root, JoinPath *best_path)
* is_async_capable_path
* Check whether a given Path node is async-capable.
*/
-static bool
+bool
is_async_capable_path(Path *path)
{
switch (nodeTag(path))
@@ -1135,7 +1134,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
List *pathkeys = best_path->path.pathkeys;
List *subplans = NIL;
ListCell *subpaths;
- int nasyncplans = 0;
RelOptInfo *rel = best_path->path.parent;
PartitionPruneInfo *partpruneinfo = NULL;
int nodenumsortkeys = 0;
@@ -1143,7 +1141,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
Oid *nodeSortOperators = NULL;
Oid *nodeCollations = NULL;
bool *nodeNullsFirst = NULL;
- bool consider_async = false;
/*
* The subpaths list could be empty, if every child was proven empty by
@@ -1207,11 +1204,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
tlist_was_changed = (orig_tlist_length != list_length(plan->plan.targetlist));
}
- /* If appropriate, consider async append */
- consider_async = (enable_async_append && pathkeys == NIL &&
- !best_path->path.parallel_safe &&
- list_length(best_path->subpaths) > 1);
-
/* Build the plan for each child */
foreach(subpaths, best_path->subpaths)
{
@@ -1280,12 +1272,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
subplans = lappend(subplans, subplan);
- /* Check to see if subplan can be executed asynchronously */
- if (consider_async && is_async_capable_path(subpath))
- {
- subplan->async_capable = true;
- ++nasyncplans;
- }
}
/*
@@ -1318,7 +1304,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
}
plan->appendplans = subplans;
- plan->nasyncplans = nasyncplans;
plan->first_partial_plan = best_path->first_partial_path;
plan->part_prune_info = partpruneinfo;
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 841401be20..00f4f5f8ed 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -251,7 +251,6 @@ typedef struct Append
Plan plan;
Bitmapset *apprelids; /* RTIs of appendrel(s) formed by this node */
List *appendplans;
- int nasyncplans; /* # of asynchronous plans */
/*
* All 'appendplans' preceding this index are non-partial plans. All
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index bf1adfc52a..8a96a19e5f 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -115,5 +115,6 @@ extern Plan *set_plan_references(PlannerInfo *root, Plan *plan);
extern void record_plan_function_dependency(PlannerInfo *root, Oid funcid);
extern void record_plan_type_dependency(PlannerInfo *root, Oid typid);
extern bool extract_query_dependencies_walker(Node *node, PlannerInfo *root);
+extern bool is_async_capable_path(Path *path);
#endif /* PLANMAIN_H */
--
2.31.1
Attachments:
[text/plain] 0001-Defer-selection-of-asynchronous-subplans-to-the-exec.patch (8.6K, ../../659c37a8-3e71-0ff2-394c-f04428c76f08@postgrespro.ru/2-0001-Defer-selection-of-asynchronous-subplans-to-the-exec.patch)
download | inline diff:
From 395b1d62389cf40520a4afd87c11301aa2b17df2 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Tue, 11 May 2021 08:43:03 +0500
Subject: [PATCH] Defer selection of asynchronous subplans to the executor
initial phase.
---
contrib/postgres_fdw/postgres_fdw.c | 10 +++++++++-
src/backend/executor/execAmi.c | 7 +++----
src/backend/executor/nodeAppend.c | 19 +++++++++++++++++++
src/backend/nodes/copyfuncs.c | 1 -
src/backend/nodes/outfuncs.c | 1 -
src/backend/nodes/readfuncs.c | 1 -
src/backend/optimizer/plan/createplan.c | 17 +----------------
src/include/nodes/plannodes.h | 1 -
src/include/optimizer/planmain.h | 1 +
9 files changed, 33 insertions(+), 25 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 4ff58d9c27..3e151a6790 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1245,6 +1245,7 @@ postgresGetForeignPlan(PlannerInfo *root,
bool has_final_sort = false;
bool has_limit = false;
ListCell *lc;
+ ForeignScan *fsplan;
/*
* Get FDW private data created by postgresGetForeignUpperPaths(), if any.
@@ -1429,7 +1430,7 @@ postgresGetForeignPlan(PlannerInfo *root,
* field of the finished plan node; we can't keep them in private state
* because then they wouldn't be subject to later planner processing.
*/
- return make_foreignscan(tlist,
+ fsplan = make_foreignscan(tlist,
local_exprs,
scan_relid,
params_list,
@@ -1437,6 +1438,13 @@ postgresGetForeignPlan(PlannerInfo *root,
fdw_scan_tlist,
fdw_recheck_quals,
outer_plan);
+
+ /* If appropriate, consider participation in async operations */
+ fsplan->scan.plan.async_capable = (enable_async_append &&
+ best_path->path.pathkeys == NIL &&
+ !fsplan->scan.plan.parallel_safe &&
+ is_async_capable_path((Path *)best_path));
+ return fsplan;
}
/*
diff --git a/src/backend/executor/execAmi.c b/src/backend/executor/execAmi.c
index b3726a54f3..4e70f4eb54 100644
--- a/src/backend/executor/execAmi.c
+++ b/src/backend/executor/execAmi.c
@@ -524,6 +524,9 @@ ExecSupportsBackwardScan(Plan *node)
if (node->parallel_aware)
return false;
+ if (node->async_capable)
+ return false;
+
switch (nodeTag(node))
{
case T_Result:
@@ -536,10 +539,6 @@ ExecSupportsBackwardScan(Plan *node)
{
ListCell *l;
- /* With async, tuples may be interleaved, so can't back up. */
- if (((Append *) node)->nasyncplans > 0)
- return false;
-
foreach(l, ((Append *) node)->appendplans)
{
if (!ExecSupportsBackwardScan((Plan *) lfirst(l)))
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index 3c1f12adaf..363cf9f4a5 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -117,6 +117,8 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
int firstvalid;
int i,
j;
+ ListCell *l;
+ bool consider_async = false;
/* check for unsupported flags */
Assert(!(eflags & EXEC_FLAG_MARK));
@@ -197,6 +199,23 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
appendplanstates = (PlanState **) palloc(nplans *
sizeof(PlanState *));
+ /* If appropriate, consider async append */
+ consider_async = (list_length(node->appendplans) > 1);
+
+ if (!consider_async)
+ {
+ foreach(l, node->appendplans)
+ {
+ Plan *subplan = (Plan *) lfirst(l);
+
+ /* Check to see if subplan can be executed asynchronously */
+ if (subplan->async_capable)
+ {
+ subplan->async_capable = false;
+ }
+ }
+ }
+
/*
* call ExecInitNode on each of the valid plans to be executed and save
* the results into the appendplanstates array.
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 90770a89b0..a44185d7fc 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -243,7 +243,6 @@ _copyAppend(const Append *from)
*/
COPY_BITMAPSET_FIELD(apprelids);
COPY_NODE_FIELD(appendplans);
- COPY_SCALAR_FIELD(nasyncplans);
COPY_SCALAR_FIELD(first_partial_plan);
COPY_NODE_FIELD(part_prune_info);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 8da8b14f0e..cd5dbce76a 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -433,7 +433,6 @@ _outAppend(StringInfo str, const Append *node)
WRITE_BITMAPSET_FIELD(apprelids);
WRITE_NODE_FIELD(appendplans);
- WRITE_INT_FIELD(nasyncplans);
WRITE_INT_FIELD(first_partial_plan);
WRITE_NODE_FIELD(part_prune_info);
}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 3772ea07df..3f5879951b 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1717,7 +1717,6 @@ _readAppend(void)
READ_BITMAPSET_FIELD(apprelids);
READ_NODE_FIELD(appendplans);
- READ_INT_FIELD(nasyncplans);
READ_INT_FIELD(first_partial_plan);
READ_NODE_FIELD(part_prune_info);
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 7003238d76..b1f2a493f0 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -82,7 +82,6 @@ static List *get_gating_quals(PlannerInfo *root, List *quals);
static Plan *create_gating_plan(PlannerInfo *root, Path *path, Plan *plan,
List *gating_quals);
static Plan *create_join_plan(PlannerInfo *root, JoinPath *best_path);
-static bool is_async_capable_path(Path *path);
static Plan *create_append_plan(PlannerInfo *root, AppendPath *best_path,
int flags);
static Plan *create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path,
@@ -1097,7 +1096,7 @@ create_join_plan(PlannerInfo *root, JoinPath *best_path)
* is_async_capable_path
* Check whether a given Path node is async-capable.
*/
-static bool
+bool
is_async_capable_path(Path *path)
{
switch (nodeTag(path))
@@ -1135,7 +1134,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
List *pathkeys = best_path->path.pathkeys;
List *subplans = NIL;
ListCell *subpaths;
- int nasyncplans = 0;
RelOptInfo *rel = best_path->path.parent;
PartitionPruneInfo *partpruneinfo = NULL;
int nodenumsortkeys = 0;
@@ -1143,7 +1141,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
Oid *nodeSortOperators = NULL;
Oid *nodeCollations = NULL;
bool *nodeNullsFirst = NULL;
- bool consider_async = false;
/*
* The subpaths list could be empty, if every child was proven empty by
@@ -1207,11 +1204,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
tlist_was_changed = (orig_tlist_length != list_length(plan->plan.targetlist));
}
- /* If appropriate, consider async append */
- consider_async = (enable_async_append && pathkeys == NIL &&
- !best_path->path.parallel_safe &&
- list_length(best_path->subpaths) > 1);
-
/* Build the plan for each child */
foreach(subpaths, best_path->subpaths)
{
@@ -1280,12 +1272,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
subplans = lappend(subplans, subplan);
- /* Check to see if subplan can be executed asynchronously */
- if (consider_async && is_async_capable_path(subpath))
- {
- subplan->async_capable = true;
- ++nasyncplans;
- }
}
/*
@@ -1318,7 +1304,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
}
plan->appendplans = subplans;
- plan->nasyncplans = nasyncplans;
plan->first_partial_plan = best_path->first_partial_path;
plan->part_prune_info = partpruneinfo;
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 841401be20..00f4f5f8ed 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -251,7 +251,6 @@ typedef struct Append
Plan plan;
Bitmapset *apprelids; /* RTIs of appendrel(s) formed by this node */
List *appendplans;
- int nasyncplans; /* # of asynchronous plans */
/*
* All 'appendplans' preceding this index are non-partial plans. All
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index bf1adfc52a..8a96a19e5f 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -115,5 +115,6 @@ extern Plan *set_plan_references(PlannerInfo *root, Plan *plan);
extern void record_plan_function_dependency(PlannerInfo *root, Oid funcid);
extern void record_plan_type_dependency(PlannerInfo *root, Oid typid);
extern bool extract_query_dependencies_walker(Node *node, PlannerInfo *root);
+extern bool is_async_capable_path(Path *path);
#endif /* PLANMAIN_H */
--
2.31.1
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2021-05-11 03:55 Zhihong Yu <zyu@yugabyte.com>
parent: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 2 replies; 33+ messages in thread
From: Zhihong Yu @ 2021-05-11 03:55 UTC (permalink / raw)
To: Andrey Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: Etsuro Fujita <etsuro.fujita@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, May 10, 2021 at 8:45 PM Andrey Lepikhov <a.lepikhov@postgrespro.ru>
wrote:
> On 7/5/21 21:05, Etsuro Fujita wrote:
> > I think it would be better to start a new thread for this, and add the
> > patch to the next CF so that it doesn’t get lost.
>
> Current implementation of async append choose asynchronous subplans at
> the phase of an append plan creation. This is safe approach, but we
> loose some optimizations, such of flattening trivial subqueries and
> can't execute some simple queries asynchronously. For example:
>
> EXPLAIN (ANALYZE, TIMING OFF, SUMMARY OFF, COSTS OFF)
> (SELECT * FROM f1 WHERE a < 10) UNION ALL
> (SELECT * FROM f2 WHERE a < 10);
>
> But, as I could understand, we can choose these subplans later, at the
> init append phase when all optimizations already passed.
> In attachment - implementation of the proposed approach.
>
> Initial script for the example see in the parent thread [1].
>
>
> [1]
>
> https://www.postgresql.org/message-id/a38bb206-8340-9528-5ef6-37de2d5cb1a3%40postgrespro.ru
>
>
> --
> regards,
> Andrey Lepikhov
> Postgres Professional
>
Hi,
+ /* Check to see if subplan can be executed asynchronously */
+ if (subplan->async_capable)
+ {
+ subplan->async_capable = false;
It seems the if statement is not needed: you can directly assign false
to subplan->async_capable.
Cheers
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2021-05-11 07:06 Andrey Lepikhov <a.lepikhov@postgrespro.ru>
parent: Zhihong Yu <zyu@yugabyte.com>
1 sibling, 0 replies; 33+ messages in thread
From: Andrey Lepikhov @ 2021-05-11 07:06 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Etsuro Fujita <etsuro.fujita@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 11/5/21 08:55, Zhihong Yu wrote:
> + /* Check to see if subplan can be executed asynchronously */
> + if (subplan->async_capable)
> + {
> + subplan->async_capable = false;
>
> It seems the if statement is not needed: you can directly assign false
> to subplan->async_capable.Thank you, I agree with you.
Close look into the postgres_fdw regression tests show at least one open
problem with this approach: we need to control situations when only one
partition doesn't pruned and append isn't exist at all.
--
regards,
Andrey Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2021-06-30 04:50 Andrey Lepikhov <a.lepikhov@postgrespro.ru>
parent: Zhihong Yu <zyu@yugabyte.com>
1 sibling, 1 reply; 33+ messages in thread
From: Andrey Lepikhov @ 2021-06-30 04:50 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Etsuro Fujita <etsuro.fujita@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 11/5/21 06:55, Zhihong Yu wrote:
> On Mon, May 10, 2021 at 8:45 PM Andrey Lepikhov
> <a.lepikhov@postgrespro.ru <mailto:a.lepikhov@postgrespro.ru>> wrote:
> It seems the if statement is not needed: you can directly assign false
> to subplan->async_capable.
I have completely rewritten this patch.
Main idea:
The async_capable field of a plan node inform us that this node could
work in async mode. Each node sets this field based on its own logic.
The actual mode of a node is defined by the async_capable of PlanState
structure. It is made at the executor initialization stage.
In this patch, only an append node could define async behaviour for its
subplans.
With such approach the IsForeignPathAsyncCapable routine become
unecessary, I think.
--
regards,
Andrey Lepikhov
Postgres Professional
From d935bbb70565d70f1b0f547bf37e71ffc6fa2ef2 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Tue, 29 Jun 2021 22:09:54 +0300
Subject: [PATCH] Choose async append subplans at the initial execution stage
---
contrib/file_fdw/file_fdw.c | 3 +-
.../postgres_fdw/expected/postgres_fdw.out | 81 ++++++++++++++++++-
contrib/postgres_fdw/postgres_fdw.c | 13 +--
contrib/postgres_fdw/sql/postgres_fdw.sql | 29 +++++++
src/backend/commands/explain.c | 2 +-
src/backend/executor/execAmi.c | 4 -
src/backend/executor/nodeAppend.c | 27 ++++---
src/backend/executor/nodeForeignscan.c | 7 --
src/backend/nodes/copyfuncs.c | 1 -
src/backend/nodes/outfuncs.c | 1 -
src/backend/nodes/readfuncs.c | 1 -
src/backend/optimizer/path/costsize.c | 1 -
src/backend/optimizer/plan/createplan.c | 45 +----------
src/backend/utils/misc/guc.c | 1 +
src/include/executor/nodeAppend.h | 2 +
src/include/nodes/plannodes.h | 1 -
src/include/optimizer/cost.h | 1 -
src/include/optimizer/planmain.h | 2 +-
18 files changed, 141 insertions(+), 81 deletions(-)
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 2c2f149fb0..5f67e1ca94 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -609,7 +609,8 @@ fileGetForeignPlan(PlannerInfo *root,
best_path->fdw_private,
NIL, /* no custom tlist */
NIL, /* no remote quals */
- outer_plan);
+ outer_plan,
+ false);
}
/*
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 31b5de91ad..30c38c6992 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10169,7 +10169,7 @@ EXECUTE async_pt_query (2000, 505);
Insert on public.result_tbl
-> Append
Subplans Removed: 2
- -> Async Foreign Scan on public.async_p1 async_pt_1
+ -> Foreign Scan on public.async_p1 async_pt_1
Output: async_pt_1.a, async_pt_1.b, async_pt_1.c
Filter: (async_pt_1.b === $2)
Remote SQL: SELECT a, b, c FROM public.base_tbl1 WHERE ((a < $1::integer))
@@ -10237,6 +10237,85 @@ SELECT * FROM local_tbl, async_pt WHERE local_tbl.a = async_pt.a AND local_tbl.c
2505 | 505 | bar | 2505 | 505 | 0505
(1 row)
+-- Subquery flattening must be done before choosing of async plans.
+EXPLAIN (VERBOSE, COSTS OFF)
+(SELECT * FROM async_p1 LIMIT 1)
+ UNION ALL
+(SELECT * FROM async_p2 WHERE a < 5)
+ UNION ALL
+(SELECT * FROM async_p2)
+ UNION ALL
+(SELECT * FROM async_p3 LIMIT 3);
+ QUERY PLAN
+--------------------------------------------------------------------------
+ Append
+ -> Async Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1 LIMIT 1::bigint
+ -> Async Foreign Scan on public.async_p2 async_p2_1
+ Output: async_p2_1.a, async_p2_1.b, async_p2_1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((a < 5))
+ -> Async Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2
+ -> Limit
+ Output: async_p3.a, async_p3.b, async_p3.c
+ -> Seq Scan on public.async_p3
+ Output: async_p3.a, async_p3.b, async_p3.c
+(14 rows)
+
+-- Check that async append doesn't break the scrollable cursors logic:
+-- If the query plan doesn't support backward scan, a materialize node will be
+-- inserted in the head of this plan.
+BEGIN;
+EXPLAIN (COSTS OFF)
+DECLARE curs1 SCROLL CURSOR FOR (SELECT * FROM async_p3);
+ QUERY PLAN
+----------------------
+ Seq Scan on async_p3
+(1 row)
+
+EXPLAIN (COSTS OFF)
+DECLARE curs1 SCROLL CURSOR FOR (SELECT * FROM async_pt);
+ QUERY PLAN
+-------------------------------------------------------
+ Materialize
+ -> Append
+ -> Async Foreign Scan on async_p1 async_pt_1
+ -> Async Foreign Scan on async_p2 async_pt_2
+ -> Seq Scan on async_p3 async_pt_3
+(5 rows)
+
+EXPLAIN (COSTS OFF)
+DECLARE curs1 NO SCROLL CURSOR FOR (SELECT * FROM async_p1);
+ QUERY PLAN
+--------------------------
+ Foreign Scan on async_p1
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF)
+DECLARE curs2 SCROLL CURSOR FOR
+ (SELECT * FROM async_p1)
+ UNION ALL
+ (SELECT * FROM async_p2 WHERE a < 5)
+ UNION ALL
+ (SELECT * FROM async_p3);
+ QUERY PLAN
+--------------------------------------------------------------------------------
+ Materialize
+ Output: async_p1.a, async_p1.b, async_p1.c
+ -> Append
+ -> Async Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1
+ -> Async Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((a < 5))
+ -> Seq Scan on public.async_p3
+ Output: async_p3.a, async_p3.b, async_p3.c
+(11 rows)
+
+ROLLBACK;
ALTER FOREIGN TABLE async_p1 OPTIONS (DROP use_remote_estimate);
ALTER FOREIGN TABLE async_p2 OPTIONS (DROP use_remote_estimate);
DROP TABLE local_tbl;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index fafbab6b02..6a52859f5e 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -163,9 +163,6 @@ typedef struct PgFdwScanState
int fetch_ct_2; /* Min(# of fetches done, 2) */
bool eof_reached; /* true if last fetch reached EOF */
- /* for asynchronous execution */
- bool async_capable; /* engage asynchronous-capable logic? */
-
/* working memory contexts */
MemoryContext batch_cxt; /* context holding current batch of tuples */
MemoryContext temp_cxt; /* context for per-tuple temporary data */
@@ -1436,7 +1433,8 @@ postgresGetForeignPlan(PlannerInfo *root,
fdw_private,
fdw_scan_tlist,
fdw_recheck_quals,
- outer_plan);
+ outer_plan,
+ fpinfo->async_capable);
}
/*
@@ -1591,9 +1589,6 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
&fsstate->param_flinfo,
&fsstate->param_exprs,
&fsstate->param_values);
-
- /* Set the async-capable flag */
- fsstate->async_capable = node->ss.ps.async_capable;
}
/*
@@ -1622,7 +1617,7 @@ postgresIterateForeignScan(ForeignScanState *node)
if (fsstate->next_tuple >= fsstate->num_tuples)
{
/* In async mode, just clear tuple slot. */
- if (fsstate->async_capable)
+ if (node->ss.ps.async_capable)
return ExecClearTuple(slot);
/* No point in another fetch if we already detected EOF, though. */
if (!fsstate->eof_reached)
@@ -3781,7 +3776,7 @@ fetch_more_data(ForeignScanState *node)
int numrows;
int i;
- if (fsstate->async_capable)
+ if (node->ss.ps.async_capable)
{
Assert(fsstate->conn_state->pendingAreq);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 286dd99573..1c1c152bcf 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3252,6 +3252,35 @@ EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
SELECT * FROM local_tbl, async_pt WHERE local_tbl.a = async_pt.a AND local_tbl.c = 'bar';
SELECT * FROM local_tbl, async_pt WHERE local_tbl.a = async_pt.a AND local_tbl.c = 'bar';
+-- Subquery flattening must be done before choosing of async plans.
+EXPLAIN (VERBOSE, COSTS OFF)
+(SELECT * FROM async_p1 LIMIT 1)
+ UNION ALL
+(SELECT * FROM async_p2 WHERE a < 5)
+ UNION ALL
+(SELECT * FROM async_p2)
+ UNION ALL
+(SELECT * FROM async_p3 LIMIT 3);
+
+-- Check that async append doesn't break the scrollable cursors logic:
+-- If the query plan doesn't support backward scan, a materialize node will be
+-- inserted in the head of this plan.
+BEGIN;
+EXPLAIN (COSTS OFF)
+DECLARE curs1 SCROLL CURSOR FOR (SELECT * FROM async_p3);
+EXPLAIN (COSTS OFF)
+DECLARE curs1 SCROLL CURSOR FOR (SELECT * FROM async_pt);
+EXPLAIN (COSTS OFF)
+DECLARE curs1 NO SCROLL CURSOR FOR (SELECT * FROM async_p1);
+EXPLAIN (VERBOSE, COSTS OFF)
+DECLARE curs2 SCROLL CURSOR FOR
+ (SELECT * FROM async_p1)
+ UNION ALL
+ (SELECT * FROM async_p2 WHERE a < 5)
+ UNION ALL
+ (SELECT * FROM async_p3);
+ROLLBACK;
+
ALTER FOREIGN TABLE async_p1 OPTIONS (DROP use_remote_estimate);
ALTER FOREIGN TABLE async_p2 OPTIONS (DROP use_remote_estimate);
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index e81b990092..6c7f8e9d9f 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1411,7 +1411,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
}
if (plan->parallel_aware)
appendStringInfoString(es->str, "Parallel ");
- if (plan->async_capable)
+ if (planstate->async_capable)
appendStringInfoString(es->str, "Async ");
appendStringInfoString(es->str, pname);
es->indent++;
diff --git a/src/backend/executor/execAmi.c b/src/backend/executor/execAmi.c
index 10f0b349b5..ddeb028cf1 100644
--- a/src/backend/executor/execAmi.c
+++ b/src/backend/executor/execAmi.c
@@ -537,10 +537,6 @@ ExecSupportsBackwardScan(Plan *node)
{
ListCell *l;
- /* With async, tuples may be interleaved, so can't back up. */
- if (((Append *) node)->nasyncplans > 0)
- return false;
-
foreach(l, ((Append *) node)->appendplans)
{
if (!ExecSupportsBackwardScan((Plan *) lfirst(l)))
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index 755c1392f0..0f2148e097 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -83,6 +83,8 @@ struct ParallelAppendState
#define INVALID_SUBPLAN_INDEX -1
#define EVENT_BUFFER_SIZE 16
+bool enable_async_append = true;
+
static TupleTableSlot *ExecAppend(PlanState *pstate);
static bool choose_next_subplan_locally(AppendState *node);
static bool choose_next_subplan_for_leader(AppendState *node);
@@ -117,6 +119,7 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
int firstvalid;
int i,
j;
+ bool consider_async;
/* check for unsupported flags */
Assert(!(eflags & EXEC_FLAG_MARK));
@@ -197,6 +200,8 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
appendplanstates = (PlanState **) palloc(nplans *
sizeof(PlanState *));
+ consider_async = (enable_async_append && !node->plan.parallel_safe &&
+ bms_num_members(validsubplans) > 1);
/*
* call ExecInitNode on each of the valid plans to be executed and save
* the results into the appendplanstates array.
@@ -212,24 +217,28 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
{
Plan *initNode = (Plan *) list_nth(node->appendplans, i);
+ /*
+ * Record the lowest appendplans index which is a valid partial plan.
+ */
+ if (i >= node->first_partial_plan && j < firstvalid)
+ firstvalid = j;
+
+ appendplanstates[j] = ExecInitNode(initNode, estate, eflags);
+
/*
* Record async subplans. When executing EvalPlanQual, we treat them
* as sync ones; don't do this when initializing an EvalPlanQual plan
* tree.
*/
- if (initNode->async_capable && estate->es_epq_active == NULL)
+ if (consider_async && initNode->async_capable &&
+ estate->es_epq_active == NULL)
{
asyncplans = bms_add_member(asyncplans, j);
nasyncplans++;
+ appendplanstates[j++]->async_capable = true;
}
-
- /*
- * Record the lowest appendplans index which is a valid partial plan.
- */
- if (i >= node->first_partial_plan && j < firstvalid)
- firstvalid = j;
-
- appendplanstates[j++] = ExecInitNode(initNode, estate, eflags);
+ else
+ appendplanstates[j++]->async_capable = false;
}
appendstate->as_first_partial_plan = firstvalid;
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 9dc38d47ea..898890fb08 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -209,13 +209,6 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
scanstate->fdw_recheck_quals =
ExecInitQual(node->fdw_recheck_quals, (PlanState *) scanstate);
- /*
- * Determine whether to scan the foreign relation asynchronously or not;
- * this has to be kept in sync with the code in ExecInitAppend().
- */
- scanstate->ss.ps.async_capable = (((Plan *) node)->async_capable &&
- estate->es_epq_active == NULL);
-
/*
* Initialize FDW-related state.
*/
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index bd87f23784..aca4a7cce4 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -243,7 +243,6 @@ _copyAppend(const Append *from)
*/
COPY_BITMAPSET_FIELD(apprelids);
COPY_NODE_FIELD(appendplans);
- COPY_SCALAR_FIELD(nasyncplans);
COPY_SCALAR_FIELD(first_partial_plan);
COPY_NODE_FIELD(part_prune_info);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index e32b92e299..8e72c1333f 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -433,7 +433,6 @@ _outAppend(StringInfo str, const Append *node)
WRITE_BITMAPSET_FIELD(apprelids);
WRITE_NODE_FIELD(appendplans);
- WRITE_INT_FIELD(nasyncplans);
WRITE_INT_FIELD(first_partial_plan);
WRITE_NODE_FIELD(part_prune_info);
}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index f0b34ecfac..a2aafcd2ce 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1717,7 +1717,6 @@ _readAppend(void)
READ_BITMAPSET_FIELD(apprelids);
READ_NODE_FIELD(appendplans);
- READ_INT_FIELD(nasyncplans);
READ_INT_FIELD(first_partial_plan);
READ_NODE_FIELD(part_prune_info);
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 8577c7b138..21e0dd0049 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -149,7 +149,6 @@ bool enable_partitionwise_aggregate = false;
bool enable_parallel_append = true;
bool enable_parallel_hash = true;
bool enable_partition_pruning = true;
-bool enable_async_append = true;
typedef struct
{
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 439e6b6426..f2baa58269 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -82,7 +82,6 @@ static List *get_gating_quals(PlannerInfo *root, List *quals);
static Plan *create_gating_plan(PlannerInfo *root, Path *path, Plan *plan,
List *gating_quals);
static Plan *create_join_plan(PlannerInfo *root, JoinPath *best_path);
-static bool is_async_capable_path(Path *path);
static Plan *create_append_plan(PlannerInfo *root, AppendPath *best_path,
int flags);
static Plan *create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path,
@@ -1093,31 +1092,6 @@ create_join_plan(PlannerInfo *root, JoinPath *best_path)
return plan;
}
-/*
- * is_async_capable_path
- * Check whether a given Path node is async-capable.
- */
-static bool
-is_async_capable_path(Path *path)
-{
- switch (nodeTag(path))
- {
- case T_ForeignPath:
- {
- FdwRoutine *fdwroutine = path->parent->fdwroutine;
-
- Assert(fdwroutine != NULL);
- if (fdwroutine->IsForeignPathAsyncCapable != NULL &&
- fdwroutine->IsForeignPathAsyncCapable((ForeignPath *) path))
- return true;
- }
- break;
- default:
- break;
- }
- return false;
-}
-
/*
* create_append_plan
* Create an Append plan for 'best_path' and (recursively) plans
@@ -1135,7 +1109,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
List *pathkeys = best_path->path.pathkeys;
List *subplans = NIL;
ListCell *subpaths;
- int nasyncplans = 0;
RelOptInfo *rel = best_path->path.parent;
PartitionPruneInfo *partpruneinfo = NULL;
int nodenumsortkeys = 0;
@@ -1143,7 +1116,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
Oid *nodeSortOperators = NULL;
Oid *nodeCollations = NULL;
bool *nodeNullsFirst = NULL;
- bool consider_async = false;
/*
* The subpaths list could be empty, if every child was proven empty by
@@ -1207,11 +1179,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
tlist_was_changed = (orig_tlist_length != list_length(plan->plan.targetlist));
}
- /* If appropriate, consider async append */
- consider_async = (enable_async_append && pathkeys == NIL &&
- !best_path->path.parallel_safe &&
- list_length(best_path->subpaths) > 1);
-
/* Build the plan for each child */
foreach(subpaths, best_path->subpaths)
{
@@ -1279,13 +1246,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
}
subplans = lappend(subplans, subplan);
-
- /* Check to see if subplan can be executed asynchronously */
- if (consider_async && is_async_capable_path(subpath))
- {
- subplan->async_capable = true;
- ++nasyncplans;
- }
}
/*
@@ -1318,7 +1278,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
}
plan->appendplans = subplans;
- plan->nasyncplans = nasyncplans;
plan->first_partial_plan = best_path->first_partial_path;
plan->part_prune_info = partpruneinfo;
@@ -5685,7 +5644,8 @@ make_foreignscan(List *qptlist,
List *fdw_private,
List *fdw_scan_tlist,
List *fdw_recheck_quals,
- Plan *outer_plan)
+ Plan *outer_plan,
+ bool async_capable)
{
ForeignScan *node = makeNode(ForeignScan);
Plan *plan = &node->scan.plan;
@@ -5695,6 +5655,7 @@ make_foreignscan(List *qptlist,
plan->qual = qpqual;
plan->lefttree = outer_plan;
plan->righttree = NULL;
+ plan->async_capable = async_capable; /* set support of async opts */
node->scan.scanrelid = scanrelid;
/* these may be overridden by the FDW's PlanDirectModify callback. */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 480e8cd199..b14fe74050 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -51,6 +51,7 @@
#include "commands/vacuum.h"
#include "commands/variable.h"
#include "common/string.h"
+#include "executor/nodeAppend.h"
#include "funcapi.h"
#include "jit/jit.h"
#include "libpq/auth.h"
diff --git a/src/include/executor/nodeAppend.h b/src/include/executor/nodeAppend.h
index fa54ac6ad2..1831d6e021 100644
--- a/src/include/executor/nodeAppend.h
+++ b/src/include/executor/nodeAppend.h
@@ -17,6 +17,8 @@
#include "access/parallel.h"
#include "nodes/execnodes.h"
+extern PGDLLIMPORT bool enable_async_append;
+
extern AppendState *ExecInitAppend(Append *node, EState *estate, int eflags);
extern void ExecEndAppend(AppendState *node);
extern void ExecReScanAppend(AppendState *node);
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index aaa3b65d04..4d7595a7b1 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -251,7 +251,6 @@ typedef struct Append
Plan plan;
Bitmapset *apprelids; /* RTIs of appendrel(s) formed by this node */
List *appendplans;
- int nasyncplans; /* # of asynchronous plans */
/*
* All 'appendplans' preceding this index are non-partial plans. All
diff --git a/src/include/optimizer/cost.h b/src/include/optimizer/cost.h
index 0fe60d82e4..67f925e793 100644
--- a/src/include/optimizer/cost.h
+++ b/src/include/optimizer/cost.h
@@ -66,7 +66,6 @@ extern PGDLLIMPORT bool enable_partitionwise_aggregate;
extern PGDLLIMPORT bool enable_parallel_append;
extern PGDLLIMPORT bool enable_parallel_hash;
extern PGDLLIMPORT bool enable_partition_pruning;
-extern PGDLLIMPORT bool enable_async_append;
extern PGDLLIMPORT int constraint_exclusion;
extern double index_pages_fetched(double tuples_fetched, BlockNumber pages,
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index bf1adfc52a..710503e501 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -42,7 +42,7 @@ extern Plan *create_plan(PlannerInfo *root, Path *best_path);
extern ForeignScan *make_foreignscan(List *qptlist, List *qpqual,
Index scanrelid, List *fdw_exprs, List *fdw_private,
List *fdw_scan_tlist, List *fdw_recheck_quals,
- Plan *outer_plan);
+ Plan *outer_plan, bool async_capable);
extern Plan *change_plan_targetlist(Plan *subplan, List *tlist,
bool tlist_parallel_safe);
extern Plan *materialize_finished_plan(Plan *subplan);
--
2.31.1
Attachments:
[text/plain] 0001-Choose-async-append-subplans-at-the-initial-executio.patch (20.7K, ../../edb1331c-e861-0c53-9fdb-f7ca7dfd884d@postgrespro.ru/2-0001-Choose-async-append-subplans-at-the-initial-executio.patch)
download | inline diff:
From d935bbb70565d70f1b0f547bf37e71ffc6fa2ef2 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Tue, 29 Jun 2021 22:09:54 +0300
Subject: [PATCH] Choose async append subplans at the initial execution stage
---
contrib/file_fdw/file_fdw.c | 3 +-
.../postgres_fdw/expected/postgres_fdw.out | 81 ++++++++++++++++++-
contrib/postgres_fdw/postgres_fdw.c | 13 +--
contrib/postgres_fdw/sql/postgres_fdw.sql | 29 +++++++
src/backend/commands/explain.c | 2 +-
src/backend/executor/execAmi.c | 4 -
src/backend/executor/nodeAppend.c | 27 ++++---
src/backend/executor/nodeForeignscan.c | 7 --
src/backend/nodes/copyfuncs.c | 1 -
src/backend/nodes/outfuncs.c | 1 -
src/backend/nodes/readfuncs.c | 1 -
src/backend/optimizer/path/costsize.c | 1 -
src/backend/optimizer/plan/createplan.c | 45 +----------
src/backend/utils/misc/guc.c | 1 +
src/include/executor/nodeAppend.h | 2 +
src/include/nodes/plannodes.h | 1 -
src/include/optimizer/cost.h | 1 -
src/include/optimizer/planmain.h | 2 +-
18 files changed, 141 insertions(+), 81 deletions(-)
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 2c2f149fb0..5f67e1ca94 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -609,7 +609,8 @@ fileGetForeignPlan(PlannerInfo *root,
best_path->fdw_private,
NIL, /* no custom tlist */
NIL, /* no remote quals */
- outer_plan);
+ outer_plan,
+ false);
}
/*
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 31b5de91ad..30c38c6992 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10169,7 +10169,7 @@ EXECUTE async_pt_query (2000, 505);
Insert on public.result_tbl
-> Append
Subplans Removed: 2
- -> Async Foreign Scan on public.async_p1 async_pt_1
+ -> Foreign Scan on public.async_p1 async_pt_1
Output: async_pt_1.a, async_pt_1.b, async_pt_1.c
Filter: (async_pt_1.b === $2)
Remote SQL: SELECT a, b, c FROM public.base_tbl1 WHERE ((a < $1::integer))
@@ -10237,6 +10237,85 @@ SELECT * FROM local_tbl, async_pt WHERE local_tbl.a = async_pt.a AND local_tbl.c
2505 | 505 | bar | 2505 | 505 | 0505
(1 row)
+-- Subquery flattening must be done before choosing of async plans.
+EXPLAIN (VERBOSE, COSTS OFF)
+(SELECT * FROM async_p1 LIMIT 1)
+ UNION ALL
+(SELECT * FROM async_p2 WHERE a < 5)
+ UNION ALL
+(SELECT * FROM async_p2)
+ UNION ALL
+(SELECT * FROM async_p3 LIMIT 3);
+ QUERY PLAN
+--------------------------------------------------------------------------
+ Append
+ -> Async Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1 LIMIT 1::bigint
+ -> Async Foreign Scan on public.async_p2 async_p2_1
+ Output: async_p2_1.a, async_p2_1.b, async_p2_1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((a < 5))
+ -> Async Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2
+ -> Limit
+ Output: async_p3.a, async_p3.b, async_p3.c
+ -> Seq Scan on public.async_p3
+ Output: async_p3.a, async_p3.b, async_p3.c
+(14 rows)
+
+-- Check that async append doesn't break the scrollable cursors logic:
+-- If the query plan doesn't support backward scan, a materialize node will be
+-- inserted in the head of this plan.
+BEGIN;
+EXPLAIN (COSTS OFF)
+DECLARE curs1 SCROLL CURSOR FOR (SELECT * FROM async_p3);
+ QUERY PLAN
+----------------------
+ Seq Scan on async_p3
+(1 row)
+
+EXPLAIN (COSTS OFF)
+DECLARE curs1 SCROLL CURSOR FOR (SELECT * FROM async_pt);
+ QUERY PLAN
+-------------------------------------------------------
+ Materialize
+ -> Append
+ -> Async Foreign Scan on async_p1 async_pt_1
+ -> Async Foreign Scan on async_p2 async_pt_2
+ -> Seq Scan on async_p3 async_pt_3
+(5 rows)
+
+EXPLAIN (COSTS OFF)
+DECLARE curs1 NO SCROLL CURSOR FOR (SELECT * FROM async_p1);
+ QUERY PLAN
+--------------------------
+ Foreign Scan on async_p1
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF)
+DECLARE curs2 SCROLL CURSOR FOR
+ (SELECT * FROM async_p1)
+ UNION ALL
+ (SELECT * FROM async_p2 WHERE a < 5)
+ UNION ALL
+ (SELECT * FROM async_p3);
+ QUERY PLAN
+--------------------------------------------------------------------------------
+ Materialize
+ Output: async_p1.a, async_p1.b, async_p1.c
+ -> Append
+ -> Async Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1
+ -> Async Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((a < 5))
+ -> Seq Scan on public.async_p3
+ Output: async_p3.a, async_p3.b, async_p3.c
+(11 rows)
+
+ROLLBACK;
ALTER FOREIGN TABLE async_p1 OPTIONS (DROP use_remote_estimate);
ALTER FOREIGN TABLE async_p2 OPTIONS (DROP use_remote_estimate);
DROP TABLE local_tbl;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index fafbab6b02..6a52859f5e 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -163,9 +163,6 @@ typedef struct PgFdwScanState
int fetch_ct_2; /* Min(# of fetches done, 2) */
bool eof_reached; /* true if last fetch reached EOF */
- /* for asynchronous execution */
- bool async_capable; /* engage asynchronous-capable logic? */
-
/* working memory contexts */
MemoryContext batch_cxt; /* context holding current batch of tuples */
MemoryContext temp_cxt; /* context for per-tuple temporary data */
@@ -1436,7 +1433,8 @@ postgresGetForeignPlan(PlannerInfo *root,
fdw_private,
fdw_scan_tlist,
fdw_recheck_quals,
- outer_plan);
+ outer_plan,
+ fpinfo->async_capable);
}
/*
@@ -1591,9 +1589,6 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
&fsstate->param_flinfo,
&fsstate->param_exprs,
&fsstate->param_values);
-
- /* Set the async-capable flag */
- fsstate->async_capable = node->ss.ps.async_capable;
}
/*
@@ -1622,7 +1617,7 @@ postgresIterateForeignScan(ForeignScanState *node)
if (fsstate->next_tuple >= fsstate->num_tuples)
{
/* In async mode, just clear tuple slot. */
- if (fsstate->async_capable)
+ if (node->ss.ps.async_capable)
return ExecClearTuple(slot);
/* No point in another fetch if we already detected EOF, though. */
if (!fsstate->eof_reached)
@@ -3781,7 +3776,7 @@ fetch_more_data(ForeignScanState *node)
int numrows;
int i;
- if (fsstate->async_capable)
+ if (node->ss.ps.async_capable)
{
Assert(fsstate->conn_state->pendingAreq);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 286dd99573..1c1c152bcf 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3252,6 +3252,35 @@ EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
SELECT * FROM local_tbl, async_pt WHERE local_tbl.a = async_pt.a AND local_tbl.c = 'bar';
SELECT * FROM local_tbl, async_pt WHERE local_tbl.a = async_pt.a AND local_tbl.c = 'bar';
+-- Subquery flattening must be done before choosing of async plans.
+EXPLAIN (VERBOSE, COSTS OFF)
+(SELECT * FROM async_p1 LIMIT 1)
+ UNION ALL
+(SELECT * FROM async_p2 WHERE a < 5)
+ UNION ALL
+(SELECT * FROM async_p2)
+ UNION ALL
+(SELECT * FROM async_p3 LIMIT 3);
+
+-- Check that async append doesn't break the scrollable cursors logic:
+-- If the query plan doesn't support backward scan, a materialize node will be
+-- inserted in the head of this plan.
+BEGIN;
+EXPLAIN (COSTS OFF)
+DECLARE curs1 SCROLL CURSOR FOR (SELECT * FROM async_p3);
+EXPLAIN (COSTS OFF)
+DECLARE curs1 SCROLL CURSOR FOR (SELECT * FROM async_pt);
+EXPLAIN (COSTS OFF)
+DECLARE curs1 NO SCROLL CURSOR FOR (SELECT * FROM async_p1);
+EXPLAIN (VERBOSE, COSTS OFF)
+DECLARE curs2 SCROLL CURSOR FOR
+ (SELECT * FROM async_p1)
+ UNION ALL
+ (SELECT * FROM async_p2 WHERE a < 5)
+ UNION ALL
+ (SELECT * FROM async_p3);
+ROLLBACK;
+
ALTER FOREIGN TABLE async_p1 OPTIONS (DROP use_remote_estimate);
ALTER FOREIGN TABLE async_p2 OPTIONS (DROP use_remote_estimate);
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index e81b990092..6c7f8e9d9f 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1411,7 +1411,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
}
if (plan->parallel_aware)
appendStringInfoString(es->str, "Parallel ");
- if (plan->async_capable)
+ if (planstate->async_capable)
appendStringInfoString(es->str, "Async ");
appendStringInfoString(es->str, pname);
es->indent++;
diff --git a/src/backend/executor/execAmi.c b/src/backend/executor/execAmi.c
index 10f0b349b5..ddeb028cf1 100644
--- a/src/backend/executor/execAmi.c
+++ b/src/backend/executor/execAmi.c
@@ -537,10 +537,6 @@ ExecSupportsBackwardScan(Plan *node)
{
ListCell *l;
- /* With async, tuples may be interleaved, so can't back up. */
- if (((Append *) node)->nasyncplans > 0)
- return false;
-
foreach(l, ((Append *) node)->appendplans)
{
if (!ExecSupportsBackwardScan((Plan *) lfirst(l)))
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index 755c1392f0..0f2148e097 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -83,6 +83,8 @@ struct ParallelAppendState
#define INVALID_SUBPLAN_INDEX -1
#define EVENT_BUFFER_SIZE 16
+bool enable_async_append = true;
+
static TupleTableSlot *ExecAppend(PlanState *pstate);
static bool choose_next_subplan_locally(AppendState *node);
static bool choose_next_subplan_for_leader(AppendState *node);
@@ -117,6 +119,7 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
int firstvalid;
int i,
j;
+ bool consider_async;
/* check for unsupported flags */
Assert(!(eflags & EXEC_FLAG_MARK));
@@ -197,6 +200,8 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
appendplanstates = (PlanState **) palloc(nplans *
sizeof(PlanState *));
+ consider_async = (enable_async_append && !node->plan.parallel_safe &&
+ bms_num_members(validsubplans) > 1);
/*
* call ExecInitNode on each of the valid plans to be executed and save
* the results into the appendplanstates array.
@@ -212,24 +217,28 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
{
Plan *initNode = (Plan *) list_nth(node->appendplans, i);
+ /*
+ * Record the lowest appendplans index which is a valid partial plan.
+ */
+ if (i >= node->first_partial_plan && j < firstvalid)
+ firstvalid = j;
+
+ appendplanstates[j] = ExecInitNode(initNode, estate, eflags);
+
/*
* Record async subplans. When executing EvalPlanQual, we treat them
* as sync ones; don't do this when initializing an EvalPlanQual plan
* tree.
*/
- if (initNode->async_capable && estate->es_epq_active == NULL)
+ if (consider_async && initNode->async_capable &&
+ estate->es_epq_active == NULL)
{
asyncplans = bms_add_member(asyncplans, j);
nasyncplans++;
+ appendplanstates[j++]->async_capable = true;
}
-
- /*
- * Record the lowest appendplans index which is a valid partial plan.
- */
- if (i >= node->first_partial_plan && j < firstvalid)
- firstvalid = j;
-
- appendplanstates[j++] = ExecInitNode(initNode, estate, eflags);
+ else
+ appendplanstates[j++]->async_capable = false;
}
appendstate->as_first_partial_plan = firstvalid;
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 9dc38d47ea..898890fb08 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -209,13 +209,6 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
scanstate->fdw_recheck_quals =
ExecInitQual(node->fdw_recheck_quals, (PlanState *) scanstate);
- /*
- * Determine whether to scan the foreign relation asynchronously or not;
- * this has to be kept in sync with the code in ExecInitAppend().
- */
- scanstate->ss.ps.async_capable = (((Plan *) node)->async_capable &&
- estate->es_epq_active == NULL);
-
/*
* Initialize FDW-related state.
*/
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index bd87f23784..aca4a7cce4 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -243,7 +243,6 @@ _copyAppend(const Append *from)
*/
COPY_BITMAPSET_FIELD(apprelids);
COPY_NODE_FIELD(appendplans);
- COPY_SCALAR_FIELD(nasyncplans);
COPY_SCALAR_FIELD(first_partial_plan);
COPY_NODE_FIELD(part_prune_info);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index e32b92e299..8e72c1333f 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -433,7 +433,6 @@ _outAppend(StringInfo str, const Append *node)
WRITE_BITMAPSET_FIELD(apprelids);
WRITE_NODE_FIELD(appendplans);
- WRITE_INT_FIELD(nasyncplans);
WRITE_INT_FIELD(first_partial_plan);
WRITE_NODE_FIELD(part_prune_info);
}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index f0b34ecfac..a2aafcd2ce 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1717,7 +1717,6 @@ _readAppend(void)
READ_BITMAPSET_FIELD(apprelids);
READ_NODE_FIELD(appendplans);
- READ_INT_FIELD(nasyncplans);
READ_INT_FIELD(first_partial_plan);
READ_NODE_FIELD(part_prune_info);
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 8577c7b138..21e0dd0049 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -149,7 +149,6 @@ bool enable_partitionwise_aggregate = false;
bool enable_parallel_append = true;
bool enable_parallel_hash = true;
bool enable_partition_pruning = true;
-bool enable_async_append = true;
typedef struct
{
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 439e6b6426..f2baa58269 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -82,7 +82,6 @@ static List *get_gating_quals(PlannerInfo *root, List *quals);
static Plan *create_gating_plan(PlannerInfo *root, Path *path, Plan *plan,
List *gating_quals);
static Plan *create_join_plan(PlannerInfo *root, JoinPath *best_path);
-static bool is_async_capable_path(Path *path);
static Plan *create_append_plan(PlannerInfo *root, AppendPath *best_path,
int flags);
static Plan *create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path,
@@ -1093,31 +1092,6 @@ create_join_plan(PlannerInfo *root, JoinPath *best_path)
return plan;
}
-/*
- * is_async_capable_path
- * Check whether a given Path node is async-capable.
- */
-static bool
-is_async_capable_path(Path *path)
-{
- switch (nodeTag(path))
- {
- case T_ForeignPath:
- {
- FdwRoutine *fdwroutine = path->parent->fdwroutine;
-
- Assert(fdwroutine != NULL);
- if (fdwroutine->IsForeignPathAsyncCapable != NULL &&
- fdwroutine->IsForeignPathAsyncCapable((ForeignPath *) path))
- return true;
- }
- break;
- default:
- break;
- }
- return false;
-}
-
/*
* create_append_plan
* Create an Append plan for 'best_path' and (recursively) plans
@@ -1135,7 +1109,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
List *pathkeys = best_path->path.pathkeys;
List *subplans = NIL;
ListCell *subpaths;
- int nasyncplans = 0;
RelOptInfo *rel = best_path->path.parent;
PartitionPruneInfo *partpruneinfo = NULL;
int nodenumsortkeys = 0;
@@ -1143,7 +1116,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
Oid *nodeSortOperators = NULL;
Oid *nodeCollations = NULL;
bool *nodeNullsFirst = NULL;
- bool consider_async = false;
/*
* The subpaths list could be empty, if every child was proven empty by
@@ -1207,11 +1179,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
tlist_was_changed = (orig_tlist_length != list_length(plan->plan.targetlist));
}
- /* If appropriate, consider async append */
- consider_async = (enable_async_append && pathkeys == NIL &&
- !best_path->path.parallel_safe &&
- list_length(best_path->subpaths) > 1);
-
/* Build the plan for each child */
foreach(subpaths, best_path->subpaths)
{
@@ -1279,13 +1246,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
}
subplans = lappend(subplans, subplan);
-
- /* Check to see if subplan can be executed asynchronously */
- if (consider_async && is_async_capable_path(subpath))
- {
- subplan->async_capable = true;
- ++nasyncplans;
- }
}
/*
@@ -1318,7 +1278,6 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
}
plan->appendplans = subplans;
- plan->nasyncplans = nasyncplans;
plan->first_partial_plan = best_path->first_partial_path;
plan->part_prune_info = partpruneinfo;
@@ -5685,7 +5644,8 @@ make_foreignscan(List *qptlist,
List *fdw_private,
List *fdw_scan_tlist,
List *fdw_recheck_quals,
- Plan *outer_plan)
+ Plan *outer_plan,
+ bool async_capable)
{
ForeignScan *node = makeNode(ForeignScan);
Plan *plan = &node->scan.plan;
@@ -5695,6 +5655,7 @@ make_foreignscan(List *qptlist,
plan->qual = qpqual;
plan->lefttree = outer_plan;
plan->righttree = NULL;
+ plan->async_capable = async_capable; /* set support of async opts */
node->scan.scanrelid = scanrelid;
/* these may be overridden by the FDW's PlanDirectModify callback. */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 480e8cd199..b14fe74050 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -51,6 +51,7 @@
#include "commands/vacuum.h"
#include "commands/variable.h"
#include "common/string.h"
+#include "executor/nodeAppend.h"
#include "funcapi.h"
#include "jit/jit.h"
#include "libpq/auth.h"
diff --git a/src/include/executor/nodeAppend.h b/src/include/executor/nodeAppend.h
index fa54ac6ad2..1831d6e021 100644
--- a/src/include/executor/nodeAppend.h
+++ b/src/include/executor/nodeAppend.h
@@ -17,6 +17,8 @@
#include "access/parallel.h"
#include "nodes/execnodes.h"
+extern PGDLLIMPORT bool enable_async_append;
+
extern AppendState *ExecInitAppend(Append *node, EState *estate, int eflags);
extern void ExecEndAppend(AppendState *node);
extern void ExecReScanAppend(AppendState *node);
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index aaa3b65d04..4d7595a7b1 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -251,7 +251,6 @@ typedef struct Append
Plan plan;
Bitmapset *apprelids; /* RTIs of appendrel(s) formed by this node */
List *appendplans;
- int nasyncplans; /* # of asynchronous plans */
/*
* All 'appendplans' preceding this index are non-partial plans. All
diff --git a/src/include/optimizer/cost.h b/src/include/optimizer/cost.h
index 0fe60d82e4..67f925e793 100644
--- a/src/include/optimizer/cost.h
+++ b/src/include/optimizer/cost.h
@@ -66,7 +66,6 @@ extern PGDLLIMPORT bool enable_partitionwise_aggregate;
extern PGDLLIMPORT bool enable_parallel_append;
extern PGDLLIMPORT bool enable_parallel_hash;
extern PGDLLIMPORT bool enable_partition_pruning;
-extern PGDLLIMPORT bool enable_async_append;
extern PGDLLIMPORT int constraint_exclusion;
extern double index_pages_fetched(double tuples_fetched, BlockNumber pages,
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index bf1adfc52a..710503e501 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -42,7 +42,7 @@ extern Plan *create_plan(PlannerInfo *root, Path *best_path);
extern ForeignScan *make_foreignscan(List *qptlist, List *qpqual,
Index scanrelid, List *fdw_exprs, List *fdw_private,
List *fdw_scan_tlist, List *fdw_recheck_quals,
- Plan *outer_plan);
+ Plan *outer_plan, bool async_capable);
extern Plan *change_plan_targetlist(Plan *subplan, List *tlist,
bool tlist_parallel_safe);
extern Plan *materialize_finished_plan(Plan *subplan);
--
2.31.1
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2021-08-23 09:18 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 33+ messages in thread
From: Etsuro Fujita @ 2021-08-23 09:18 UTC (permalink / raw)
To: Andrey Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: Zhihong Yu <zyu@yugabyte.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Jun 30, 2021 at 1:50 PM Andrey Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> I have completely rewritten this patch.
>
> Main idea:
>
> The async_capable field of a plan node inform us that this node could
> work in async mode. Each node sets this field based on its own logic.
> The actual mode of a node is defined by the async_capable of PlanState
> structure. It is made at the executor initialization stage.
> In this patch, only an append node could define async behaviour for its
> subplans.
I finally reviewed the patch. One thing I noticed about the patch is
that it would break ordered Appends. Here is such an example using
the patch:
create table pt (a int) partition by range (a);
create table loct1 (a int);
create table loct2 (a int);
create foreign table p1 partition of pt for values from (10) to (20)
server loopback1 options (table_name 'loct1');
create foreign table p2 partition of pt for values from (20) to (30)
server loopback2 options (table_name 'loct2');
explain verbose select * from pt order by a;
QUERY PLAN
-------------------------------------------------------------------------------------
Append (cost=200.00..440.45 rows=5850 width=4)
-> Async Foreign Scan on public.p1 pt_1 (cost=100.00..205.60
rows=2925 width=4)
Output: pt_1.a
Remote SQL: SELECT a FROM public.loct1 ORDER BY a ASC NULLS LAST
-> Async Foreign Scan on public.p2 pt_2 (cost=100.00..205.60
rows=2925 width=4)
Output: pt_2.a
Remote SQL: SELECT a FROM public.loct2 ORDER BY a ASC NULLS LAST
(7 rows)
This would not always provide tuples in the required order, as async
execution would provide them from the subplans rather randomly. I
think it would not only be too late but be not efficient to do the
planning work at execution time (consider executing generic plans!),
so I think we should avoid doing so. (The cost of doing that work for
simple foreign scans is small, but if we support async execution for
upper plan nodes such as NestLoop as discussed before, I think the
cost for such plan nodes would not be small anymore.)
To just execute what was planned at execution time, I think we should
return to the patch in [1]. The patch was created for Horiguchi-san’s
async-execution patch, so I modified it to work with HEAD, and added a
simplified version of your test cases. Please find attached a patch.
Best regards,
Etsuro Fujita
[1] https://www.postgresql.org/message-id/7fe10f95-ac6c-c81d-a9d3-227493eb9055@postgrespro.ru
Attachments:
[application/octet-stream] allow-async-in-more-cases.patch (11.0K, ../../CAPmGK15AshMEtgUqs3yZfD5mzW87wyn=_uAXbBeRLG0nod7vHA@mail.gmail.com/2-allow-async-in-more-cases.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index e3ee30f1aa..402882f0d2 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10456,6 +10456,88 @@ DROP TABLE local_tbl;
DROP INDEX base_tbl1_idx;
DROP INDEX base_tbl2_idx;
DROP INDEX async_p3_idx;
+-- UNION queries
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT * FROM async_p2 WHERE b < 10);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------
+ Insert on public.result_tbl
+ -> HashAggregate
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Group Key: async_p1.a, async_p1.b, async_p1.c
+ -> Append
+ -> Async Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1 ORDER BY a ASC NULLS LAST LIMIT 10::bigint
+ -> Async Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((b < 10))
+(11 rows)
+
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT * FROM async_p2 WHERE b < 10);
+SELECT * FROM result_tbl ORDER BY a;
+ a | b | c
+------+----+------
+ 1000 | 0 | 0000
+ 1005 | 5 | 0005
+ 1010 | 10 | 0010
+ 1015 | 15 | 0015
+ 1020 | 20 | 0020
+ 1025 | 25 | 0025
+ 1030 | 30 | 0030
+ 1035 | 35 | 0035
+ 1040 | 40 | 0040
+ 1045 | 45 | 0045
+ 2000 | 0 | 0000
+ 2005 | 5 | 0005
+(12 rows)
+
+DELETE FROM result_tbl;
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT * FROM async_p2 WHERE b < 10);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ Insert on public.result_tbl
+ -> Append
+ -> Async Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1 ORDER BY a ASC NULLS LAST LIMIT 10::bigint
+ -> Async Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((b < 10))
+(8 rows)
+
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT * FROM async_p2 WHERE b < 10);
+SELECT * FROM result_tbl ORDER BY a;
+ a | b | c
+------+----+------
+ 1000 | 0 | 0000
+ 1005 | 5 | 0005
+ 1010 | 10 | 0010
+ 1015 | 15 | 0015
+ 1020 | 20 | 0020
+ 1025 | 25 | 0025
+ 1030 | 30 | 0030
+ 1035 | 35 | 0035
+ 1040 | 40 | 0040
+ 1045 | 45 | 0045
+ 2000 | 0 | 0000
+ 2005 | 5 | 0005
+(12 rows)
+
+DELETE FROM result_tbl;
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 30b5175da5..22848710c5 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3322,6 +3322,33 @@ DROP INDEX base_tbl1_idx;
DROP INDEX base_tbl2_idx;
DROP INDEX async_p3_idx;
+-- UNION queries
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT * FROM async_p2 WHERE b < 10);
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT * FROM async_p2 WHERE b < 10);
+
+SELECT * FROM result_tbl ORDER BY a;
+DELETE FROM result_tbl;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT * FROM async_p2 WHERE b < 10);
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT * FROM async_p2 WHERE b < 10);
+
+SELECT * FROM result_tbl ORDER BY a;
+DELETE FROM result_tbl;
+
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 38251c2b8e..5ef1888407 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -626,6 +626,7 @@ _copySubqueryScan(const SubqueryScan *from)
* copy remainder of node
*/
COPY_NODE_FIELD(subplan);
+ COPY_SCALAR_FIELD(status);
return newnode;
}
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 87561cbb6f..0d41a9ab4f 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -629,6 +629,7 @@ _outSubqueryScan(StringInfo str, const SubqueryScan *node)
_outScanInfo(str, (const Scan *) node);
WRITE_NODE_FIELD(subplan);
+ WRITE_ENUM_FIELD(status, SubqueryScanStatus);
}
static void
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 0dd1ad7dfc..ebee3b6acf 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1967,6 +1967,7 @@ _readSubqueryScan(void)
ReadCommonScan(&local_node->scan);
READ_NODE_FIELD(subplan);
+ READ_ENUM_FIELD(status, SubqueryScanStatus);
READ_DONE();
}
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index a5f6d678cc..9b6a8cfb74 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -82,7 +82,7 @@ static List *get_gating_quals(PlannerInfo *root, List *quals);
static Plan *create_gating_plan(PlannerInfo *root, Path *path, Plan *plan,
List *gating_quals);
static Plan *create_join_plan(PlannerInfo *root, JoinPath *best_path);
-static bool is_async_capable_path(Path *path);
+static bool mark_async_capable_plan(Plan *plan, Path *path);
static Plan *create_append_plan(PlannerInfo *root, AppendPath *best_path,
int flags);
static Plan *create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path,
@@ -1091,14 +1091,25 @@ create_join_plan(PlannerInfo *root, JoinPath *best_path)
}
/*
- * is_async_capable_path
- * Check whether a given Path node is async-capable.
+ * mark_async_capable_plan
+ * Check whether a given Path node is async-capable, and if so, mark the
+ * Plan node created from it as such.
*/
static bool
-is_async_capable_path(Path *path)
+mark_async_capable_plan(Plan *plan, Path *path)
{
switch (nodeTag(path))
{
+ case T_SubqueryScanPath:
+ {
+ SubqueryScan *splan = (SubqueryScan *) plan;
+
+ if (trivial_subqueryscan(splan) &&
+ mark_async_capable_plan(splan->subplan,
+ ((SubqueryScanPath *) path)->subpath))
+ break;
+ return false;
+ }
case T_ForeignPath:
{
FdwRoutine *fdwroutine = path->parent->fdwroutine;
@@ -1106,13 +1117,15 @@ is_async_capable_path(Path *path)
Assert(fdwroutine != NULL);
if (fdwroutine->IsForeignPathAsyncCapable != NULL &&
fdwroutine->IsForeignPathAsyncCapable((ForeignPath *) path))
- return true;
+ break;
+ return false;
}
- break;
default:
- break;
+ return false;
}
- return false;
+
+ plan->async_capable = true;
+ return true;
}
/*
@@ -1278,9 +1291,9 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
subplans = lappend(subplans, subplan);
/* Check to see if subplan can be executed asynchronously */
- if (consider_async && is_async_capable_path(subpath))
+ if (consider_async && mark_async_capable_plan(subplan, subpath))
{
- subplan->async_capable = true;
+ Assert(subplan->async_capable);
++nasyncplans;
}
}
@@ -5551,6 +5564,7 @@ make_subqueryscan(List *qptlist,
plan->righttree = NULL;
node->scan.scanrelid = scanrelid;
node->subplan = subplan;
+ node->status = SUBQUERY_SCAN_UNKNOWN;
return node;
}
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index e50624c465..a4455b8a44 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -115,7 +115,6 @@ static Plan *set_indexonlyscan_references(PlannerInfo *root,
static Plan *set_subqueryscan_references(PlannerInfo *root,
SubqueryScan *plan,
int rtoffset);
-static bool trivial_subqueryscan(SubqueryScan *plan);
static Plan *clean_up_removed_plan_level(Plan *parent, Plan *child);
static void set_foreignscan_references(PlannerInfo *root,
ForeignScan *fscan,
@@ -1206,13 +1205,22 @@ set_subqueryscan_references(PlannerInfo *root,
* We can delete it if it has no qual to check and the targetlist just
* regurgitates the output of the child plan.
*/
-static bool
+bool
trivial_subqueryscan(SubqueryScan *plan)
{
int attrno;
ListCell *lp,
*lc;
+ /* We might have detected this already */
+ if (plan->status == SUBQUERY_SCAN_TRIVIAL)
+ return true;
+ if (plan->status == SUBQUERY_SCAN_NONTRIVIAL)
+ return false;
+ Assert(plan->status == SUBQUERY_SCAN_UNKNOWN);
+
+ plan->status = SUBQUERY_SCAN_NONTRIVIAL;
+
if (plan->scan.plan.qual != NIL)
return false;
@@ -1254,6 +1262,7 @@ trivial_subqueryscan(SubqueryScan *plan)
attrno++;
}
+ plan->status = SUBQUERY_SCAN_TRIVIAL;
return true;
}
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index ec9a8b0c81..67f21080df 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -518,16 +518,28 @@ typedef struct TidRangeScan
* relation, we make this a descendant of Scan anyway for code-sharing
* purposes.
*
+ * SubqueryScanStatus caches the trivial_subqueryscan property of the node.
+ * SUBQUERY_SCAN_UNKNOWN means not yet determined. This is only used during
+ * planning.
+ *
* Note: we store the sub-plan in the type-specific subplan field, not in
* the generic lefttree field as you might expect. This is because we do
* not want plan-tree-traversal routines to recurse into the subplan without
* knowing that they are changing Query contexts.
* ----------------
*/
+typedef enum SubqueryScanStatus
+{
+ SUBQUERY_SCAN_UNKNOWN,
+ SUBQUERY_SCAN_TRIVIAL,
+ SUBQUERY_SCAN_NONTRIVIAL
+} SubqueryScanStatus;
+
typedef struct SubqueryScan
{
Scan scan;
Plan *subplan;
+ SubqueryScanStatus status;
} SubqueryScan;
/* ----------------
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index bf1adfc52a..c908a49490 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -112,6 +112,7 @@ extern bool innerrel_is_unique(PlannerInfo *root,
* prototypes for plan/setrefs.c
*/
extern Plan *set_plan_references(PlannerInfo *root, Plan *plan);
+extern bool trivial_subqueryscan(SubqueryScan *plan);
extern void record_plan_function_dependency(PlannerInfo *root, Oid funcid);
extern void record_plan_type_dependency(PlannerInfo *root, Oid typid);
extern bool extract_query_dependencies_walker(Node *node, PlannerInfo *root);
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2021-08-30 08:36 Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 33+ messages in thread
From: Andrey V. Lepikhov @ 2021-08-30 08:36 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Zhihong Yu <zyu@yugabyte.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 8/23/21 2:18 PM, Etsuro Fujita wrote:
> To just execute what was planned at execution time, I think we should
> return to the patch in [1]. The patch was created for Horiguchi-san’s
> async-execution patch, so I modified it to work with HEAD, and added a
> simplified version of your test cases. Please find attached a patch.
> [1] https://www.postgresql.org/message-id/7fe10f95-ac6c-c81d-a9d3-227493eb9055@postgrespro.ru
I agree, this way is more safe. I tried to search for another approach,
because here isn't general solution: for each plan node we should
implement support of asynchronous behaviour.
But for practical use, for small set of nodes, it will work good. I
haven't any objections for this patch.
--
regards,
Andrey Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2021-08-30 09:52 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 33+ messages in thread
From: Etsuro Fujita @ 2021-08-30 09:52 UTC (permalink / raw)
To: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: Zhihong Yu <zyu@yugabyte.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Aug 30, 2021 at 5:36 PM Andrey V. Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> On 8/23/21 2:18 PM, Etsuro Fujita wrote:
> > To just execute what was planned at execution time, I think we should
> > return to the patch in [1]. The patch was created for Horiguchi-san’s
> > async-execution patch, so I modified it to work with HEAD, and added a
> > simplified version of your test cases. Please find attached a patch.
> > [1] https://www.postgresql.org/message-id/7fe10f95-ac6c-c81d-a9d3-227493eb9055@postgrespro.ru
> I agree, this way is more safe. I tried to search for another approach,
> because here isn't general solution: for each plan node we should
> implement support of asynchronous behaviour.
I think so too.
> But for practical use, for small set of nodes, it will work good. I
> haven't any objections for this patch.
OK
To allow async execution in a bit more cases, I modified the patch a
bit further: a ProjectionPath put directly above an async-capable
ForeignPath would also be considered async-capable as ForeignScan can
project and no separate Result is needed in that case, so I modified
mark_async_capable_plan() as such, and added test cases to the
postgres_fdw regression test. Attached is an updated version of the
patch.
Thanks for the review!
Best regards,
Etsuro Fujita
Attachments:
[application/octet-stream] allow-async-in-more-cases-2.patch (13.1K, ../../CAPmGK14xPU5KaQZ_yjNbOmx-vehmbD-M90LpSXHG+KoEbmv9eQ@mail.gmail.com/2-allow-async-in-more-cases-2.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index e3ee30f1aa..3e7f64a6ed 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10141,6 +10141,31 @@ SELECT * FROM result_tbl ORDER BY a;
2505 | 505 | 0505
(2 rows)
+DELETE FROM result_tbl;
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl SELECT a, b + 1000, 'AAA' || c FROM async_pt WHERE b === 505;
+ QUERY PLAN
+------------------------------------------------------------------------------------------
+ Insert on public.result_tbl
+ -> Append
+ -> Async Foreign Scan on public.async_p1 async_pt_1
+ Output: async_pt_1.a, (async_pt_1.b + 1000), ('AAA'::text || async_pt_1.c)
+ Filter: (async_pt_1.b === 505)
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1
+ -> Async Foreign Scan on public.async_p2 async_pt_2
+ Output: async_pt_2.a, (async_pt_2.b + 1000), ('AAA'::text || async_pt_2.c)
+ Filter: (async_pt_2.b === 505)
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2
+(10 rows)
+
+INSERT INTO result_tbl SELECT a, b + 1000, 'AAA' || c FROM async_pt WHERE b === 505;
+SELECT * FROM result_tbl ORDER BY a;
+ a | b | c
+------+------+---------
+ 1505 | 1505 | AAA0505
+ 2505 | 1505 | AAA0505
+(2 rows)
+
DELETE FROM result_tbl;
-- Check case where multiple partitions use the same connection
CREATE TABLE base_tbl3 (a int, b int, c text);
@@ -10456,6 +10481,88 @@ DROP TABLE local_tbl;
DROP INDEX base_tbl1_idx;
DROP INDEX base_tbl2_idx;
DROP INDEX async_p3_idx;
+-- UNION queries
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT * FROM async_p2 WHERE b < 10);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------
+ Insert on public.result_tbl
+ -> HashAggregate
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Group Key: async_p1.a, async_p1.b, async_p1.c
+ -> Append
+ -> Async Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1 ORDER BY a ASC NULLS LAST LIMIT 10::bigint
+ -> Async Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((b < 10))
+(11 rows)
+
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT * FROM async_p2 WHERE b < 10);
+SELECT * FROM result_tbl ORDER BY a;
+ a | b | c
+------+----+------
+ 1000 | 0 | 0000
+ 1005 | 5 | 0005
+ 1010 | 10 | 0010
+ 1015 | 15 | 0015
+ 1020 | 20 | 0020
+ 1025 | 25 | 0025
+ 1030 | 30 | 0030
+ 1035 | 35 | 0035
+ 1040 | 40 | 0040
+ 1045 | 45 | 0045
+ 2000 | 0 | 0000
+ 2005 | 5 | 0005
+(12 rows)
+
+DELETE FROM result_tbl;
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT * FROM async_p2 WHERE b < 10);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ Insert on public.result_tbl
+ -> Append
+ -> Async Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1 ORDER BY a ASC NULLS LAST LIMIT 10::bigint
+ -> Async Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((b < 10))
+(8 rows)
+
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT * FROM async_p2 WHERE b < 10);
+SELECT * FROM result_tbl ORDER BY a;
+ a | b | c
+------+----+------
+ 1000 | 0 | 0000
+ 1005 | 5 | 0005
+ 1010 | 10 | 0010
+ 1015 | 15 | 0015
+ 1020 | 20 | 0020
+ 1025 | 25 | 0025
+ 1030 | 30 | 0030
+ 1035 | 35 | 0035
+ 1040 | 40 | 0040
+ 1045 | 45 | 0045
+ 2000 | 0 | 0000
+ 2005 | 5 | 0005
+(12 rows)
+
+DELETE FROM result_tbl;
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 30b5175da5..e881a8fd63 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3210,6 +3210,13 @@ INSERT INTO result_tbl SELECT * FROM async_pt WHERE b === 505;
SELECT * FROM result_tbl ORDER BY a;
DELETE FROM result_tbl;
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl SELECT a, b + 1000, 'AAA' || c FROM async_pt WHERE b === 505;
+INSERT INTO result_tbl SELECT a, b + 1000, 'AAA' || c FROM async_pt WHERE b === 505;
+
+SELECT * FROM result_tbl ORDER BY a;
+DELETE FROM result_tbl;
+
-- Check case where multiple partitions use the same connection
CREATE TABLE base_tbl3 (a int, b int, c text);
CREATE FOREIGN TABLE async_p3 PARTITION OF async_pt FOR VALUES FROM (3000) TO (4000)
@@ -3322,6 +3329,33 @@ DROP INDEX base_tbl1_idx;
DROP INDEX base_tbl2_idx;
DROP INDEX async_p3_idx;
+-- UNION queries
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT * FROM async_p2 WHERE b < 10);
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT * FROM async_p2 WHERE b < 10);
+
+SELECT * FROM result_tbl ORDER BY a;
+DELETE FROM result_tbl;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT * FROM async_p2 WHERE b < 10);
+INSERT INTO result_tbl
+(SELECT * FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT * FROM async_p2 WHERE b < 10);
+
+SELECT * FROM result_tbl ORDER BY a;
+DELETE FROM result_tbl;
+
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 38251c2b8e..5ef1888407 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -626,6 +626,7 @@ _copySubqueryScan(const SubqueryScan *from)
* copy remainder of node
*/
COPY_NODE_FIELD(subplan);
+ COPY_SCALAR_FIELD(status);
return newnode;
}
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 87561cbb6f..0d41a9ab4f 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -629,6 +629,7 @@ _outSubqueryScan(StringInfo str, const SubqueryScan *node)
_outScanInfo(str, (const Scan *) node);
WRITE_NODE_FIELD(subplan);
+ WRITE_ENUM_FIELD(status, SubqueryScanStatus);
}
static void
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 0dd1ad7dfc..ebee3b6acf 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1967,6 +1967,7 @@ _readSubqueryScan(void)
ReadCommonScan(&local_node->scan);
READ_NODE_FIELD(subplan);
+ READ_ENUM_FIELD(status, SubqueryScanStatus);
READ_DONE();
}
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index a5f6d678cc..a420fe09f2 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -82,7 +82,7 @@ static List *get_gating_quals(PlannerInfo *root, List *quals);
static Plan *create_gating_plan(PlannerInfo *root, Path *path, Plan *plan,
List *gating_quals);
static Plan *create_join_plan(PlannerInfo *root, JoinPath *best_path);
-static bool is_async_capable_path(Path *path);
+static bool mark_async_capable_plan(Plan *plan, Path *path);
static Plan *create_append_plan(PlannerInfo *root, AppendPath *best_path,
int flags);
static Plan *create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path,
@@ -1091,14 +1091,25 @@ create_join_plan(PlannerInfo *root, JoinPath *best_path)
}
/*
- * is_async_capable_path
- * Check whether a given Path node is async-capable.
+ * mark_async_capable_plan
+ * Check whether a given Path node is async-capable, and if so, mark the
+ * Plan node created from it as such.
*/
static bool
-is_async_capable_path(Path *path)
+mark_async_capable_plan(Plan *plan, Path *path)
{
switch (nodeTag(path))
{
+ case T_SubqueryScanPath:
+ {
+ SubqueryScan *splan = (SubqueryScan *) plan;
+
+ if (trivial_subqueryscan(splan) &&
+ mark_async_capable_plan(splan->subplan,
+ ((SubqueryScanPath *) path)->subpath))
+ break;
+ return false;
+ }
case T_ForeignPath:
{
FdwRoutine *fdwroutine = path->parent->fdwroutine;
@@ -1106,13 +1117,21 @@ is_async_capable_path(Path *path)
Assert(fdwroutine != NULL);
if (fdwroutine->IsForeignPathAsyncCapable != NULL &&
fdwroutine->IsForeignPathAsyncCapable((ForeignPath *) path))
- return true;
+ break;
+ return false;
}
- break;
+ case T_ProjectionPath:
+ if (!IsA(plan, Result) &&
+ mark_async_capable_plan(plan,
+ ((ProjectionPath *) path)->subpath))
+ return true;
+ return false;
default:
- break;
+ return false;
}
- return false;
+
+ plan->async_capable = true;
+ return true;
}
/*
@@ -1278,9 +1297,9 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
subplans = lappend(subplans, subplan);
/* Check to see if subplan can be executed asynchronously */
- if (consider_async && is_async_capable_path(subpath))
+ if (consider_async && mark_async_capable_plan(subplan, subpath))
{
- subplan->async_capable = true;
+ Assert(subplan->async_capable);
++nasyncplans;
}
}
@@ -5551,6 +5570,7 @@ make_subqueryscan(List *qptlist,
plan->righttree = NULL;
node->scan.scanrelid = scanrelid;
node->subplan = subplan;
+ node->status = SUBQUERY_SCAN_UNKNOWN;
return node;
}
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index e50624c465..a4455b8a44 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -115,7 +115,6 @@ static Plan *set_indexonlyscan_references(PlannerInfo *root,
static Plan *set_subqueryscan_references(PlannerInfo *root,
SubqueryScan *plan,
int rtoffset);
-static bool trivial_subqueryscan(SubqueryScan *plan);
static Plan *clean_up_removed_plan_level(Plan *parent, Plan *child);
static void set_foreignscan_references(PlannerInfo *root,
ForeignScan *fscan,
@@ -1206,13 +1205,22 @@ set_subqueryscan_references(PlannerInfo *root,
* We can delete it if it has no qual to check and the targetlist just
* regurgitates the output of the child plan.
*/
-static bool
+bool
trivial_subqueryscan(SubqueryScan *plan)
{
int attrno;
ListCell *lp,
*lc;
+ /* We might have detected this already */
+ if (plan->status == SUBQUERY_SCAN_TRIVIAL)
+ return true;
+ if (plan->status == SUBQUERY_SCAN_NONTRIVIAL)
+ return false;
+ Assert(plan->status == SUBQUERY_SCAN_UNKNOWN);
+
+ plan->status = SUBQUERY_SCAN_NONTRIVIAL;
+
if (plan->scan.plan.qual != NIL)
return false;
@@ -1254,6 +1262,7 @@ trivial_subqueryscan(SubqueryScan *plan)
attrno++;
}
+ plan->status = SUBQUERY_SCAN_TRIVIAL;
return true;
}
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index ec9a8b0c81..67f21080df 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -518,16 +518,28 @@ typedef struct TidRangeScan
* relation, we make this a descendant of Scan anyway for code-sharing
* purposes.
*
+ * SubqueryScanStatus caches the trivial_subqueryscan property of the node.
+ * SUBQUERY_SCAN_UNKNOWN means not yet determined. This is only used during
+ * planning.
+ *
* Note: we store the sub-plan in the type-specific subplan field, not in
* the generic lefttree field as you might expect. This is because we do
* not want plan-tree-traversal routines to recurse into the subplan without
* knowing that they are changing Query contexts.
* ----------------
*/
+typedef enum SubqueryScanStatus
+{
+ SUBQUERY_SCAN_UNKNOWN,
+ SUBQUERY_SCAN_TRIVIAL,
+ SUBQUERY_SCAN_NONTRIVIAL
+} SubqueryScanStatus;
+
typedef struct SubqueryScan
{
Scan scan;
Plan *subplan;
+ SubqueryScanStatus status;
} SubqueryScan;
/* ----------------
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index bf1adfc52a..c908a49490 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -112,6 +112,7 @@ extern bool innerrel_is_unique(PlannerInfo *root,
* prototypes for plan/setrefs.c
*/
extern Plan *set_plan_references(PlannerInfo *root, Plan *plan);
+extern bool trivial_subqueryscan(SubqueryScan *plan);
extern void record_plan_function_dependency(PlannerInfo *root, Oid funcid);
extern void record_plan_type_dependency(PlannerInfo *root, Oid typid);
extern bool extract_query_dependencies_walker(Node *node, PlannerInfo *root);
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2021-09-15 06:40 Alexander Pyhalov <a.pyhalov@postgrespro.ru>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 33+ messages in thread
From: Alexander Pyhalov @ 2021-09-15 06:40 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Zhihong Yu <zyu@yugabyte.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Etsuro Fujita писал 2021-08-30 12:52:
> On Mon, Aug 30, 2021 at 5:36 PM Andrey V. Lepikhov
>
> To allow async execution in a bit more cases, I modified the patch a
> bit further: a ProjectionPath put directly above an async-capable
> ForeignPath would also be considered async-capable as ForeignScan can
> project and no separate Result is needed in that case, so I modified
> mark_async_capable_plan() as such, and added test cases to the
> postgres_fdw regression test. Attached is an updated version of the
> patch.
>
Hi.
The patch looks good to me and seems to work as expected.
--
Best regards,
Alexander Pyhalov,
Postgres Professional
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-03-13 09:39 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
0 siblings, 1 reply; 33+ messages in thread
From: Etsuro Fujita @ 2022-03-13 09:39 UTC (permalink / raw)
To: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; +Cc: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Zhihong Yu <zyu@yugabyte.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi Alexander,
On Wed, Sep 15, 2021 at 3:40 PM Alexander Pyhalov
<a.pyhalov@postgrespro.ru> wrote:
> Etsuro Fujita писал 2021-08-30 12:52:
> > To allow async execution in a bit more cases, I modified the patch a
> > bit further: a ProjectionPath put directly above an async-capable
> > ForeignPath would also be considered async-capable as ForeignScan can
> > project and no separate Result is needed in that case, so I modified
> > mark_async_capable_plan() as such, and added test cases to the
> > postgres_fdw regression test. Attached is an updated version of the
> > patch.
> The patch looks good to me and seems to work as expected.
Thanks for reviewing! I’m planning to commit the patch.
Sorry for the long delay.
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-03 10:29 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 2 replies; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-03 10:29 UTC (permalink / raw)
To: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; +Cc: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Zhihong Yu <zyu@yugabyte.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sun, Mar 13, 2022 at 6:39 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Wed, Sep 15, 2021 at 3:40 PM Alexander Pyhalov
> <a.pyhalov@postgrespro.ru> wrote:
> > The patch looks good to me and seems to work as expected.
>
> I’m planning to commit the patch.
I polished the patch a bit:
* Reordered a bit of code in create_append_plan() in logical order (no
functional changes).
* Added more comments.
* Added/Tweaked regression test cases.
Also, I added the commit message. Attached is a new version of the
patch. Barring objections, I’ll commit this.
Best regards,
Etsuro Fujita
Attachments:
[application/octet-stream] allow-async-in-more-cases-3.patch (20.3K, ../../CAPmGK17ptUuZPTmzzRA_rcXSS-x++T7Ocv9UVcfYgnC21XQ9Gw@mail.gmail.com/2-allow-async-in-more-cases-3.patch)
download | inline diff:
From 9935ea071408d46c140916dbcca7ff069f9e7d38 Mon Sep 17 00:00:00 2001
From: Etsuro Fujita <etsuro.fujita@gmail.com>
Date: Sun, 3 Apr 2022 18:55:34 +0900
Subject: [PATCH] Allow asynchronous execution in more cases.
In commit 27e1f1456, create_append_plan() only allowed the subplan
created from a given subpath to be executed asynchronously when it was
an async-capable ForeignPath. To extend coverage, this patch handles
cases when the given subpath includes some other Path types as well that
can be omitted in the plan processing, such as a ProjectionPath directly
atop an async-capable ForeignPath, allowing asynchronous execution in
partitioned-scan/partitioned-join queries with non-Var tlist expressions
and more UNION queries.
Andrey Lepikhov and Etsuro Fujita, reviewed by Alexander Pyhalov.
Discussion: https://postgr.es/m/659c37a8-3e71-0ff2-394c-f04428c76f08%40postgrespro.ru
---
contrib/postgres_fdw/expected/postgres_fdw.out | 170 +++++++++++++++++++++++++
contrib/postgres_fdw/sql/postgres_fdw.sql | 41 ++++++
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/outfuncs.c | 1 +
src/backend/nodes/readfuncs.c | 1 +
src/backend/optimizer/plan/createplan.c | 56 ++++++--
src/backend/optimizer/plan/setrefs.c | 18 ++-
src/include/nodes/plannodes.h | 12 ++
src/include/optimizer/planmain.h | 1 +
9 files changed, 286 insertions(+), 15 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 11e9b4e8cc..30e95f585f 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10221,6 +10221,31 @@ SELECT * FROM result_tbl ORDER BY a;
2505 | 505 | 0505
(2 rows)
+DELETE FROM result_tbl;
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl SELECT a, b, 'AAA' || c FROM async_pt WHERE b === 505;
+ QUERY PLAN
+---------------------------------------------------------------------------------
+ Insert on public.result_tbl
+ -> Append
+ -> Async Foreign Scan on public.async_p1 async_pt_1
+ Output: async_pt_1.a, async_pt_1.b, ('AAA'::text || async_pt_1.c)
+ Filter: (async_pt_1.b === 505)
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1
+ -> Async Foreign Scan on public.async_p2 async_pt_2
+ Output: async_pt_2.a, async_pt_2.b, ('AAA'::text || async_pt_2.c)
+ Filter: (async_pt_2.b === 505)
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2
+(10 rows)
+
+INSERT INTO result_tbl SELECT a, b, 'AAA' || c FROM async_pt WHERE b === 505;
+SELECT * FROM result_tbl ORDER BY a;
+ a | b | c
+------+-----+---------
+ 1505 | 505 | AAA0505
+ 2505 | 505 | AAA0505
+(2 rows)
+
DELETE FROM result_tbl;
-- Check case where multiple partitions use the same connection
CREATE TABLE base_tbl3 (a int, b int, c text);
@@ -10358,6 +10383,69 @@ SELECT * FROM join_tbl ORDER BY a1;
3900 | 900 | 0900 | 3900 | 900 | 0900
(30 rows)
+DELETE FROM join_tbl;
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO join_tbl SELECT t1.a, t1.b, 'AAA' || t1.c, t2.a, t2.b, 'AAA' || t2.c FROM async_pt t1, async_pt t2 WHERE t1.a = t2.a AND t1.b = t2.b AND t1.b % 100 = 0;
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Insert on public.join_tbl
+ -> Append
+ -> Async Foreign Scan
+ Output: t1_1.a, t1_1.b, ('AAA'::text || t1_1.c), t2_1.a, t2_1.b, ('AAA'::text || t2_1.c)
+ Relations: (public.async_p1 t1_1) INNER JOIN (public.async_p1 t2_1)
+ Remote SQL: SELECT r5.a, r5.b, r5.c, r8.a, r8.b, r8.c FROM (public.base_tbl1 r5 INNER JOIN public.base_tbl1 r8 ON (((r5.a = r8.a)) AND ((r5.b = r8.b)) AND (((r5.b % 100) = 0))))
+ -> Async Foreign Scan
+ Output: t1_2.a, t1_2.b, ('AAA'::text || t1_2.c), t2_2.a, t2_2.b, ('AAA'::text || t2_2.c)
+ Relations: (public.async_p2 t1_2) INNER JOIN (public.async_p2 t2_2)
+ Remote SQL: SELECT r6.a, r6.b, r6.c, r9.a, r9.b, r9.c FROM (public.base_tbl2 r6 INNER JOIN public.base_tbl2 r9 ON (((r6.a = r9.a)) AND ((r6.b = r9.b)) AND (((r6.b % 100) = 0))))
+ -> Hash Join
+ Output: t1_3.a, t1_3.b, ('AAA'::text || t1_3.c), t2_3.a, t2_3.b, ('AAA'::text || t2_3.c)
+ Hash Cond: ((t2_3.a = t1_3.a) AND (t2_3.b = t1_3.b))
+ -> Seq Scan on public.async_p3 t2_3
+ Output: t2_3.a, t2_3.b, t2_3.c
+ -> Hash
+ Output: t1_3.a, t1_3.b, t1_3.c
+ -> Seq Scan on public.async_p3 t1_3
+ Output: t1_3.a, t1_3.b, t1_3.c
+ Filter: ((t1_3.b % 100) = 0)
+(20 rows)
+
+INSERT INTO join_tbl SELECT t1.a, t1.b, 'AAA' || t1.c, t2.a, t2.b, 'AAA' || t2.c FROM async_pt t1, async_pt t2 WHERE t1.a = t2.a AND t1.b = t2.b AND t1.b % 100 = 0;
+SELECT * FROM join_tbl ORDER BY a1;
+ a1 | b1 | c1 | a2 | b2 | c2
+------+-----+---------+------+-----+---------
+ 1000 | 0 | AAA0000 | 1000 | 0 | AAA0000
+ 1100 | 100 | AAA0100 | 1100 | 100 | AAA0100
+ 1200 | 200 | AAA0200 | 1200 | 200 | AAA0200
+ 1300 | 300 | AAA0300 | 1300 | 300 | AAA0300
+ 1400 | 400 | AAA0400 | 1400 | 400 | AAA0400
+ 1500 | 500 | AAA0500 | 1500 | 500 | AAA0500
+ 1600 | 600 | AAA0600 | 1600 | 600 | AAA0600
+ 1700 | 700 | AAA0700 | 1700 | 700 | AAA0700
+ 1800 | 800 | AAA0800 | 1800 | 800 | AAA0800
+ 1900 | 900 | AAA0900 | 1900 | 900 | AAA0900
+ 2000 | 0 | AAA0000 | 2000 | 0 | AAA0000
+ 2100 | 100 | AAA0100 | 2100 | 100 | AAA0100
+ 2200 | 200 | AAA0200 | 2200 | 200 | AAA0200
+ 2300 | 300 | AAA0300 | 2300 | 300 | AAA0300
+ 2400 | 400 | AAA0400 | 2400 | 400 | AAA0400
+ 2500 | 500 | AAA0500 | 2500 | 500 | AAA0500
+ 2600 | 600 | AAA0600 | 2600 | 600 | AAA0600
+ 2700 | 700 | AAA0700 | 2700 | 700 | AAA0700
+ 2800 | 800 | AAA0800 | 2800 | 800 | AAA0800
+ 2900 | 900 | AAA0900 | 2900 | 900 | AAA0900
+ 3000 | 0 | AAA0000 | 3000 | 0 | AAA0000
+ 3100 | 100 | AAA0100 | 3100 | 100 | AAA0100
+ 3200 | 200 | AAA0200 | 3200 | 200 | AAA0200
+ 3300 | 300 | AAA0300 | 3300 | 300 | AAA0300
+ 3400 | 400 | AAA0400 | 3400 | 400 | AAA0400
+ 3500 | 500 | AAA0500 | 3500 | 500 | AAA0500
+ 3600 | 600 | AAA0600 | 3600 | 600 | AAA0600
+ 3700 | 700 | AAA0700 | 3700 | 700 | AAA0700
+ 3800 | 800 | AAA0800 | 3800 | 800 | AAA0800
+ 3900 | 900 | AAA0900 | 3900 | 900 | AAA0900
+(30 rows)
+
DELETE FROM join_tbl;
RESET enable_partitionwise_join;
-- Test rescan of an async Append node with do_exec_prune=false
@@ -10536,6 +10624,88 @@ DROP TABLE local_tbl;
DROP INDEX base_tbl1_idx;
DROP INDEX base_tbl2_idx;
DROP INDEX async_p3_idx;
+-- UNION queries
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT a, b, 'AAA' || c FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT a, b, 'AAA' || c FROM async_p2 WHERE b < 10);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------
+ Insert on public.result_tbl
+ -> HashAggregate
+ Output: async_p1.a, async_p1.b, (('AAA'::text || async_p1.c))
+ Group Key: async_p1.a, async_p1.b, (('AAA'::text || async_p1.c))
+ -> Append
+ -> Async Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, ('AAA'::text || async_p1.c)
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1 ORDER BY a ASC NULLS LAST LIMIT 10::bigint
+ -> Async Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, ('AAA'::text || async_p2.c)
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((b < 10))
+(11 rows)
+
+INSERT INTO result_tbl
+(SELECT a, b, 'AAA' || c FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT a, b, 'AAA' || c FROM async_p2 WHERE b < 10);
+SELECT * FROM result_tbl ORDER BY a;
+ a | b | c
+------+----+---------
+ 1000 | 0 | AAA0000
+ 1005 | 5 | AAA0005
+ 1010 | 10 | AAA0010
+ 1015 | 15 | AAA0015
+ 1020 | 20 | AAA0020
+ 1025 | 25 | AAA0025
+ 1030 | 30 | AAA0030
+ 1035 | 35 | AAA0035
+ 1040 | 40 | AAA0040
+ 1045 | 45 | AAA0045
+ 2000 | 0 | AAA0000
+ 2005 | 5 | AAA0005
+(12 rows)
+
+DELETE FROM result_tbl;
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT a, b, 'AAA' || c FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT a, b, 'AAA' || c FROM async_p2 WHERE b < 10);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ Insert on public.result_tbl
+ -> Append
+ -> Async Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, ('AAA'::text || async_p1.c)
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1 ORDER BY a ASC NULLS LAST LIMIT 10::bigint
+ -> Async Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, ('AAA'::text || async_p2.c)
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((b < 10))
+(8 rows)
+
+INSERT INTO result_tbl
+(SELECT a, b, 'AAA' || c FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT a, b, 'AAA' || c FROM async_p2 WHERE b < 10);
+SELECT * FROM result_tbl ORDER BY a;
+ a | b | c
+------+----+---------
+ 1000 | 0 | AAA0000
+ 1005 | 5 | AAA0005
+ 1010 | 10 | AAA0010
+ 1015 | 15 | AAA0015
+ 1020 | 20 | AAA0020
+ 1025 | 25 | AAA0025
+ 1030 | 30 | AAA0030
+ 1035 | 35 | AAA0035
+ 1040 | 40 | AAA0040
+ 1045 | 45 | AAA0045
+ 2000 | 0 | AAA0000
+ 2005 | 5 | AAA0005
+(12 rows)
+
+DELETE FROM result_tbl;
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 6b5de89e14..ea35e61eb8 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3245,6 +3245,13 @@ INSERT INTO result_tbl SELECT * FROM async_pt WHERE b === 505;
SELECT * FROM result_tbl ORDER BY a;
DELETE FROM result_tbl;
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl SELECT a, b, 'AAA' || c FROM async_pt WHERE b === 505;
+INSERT INTO result_tbl SELECT a, b, 'AAA' || c FROM async_pt WHERE b === 505;
+
+SELECT * FROM result_tbl ORDER BY a;
+DELETE FROM result_tbl;
+
-- Check case where multiple partitions use the same connection
CREATE TABLE base_tbl3 (a int, b int, c text);
CREATE FOREIGN TABLE async_p3 PARTITION OF async_pt FOR VALUES FROM (3000) TO (4000)
@@ -3286,6 +3293,13 @@ INSERT INTO join_tbl SELECT * FROM async_pt t1, async_pt t2 WHERE t1.a = t2.a AN
SELECT * FROM join_tbl ORDER BY a1;
DELETE FROM join_tbl;
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO join_tbl SELECT t1.a, t1.b, 'AAA' || t1.c, t2.a, t2.b, 'AAA' || t2.c FROM async_pt t1, async_pt t2 WHERE t1.a = t2.a AND t1.b = t2.b AND t1.b % 100 = 0;
+INSERT INTO join_tbl SELECT t1.a, t1.b, 'AAA' || t1.c, t2.a, t2.b, 'AAA' || t2.c FROM async_pt t1, async_pt t2 WHERE t1.a = t2.a AND t1.b = t2.b AND t1.b % 100 = 0;
+
+SELECT * FROM join_tbl ORDER BY a1;
+DELETE FROM join_tbl;
+
RESET enable_partitionwise_join;
-- Test rescan of an async Append node with do_exec_prune=false
@@ -3357,6 +3371,33 @@ DROP INDEX base_tbl1_idx;
DROP INDEX base_tbl2_idx;
DROP INDEX async_p3_idx;
+-- UNION queries
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT a, b, 'AAA' || c FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT a, b, 'AAA' || c FROM async_p2 WHERE b < 10);
+INSERT INTO result_tbl
+(SELECT a, b, 'AAA' || c FROM async_p1 ORDER BY a LIMIT 10)
+UNION
+(SELECT a, b, 'AAA' || c FROM async_p2 WHERE b < 10);
+
+SELECT * FROM result_tbl ORDER BY a;
+DELETE FROM result_tbl;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+INSERT INTO result_tbl
+(SELECT a, b, 'AAA' || c FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT a, b, 'AAA' || c FROM async_p2 WHERE b < 10);
+INSERT INTO result_tbl
+(SELECT a, b, 'AAA' || c FROM async_p1 ORDER BY a LIMIT 10)
+UNION ALL
+(SELECT a, b, 'AAA' || c FROM async_p2 WHERE b < 10);
+
+SELECT * FROM result_tbl ORDER BY a;
+DELETE FROM result_tbl;
+
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 11c016495e..e2f464f8cb 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -632,6 +632,7 @@ _copySubqueryScan(const SubqueryScan *from)
* copy remainder of node
*/
COPY_NODE_FIELD(subplan);
+ COPY_SCALAR_FIELD(status);
return newnode;
}
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 6e39590730..b572d0727b 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -638,6 +638,7 @@ _outSubqueryScan(StringInfo str, const SubqueryScan *node)
_outScanInfo(str, (const Scan *) node);
WRITE_NODE_FIELD(subplan);
+ WRITE_ENUM_FIELD(status, SubqueryScanStatus);
}
static void
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index c94b2561f0..af7a49f655 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -2164,6 +2164,7 @@ _readSubqueryScan(void)
ReadCommonScan(&local_node->scan);
READ_NODE_FIELD(subplan);
+ READ_ENUM_FIELD(status, SubqueryScanStatus);
READ_DONE();
}
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 179c87c671..2fd297e716 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -82,7 +82,7 @@ static List *get_gating_quals(PlannerInfo *root, List *quals);
static Plan *create_gating_plan(PlannerInfo *root, Path *path, Plan *plan,
List *gating_quals);
static Plan *create_join_plan(PlannerInfo *root, JoinPath *best_path);
-static bool is_async_capable_path(Path *path);
+static bool mark_async_capable_plan(Plan *plan, Path *path);
static Plan *create_append_plan(PlannerInfo *root, AppendPath *best_path,
int flags);
static Plan *create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path,
@@ -1110,14 +1110,29 @@ create_join_plan(PlannerInfo *root, JoinPath *best_path)
}
/*
- * is_async_capable_path
- * Check whether a given Path node is async-capable.
+ * mark_async_capable_plan
+ * Check whether a given Path node is async-capable, and if so, mark the
+ * Plan node created from it as such.
*/
static bool
-is_async_capable_path(Path *path)
+mark_async_capable_plan(Plan *plan, Path *path)
{
switch (nodeTag(path))
{
+ case T_SubqueryScanPath:
+ {
+ SubqueryScan *scan_plan = (SubqueryScan *) plan;
+
+ /*
+ * If a SubqueryScan node atop of an async-capable plan node
+ * is deletable, consider it as async-capable.
+ */
+ if (trivial_subqueryscan(scan_plan) &&
+ mark_async_capable_plan(scan_plan->subplan,
+ ((SubqueryScanPath *) path)->subpath))
+ break;
+ return false;
+ }
case T_ForeignPath:
{
FdwRoutine *fdwroutine = path->parent->fdwroutine;
@@ -1125,13 +1140,27 @@ is_async_capable_path(Path *path)
Assert(fdwroutine != NULL);
if (fdwroutine->IsForeignPathAsyncCapable != NULL &&
fdwroutine->IsForeignPathAsyncCapable((ForeignPath *) path))
- return true;
+ break;
+ return false;
}
- break;
+ case T_ProjectionPath:
+
+ /*
+ * If the generated plan node doesn't include a Result node,
+ * consider it as async-capable if the subpath is async-capable.
+ */
+ if (!IsA(plan, Result) &&
+ mark_async_capable_plan(plan,
+ ((ProjectionPath *) path)->subpath))
+ return true;
+ return false;
default:
- break;
+ return false;
}
- return false;
+
+ plan->async_capable = true;
+
+ return true;
}
/*
@@ -1294,14 +1323,14 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
}
}
- subplans = lappend(subplans, subplan);
-
- /* Check to see if subplan can be executed asynchronously */
- if (consider_async && is_async_capable_path(subpath))
+ /* If needed, check to see if subplan can be executed asynchronously */
+ if (consider_async && mark_async_capable_plan(subplan, subpath))
{
- subplan->async_capable = true;
+ Assert(subplan->async_capable);
++nasyncplans;
}
+
+ subplans = lappend(subplans, subplan);
}
/*
@@ -5598,6 +5627,7 @@ make_subqueryscan(List *qptlist,
plan->righttree = NULL;
node->scan.scanrelid = scanrelid;
node->subplan = subplan;
+ node->status = SUBQUERY_SCAN_UNKNOWN;
return node;
}
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index bf4c722c02..df55ae96ba 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -115,7 +115,6 @@ static Plan *set_indexonlyscan_references(PlannerInfo *root,
static Plan *set_subqueryscan_references(PlannerInfo *root,
SubqueryScan *plan,
int rtoffset);
-static bool trivial_subqueryscan(SubqueryScan *plan);
static Plan *clean_up_removed_plan_level(Plan *parent, Plan *child);
static void set_foreignscan_references(PlannerInfo *root,
ForeignScan *fscan,
@@ -1319,14 +1318,26 @@ set_subqueryscan_references(PlannerInfo *root,
*
* We can delete it if it has no qual to check and the targetlist just
* regurgitates the output of the child plan.
+ *
+ * This might be called repeatedly on a SubqueryScan node, so we cache the
+ * result in the SubqueryScan node to avoid repeated computation.
*/
-static bool
+bool
trivial_subqueryscan(SubqueryScan *plan)
{
int attrno;
ListCell *lp,
*lc;
+ /* We might have detected this already (see mark_async_capable_plan) */
+ if (plan->status == SUBQUERY_SCAN_TRIVIAL)
+ return true;
+ if (plan->status == SUBQUERY_SCAN_NONTRIVIAL)
+ return false;
+ Assert(plan->status == SUBQUERY_SCAN_UNKNOWN);
+ /* Initially, mark the SubqueryScan as non-deletable from the plan tree */
+ plan->status = SUBQUERY_SCAN_NONTRIVIAL;
+
if (plan->scan.plan.qual != NIL)
return false;
@@ -1368,6 +1379,9 @@ trivial_subqueryscan(SubqueryScan *plan)
attrno++;
}
+ /* Re-mark the SubqueryScan as deletable from the plan tree */
+ plan->status = SUBQUERY_SCAN_TRIVIAL;
+
return true;
}
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 50ef3dda05..430c95bca9 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -536,16 +536,28 @@ typedef struct TidRangeScan
* relation, we make this a descendant of Scan anyway for code-sharing
* purposes.
*
+ * SubqueryScanStatus caches the trivial_subqueryscan property of the node.
+ * SUBQUERY_SCAN_UNKNOWN means not yet determined. This is only used during
+ * planning.
+ *
* Note: we store the sub-plan in the type-specific subplan field, not in
* the generic lefttree field as you might expect. This is because we do
* not want plan-tree-traversal routines to recurse into the subplan without
* knowing that they are changing Query contexts.
* ----------------
*/
+typedef enum SubqueryScanStatus
+{
+ SUBQUERY_SCAN_UNKNOWN,
+ SUBQUERY_SCAN_TRIVIAL,
+ SUBQUERY_SCAN_NONTRIVIAL
+} SubqueryScanStatus;
+
typedef struct SubqueryScan
{
Scan scan;
Plan *subplan;
+ SubqueryScanStatus status;
} SubqueryScan;
/* ----------------
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index 54a0d4c188..6947bc65d1 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -112,6 +112,7 @@ extern bool innerrel_is_unique(PlannerInfo *root,
* prototypes for plan/setrefs.c
*/
extern Plan *set_plan_references(PlannerInfo *root, Plan *plan);
+extern bool trivial_subqueryscan(SubqueryScan *plan);
extern void record_plan_function_dependency(PlannerInfo *root, Oid funcid);
extern void record_plan_type_dependency(PlannerInfo *root, Oid typid);
extern bool extract_query_dependencies_walker(Node *node, PlannerInfo *root);
--
2.14.3 (Apple Git-98)
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-03 14:42 Zhihong Yu <zyu@yugabyte.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
1 sibling, 1 reply; 33+ messages in thread
From: Zhihong Yu @ 2022-04-03 14:42 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sun, Apr 3, 2022 at 3:28 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:
> On Sun, Mar 13, 2022 at 6:39 PM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> > On Wed, Sep 15, 2021 at 3:40 PM Alexander Pyhalov
> > <a.pyhalov@postgrespro.ru> wrote:
> > > The patch looks good to me and seems to work as expected.
> >
> > I’m planning to commit the patch.
>
> I polished the patch a bit:
>
> * Reordered a bit of code in create_append_plan() in logical order (no
> functional changes).
> * Added more comments.
> * Added/Tweaked regression test cases.
>
> Also, I added the commit message. Attached is a new version of the
> patch. Barring objections, I’ll commit this.
>
> Best regards,
> Etsuro Fujita
>
Hi,
+ WRITE_ENUM_FIELD(status, SubqueryScanStatus);
Looks like the new field can be named subquerystatus - this way its purpose
is clearer.
+ * mark_async_capable_plan
+ * Check whether a given Path node is async-capable, and if so, mark
the
+ * Plan node created from it as such.
Please add comment explaining what the return value means.
+ if (!IsA(plan, Result) &&
+ mark_async_capable_plan(plan,
+ ((ProjectionPath *) path)->subpath))
+ return true;
by returning true, `plan->async_capable = true;` is skipped.
Is that intentional ?
Cheers
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-04 04:06 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Zhihong Yu <zyu@yugabyte.com>
0 siblings, 1 reply; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-04 04:06 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi Zhihong,
On Sun, Apr 3, 2022 at 11:38 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> + WRITE_ENUM_FIELD(status, SubqueryScanStatus);
>
> Looks like the new field can be named subquerystatus - this way its purpose is clearer.
I agree that “status” is too general. “subquerystatus” might be good,
but I’d like to propose “scanstatus” instead, because I think this
would be consistent with the naming of the RowMarkType-enum member
“markType” in PlanRowMark defined in the same file.
> + * mark_async_capable_plan
> + * Check whether a given Path node is async-capable, and if so, mark the
> + * Plan node created from it as such.
>
> Please add comment explaining what the return value means.
Ok, how about something like this?
“Check whether a given Path node is async-capable, and if so, mark the
Plan node created from it as such and return true; otherwise, return
false.”
> + if (!IsA(plan, Result) &&
> + mark_async_capable_plan(plan,
> + ((ProjectionPath *) path)->subpath))
> + return true;
>
> by returning true, `plan->async_capable = true;` is skipped.
> Is that intentional ?
That is intentional; we don’t need to set the async_capable flag
because in that case the flag would already have been set by the above
mark_async_capable_plan(). Note that we pass “plan” to that function.
Thanks for reviewing!
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-04 09:30 Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
1 sibling, 1 reply; 33+ messages in thread
From: Andrey V. Lepikhov @ 2022-04-04 09:30 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; +Cc: Zhihong Yu <zyu@yugabyte.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 4/3/22 15:29, Etsuro Fujita wrote:
> On Sun, Mar 13, 2022 at 6:39 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> On Wed, Sep 15, 2021 at 3:40 PM Alexander Pyhalov
>> <a.pyhalov@postgrespro.ru> wrote:
>>> The patch looks good to me and seems to work as expected.
>>
>> I’m planning to commit the patch.
>
> I polished the patch a bit:
>
> * Reordered a bit of code in create_append_plan() in logical order (no
> functional changes).
> * Added more comments.
> * Added/Tweaked regression test cases.
>
> Also, I added the commit message. Attached is a new version of the
> patch. Barring objections, I’ll commit this.
Sorry for late answer - just vacation.
I looked through this patch - looks much more stable.
But, as far as I remember, on previous version some problems were found
out on the TPC-H test. I want to play a bit with the TPC-H and with
parameterized plans.
--
regards,
Andrey Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-06 06:58 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 2 replies; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-06 06:58 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Apr 4, 2022 at 1:06 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Sun, Apr 3, 2022 at 11:38 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> > + WRITE_ENUM_FIELD(status, SubqueryScanStatus);
> >
> > Looks like the new field can be named subquerystatus - this way its purpose is clearer.
>
> I agree that “status” is too general. “subquerystatus” might be good,
> but I’d like to propose “scanstatus” instead, because I think this
> would be consistent with the naming of the RowMarkType-enum member
> “markType” in PlanRowMark defined in the same file.
>
> > + * mark_async_capable_plan
> > + * Check whether a given Path node is async-capable, and if so, mark the
> > + * Plan node created from it as such.
> >
> > Please add comment explaining what the return value means.
>
> Ok, how about something like this?
>
> “Check whether a given Path node is async-capable, and if so, mark the
> Plan node created from it as such and return true; otherwise, return
> false.”
I have committed the patch after modifying it as such. (I think we
can improve these later, if necessary.)
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-06 07:05 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 0 replies; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-06 07:05 UTC (permalink / raw)
To: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Zhihong Yu <zyu@yugabyte.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Apr 4, 2022 at 6:30 PM Andrey V. Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> On 4/3/22 15:29, Etsuro Fujita wrote:
> > Also, I added the commit message. Attached is a new version of the
> > patch. Barring objections, I’ll commit this.
> I looked through this patch - looks much more stable.
> But, as far as I remember, on previous version some problems were found
> out on the TPC-H test. I want to play a bit with the TPC-H and with
> parameterized plans.
I might be missing something, but I don't see any problems, so I have
committed the patch after some modifications. If you find them,
please let me know.
Thanks!
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-08 12:43 Justin Pryzby <pryzby@telsasoft.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
1 sibling, 2 replies; 33+ messages in thread
From: Justin Pryzby @ 2022-04-08 12:43 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Zhihong Yu <zyu@yugabyte.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-hackers@lists.postgresql.org
On Wed, Apr 06, 2022 at 03:58:29PM +0900, Etsuro Fujita wrote:
> I have committed the patch after modifying it as such. (I think we
> can improve these later, if necessary.)
This patch seems to be causing the planner to crash.
Here's a query reduced from sqlsmith.
| explain SELECT 1 FROM information_schema.constraint_column_usage WHERE 1 <= pg_trigger_depth();
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x000055b4396a2edf in trivial_subqueryscan (plan=0x7f4219ed93b0) at ../../../../src/include/nodes/pg_list.h:151
151 return l ? l->length : 0;
(gdb) bt
#0 0x000055b4396a2edf in trivial_subqueryscan (plan=0x7f4219ed93b0) at ../../../../src/include/nodes/pg_list.h:151
#1 0x000055b43968af89 in mark_async_capable_plan (plan=plan@entry=0x7f4219ed93b0, path=path@entry=0x7f4219e89538) at createplan.c:1132
#2 0x000055b439691924 in create_append_plan (root=root@entry=0x55b43affb2b0, best_path=best_path@entry=0x7f4219ed0cb8, flags=flags@entry=0) at createplan.c:1329
#3 0x000055b43968fa21 in create_plan_recurse (root=root@entry=0x55b43affb2b0, best_path=best_path@entry=0x7f4219ed0cb8, flags=flags@entry=0) at createplan.c:421
#4 0x000055b43968f974 in create_projection_plan (root=root@entry=0x55b43affb2b0, best_path=best_path@entry=0x7f4219ed0f60, flags=flags@entry=1) at createplan.c:2039
#5 0x000055b43968fa6f in create_plan_recurse (root=root@entry=0x55b43affb2b0, best_path=0x7f4219ed0f60, flags=flags@entry=1) at createplan.c:433
#6 0x000055b439690221 in create_plan (root=root@entry=0x55b43affb2b0, best_path=<optimized out>) at createplan.c:348
#7 0x000055b4396a1451 in standard_planner (parse=0x55b43af05e28, query_string=<optimized out>, cursorOptions=2048, boundParams=0x0) at planner.c:413
#8 0x000055b4396a19c1 in planner (parse=parse@entry=0x55b43af05e28, query_string=query_string@entry=0x55b43af04c40 "SELECT 1 FROM information_schema.constraint_column_usage WHERE 1 > pg_trigger_depth();",
cursorOptions=cursorOptions@entry=2048, boundParams=boundParams@entry=0x0) at planner.c:277
#9 0x000055b439790c78 in pg_plan_query (querytree=querytree@entry=0x55b43af05e28, query_string=query_string@entry=0x55b43af04c40 "SELECT 1 FROM information_schema.constraint_column_usage WHERE 1 > pg_trigger_depth();",
cursorOptions=cursorOptions@entry=2048, boundParams=boundParams@entry=0x0) at postgres.c:883
#10 0x000055b439790d54 in pg_plan_queries (querytrees=0x55b43afdd528, query_string=query_string@entry=0x55b43af04c40 "SELECT 1 FROM information_schema.constraint_column_usage WHERE 1 > pg_trigger_depth();",
cursorOptions=cursorOptions@entry=2048, boundParams=boundParams@entry=0x0) at postgres.c:975
#11 0x000055b439791239 in exec_simple_query (query_string=query_string@entry=0x55b43af04c40 "SELECT 1 FROM information_schema.constraint_column_usage WHERE 1 > pg_trigger_depth();") at postgres.c:1169
#12 0x000055b439793183 in PostgresMain (dbname=<optimized out>, username=<optimized out>) at postgres.c:4542
#13 0x000055b4396e6af7 in BackendRun (port=port@entry=0x55b43af2ffe0) at postmaster.c:4489
#14 0x000055b4396e9c03 in BackendStartup (port=port@entry=0x55b43af2ffe0) at postmaster.c:4217
#15 0x000055b4396e9e4a in ServerLoop () at postmaster.c:1791
#16 0x000055b4396eb401 in PostmasterMain (argc=7, argv=<optimized out>) at postmaster.c:1463
#17 0x000055b43962b4df in main (argc=7, argv=0x55b43aeff0c0) at main.c:202
Actually, the original query failed like this:
#2 0x000055b4398e9f90 in ExceptionalCondition (conditionName=conditionName@entry=0x55b439a61238 "plan->scanstatus == SUBQUERY_SCAN_UNKNOWN", errorType=errorType@entry=0x55b43994b00b "FailedAssertion",
#3 0x000055b4396a2ecf in trivial_subqueryscan (plan=0x55b43b59cac8) at setrefs.c:1367
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-08 16:28 Zhihong Yu <zyu@yugabyte.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 1 reply; 33+ messages in thread
From: Zhihong Yu @ 2022-04-08 16:28 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Etsuro Fujita <etsuro.fujita@gmail.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Fri, Apr 8, 2022 at 5:43 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Wed, Apr 06, 2022 at 03:58:29PM +0900, Etsuro Fujita wrote:
> > I have committed the patch after modifying it as such. (I think we
> > can improve these later, if necessary.)
>
> This patch seems to be causing the planner to crash.
> Here's a query reduced from sqlsmith.
>
> | explain SELECT 1 FROM information_schema.constraint_column_usage WHERE 1
> <= pg_trigger_depth();
>
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0 0x000055b4396a2edf in trivial_subqueryscan (plan=0x7f4219ed93b0) at
> ../../../../src/include/nodes/pg_list.h:151
> 151 return l ? l->length : 0;
> (gdb) bt
> #0 0x000055b4396a2edf in trivial_subqueryscan (plan=0x7f4219ed93b0) at
> ../../../../src/include/nodes/pg_list.h:151
> #1 0x000055b43968af89 in mark_async_capable_plan (plan=plan@entry=0x7f4219ed93b0,
> path=path@entry=0x7f4219e89538) at createplan.c:1132
> #2 0x000055b439691924 in create_append_plan (root=root@entry=0x55b43affb2b0,
> best_path=best_path@entry=0x7f4219ed0cb8, flags=flags@entry=0) at
> createplan.c:1329
> #3 0x000055b43968fa21 in create_plan_recurse (root=root@entry=0x55b43affb2b0,
> best_path=best_path@entry=0x7f4219ed0cb8, flags=flags@entry=0) at
> createplan.c:421
> #4 0x000055b43968f974 in create_projection_plan (root=root@entry=0x55b43affb2b0,
> best_path=best_path@entry=0x7f4219ed0f60, flags=flags@entry=1) at
> createplan.c:2039
> #5 0x000055b43968fa6f in create_plan_recurse (root=root@entry=0x55b43affb2b0,
> best_path=0x7f4219ed0f60, flags=flags@entry=1) at createplan.c:433
> #6 0x000055b439690221 in create_plan (root=root@entry=0x55b43affb2b0,
> best_path=<optimized out>) at createplan.c:348
> #7 0x000055b4396a1451 in standard_planner (parse=0x55b43af05e28,
> query_string=<optimized out>, cursorOptions=2048, boundParams=0x0) at
> planner.c:413
> #8 0x000055b4396a19c1 in planner (parse=parse@entry=0x55b43af05e28,
> query_string=query_string@entry=0x55b43af04c40 "SELECT 1 FROM
> information_schema.constraint_column_usage WHERE 1 > pg_trigger_depth();",
> cursorOptions=cursorOptions@entry=2048, boundParams=boundParams@entry=0x0)
> at planner.c:277
> #9 0x000055b439790c78 in pg_plan_query (querytree=querytree@entry=0x55b43af05e28,
> query_string=query_string@entry=0x55b43af04c40 "SELECT 1 FROM
> information_schema.constraint_column_usage WHERE 1 > pg_trigger_depth();",
> cursorOptions=cursorOptions@entry=2048, boundParams=boundParams@entry=0x0)
> at postgres.c:883
> #10 0x000055b439790d54 in pg_plan_queries (querytrees=0x55b43afdd528,
> query_string=query_string@entry=0x55b43af04c40 "SELECT 1 FROM
> information_schema.constraint_column_usage WHERE 1 > pg_trigger_depth();",
> cursorOptions=cursorOptions@entry=2048, boundParams=boundParams@entry=0x0)
> at postgres.c:975
> #11 0x000055b439791239 in exec_simple_query
> (query_string=query_string@entry=0x55b43af04c40 "SELECT 1 FROM
> information_schema.constraint_column_usage WHERE 1 > pg_trigger_depth();")
> at postgres.c:1169
> #12 0x000055b439793183 in PostgresMain (dbname=<optimized out>,
> username=<optimized out>) at postgres.c:4542
> #13 0x000055b4396e6af7 in BackendRun (port=port@entry=0x55b43af2ffe0) at
> postmaster.c:4489
> #14 0x000055b4396e9c03 in BackendStartup (port=port@entry=0x55b43af2ffe0)
> at postmaster.c:4217
> #15 0x000055b4396e9e4a in ServerLoop () at postmaster.c:1791
> #16 0x000055b4396eb401 in PostmasterMain (argc=7, argv=<optimized out>) at
> postmaster.c:1463
> #17 0x000055b43962b4df in main (argc=7, argv=0x55b43aeff0c0) at main.c:202
>
> Actually, the original query failed like this:
> #2 0x000055b4398e9f90 in ExceptionalCondition
> (conditionName=conditionName@entry=0x55b439a61238 "plan->scanstatus ==
> SUBQUERY_SCAN_UNKNOWN", errorType=errorType@entry=0x55b43994b00b
> "FailedAssertion",
> #3 0x000055b4396a2ecf in trivial_subqueryscan (plan=0x55b43b59cac8) at
> setrefs.c:1367
>
Hi,
I logged the value of plan->scanstatus before the assertion :
2022-04-08 16:20:59.601 UTC [26325] LOG: scan status 0
2022-04-08 16:20:59.601 UTC [26325] STATEMENT: explain SELECT 1 FROM
information_schema.constraint_column_usage WHERE 1 <= pg_trigger_depth();
2022-04-08 16:20:59.796 UTC [26296] LOG: server process (PID 26325) was
terminated by signal 11: Segmentation fault
It seems its value was SUBQUERY_SCAN_UNKNOWN.
Still trying to find out the cause for the crash.
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-08 16:58 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 1 reply; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-08 16:58 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Zhihong Yu <zyu@yugabyte.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-hackers@lists.postgresql.org
Hi,
On Fri, Apr 8, 2022 at 9:43 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> This patch seems to be causing the planner to crash.
> Here's a query reduced from sqlsmith.
>
> | explain SELECT 1 FROM information_schema.constraint_column_usage WHERE 1 <= pg_trigger_depth();
>
> Program terminated with signal SIGSEGV, Segmentation fault.
Reproduced. Will look into this.
Thanks for the report!
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-10 10:43 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 2 replies; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-10 10:43 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Zhihong Yu <zyu@yugabyte.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-hackers@lists.postgresql.org
On Sat, Apr 9, 2022 at 1:58 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Fri, Apr 8, 2022 at 9:43 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > This patch seems to be causing the planner to crash.
> > Here's a query reduced from sqlsmith.
> >
> > | explain SELECT 1 FROM information_schema.constraint_column_usage WHERE 1 <= pg_trigger_depth();
> >
> > Program terminated with signal SIGSEGV, Segmentation fault.
>
> Reproduced. Will look into this.
I think the cause of this is that mark_async_capable_plan() failed to
take into account that when the given path is a SubqueryScanPath or
ForeignPath, the given corresponding plan might include a gating
Result node that evaluates pseudoconstant quals. My oversight. :-(
Attached is a patch for fixing that. I think v14 has the same issue,
so I think we need backpatching.
Best regards,
Etsuro Fujita
Attachments:
[application/octet-stream] prevent-async.patch (5.5K, ../../CAPmGK15NkuaVo0Fu_0TfoCpPPJaJi4OMLzEQtkE6Bt6YT52fPQ@mail.gmail.com/2-prevent-async.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 30e95f585f..5f74595198 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10706,6 +10706,72 @@ SELECT * FROM result_tbl ORDER BY a;
(12 rows)
DELETE FROM result_tbl;
+-- Prevent async execution if we use gating Result nodes for pseudoconstant
+-- quals
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM async_pt WHERE CURRENT_USER = SESSION_USER;
+ QUERY PLAN
+----------------------------------------------------------------
+ Append
+ -> Result
+ Output: async_pt_1.a, async_pt_1.b, async_pt_1.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p1 async_pt_1
+ Output: async_pt_1.a, async_pt_1.b, async_pt_1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1
+ -> Result
+ Output: async_pt_2.a, async_pt_2.b, async_pt_2.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p2 async_pt_2
+ Output: async_pt_2.a, async_pt_2.b, async_pt_2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2
+ -> Result
+ Output: async_pt_3.a, async_pt_3.b, async_pt_3.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Seq Scan on public.async_p3 async_pt_3
+ Output: async_pt_3.a, async_pt_3.b, async_pt_3.c
+(18 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF)
+(SELECT * FROM async_p1 WHERE CURRENT_USER = SESSION_USER)
+UNION ALL
+(SELECT * FROM async_p2 WHERE CURRENT_USER = SESSION_USER);
+ QUERY PLAN
+----------------------------------------------------------------
+ Append
+ -> Result
+ Output: async_p1.a, async_p1.b, async_p1.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1
+ -> Result
+ Output: async_p2.a, async_p2.b, async_p2.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2
+(13 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ((SELECT * FROM async_p1 WHERE b < 10) UNION ALL (SELECT * FROM async_p2 WHERE b < 10)) s WHERE CURRENT_USER = SESSION_USER;
+ QUERY PLAN
+---------------------------------------------------------------------------------
+ Append
+ -> Result
+ Output: async_p1.a, async_p1.b, async_p1.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1 WHERE ((b < 10))
+ -> Result
+ Output: async_p2.a, async_p2.b, async_p2.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((b < 10))
+(13 rows)
+
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index ea35e61eb8..e1b93f39f1 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3398,6 +3398,19 @@ UNION ALL
SELECT * FROM result_tbl ORDER BY a;
DELETE FROM result_tbl;
+-- Prevent async execution if we use gating Result nodes for pseudoconstant
+-- quals
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM async_pt WHERE CURRENT_USER = SESSION_USER;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+(SELECT * FROM async_p1 WHERE CURRENT_USER = SESSION_USER)
+UNION ALL
+(SELECT * FROM async_p2 WHERE CURRENT_USER = SESSION_USER);
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ((SELECT * FROM async_p1 WHERE b < 10) UNION ALL (SELECT * FROM async_p2 WHERE b < 10)) s WHERE CURRENT_USER = SESSION_USER;
+
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 95476ada0b..447d908c8a 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1125,6 +1125,13 @@ mark_async_capable_plan(Plan *plan, Path *path)
{
SubqueryScan *scan_plan = (SubqueryScan *) plan;
+ /*
+ * If the generated plan node includes a gating Result node,
+ * we can't execute it asynchronously.
+ */
+ if (IsA(plan, Result))
+ return false;
+
/*
* If a SubqueryScan node atop of an async-capable plan node
* is deletable, consider it as async-capable.
@@ -1139,6 +1146,13 @@ mark_async_capable_plan(Plan *plan, Path *path)
{
FdwRoutine *fdwroutine = path->parent->fdwroutine;
+ /*
+ * If the generated plan node includes a gating Result node,
+ * we can't execute it asynchronously.
+ */
+ if (IsA(plan, Result))
+ return false;
+
Assert(fdwroutine != NULL);
if (fdwroutine->IsForeignPathAsyncCapable != NULL &&
fdwroutine->IsForeignPathAsyncCapable((ForeignPath *) path))
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-10 10:58 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Zhihong Yu <zyu@yugabyte.com>
0 siblings, 0 replies; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-10 10:58 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Hi,
On Sat, Apr 9, 2022 at 1:24 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> On Fri, Apr 8, 2022 at 5:43 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>> This patch seems to be causing the planner to crash.
>> Here's a query reduced from sqlsmith.
>>
>> | explain SELECT 1 FROM information_schema.constraint_column_usage WHERE 1 <= pg_trigger_depth();
>>
>> Program terminated with signal SIGSEGV, Segmentation fault.
> I logged the value of plan->scanstatus before the assertion :
>
> 2022-04-08 16:20:59.601 UTC [26325] LOG: scan status 0
> 2022-04-08 16:20:59.601 UTC [26325] STATEMENT: explain SELECT 1 FROM information_schema.constraint_column_usage WHERE 1 <= pg_trigger_depth();
> 2022-04-08 16:20:59.796 UTC [26296] LOG: server process (PID 26325) was terminated by signal 11: Segmentation fault
>
> It seems its value was SUBQUERY_SCAN_UNKNOWN.
>
> Still trying to find out the cause for the crash.
I think the cause is an oversight in mark_async_capable_plan(). See [1].
Thanks!
Best regards,
Etsuro Fujita
[1] https://www.postgresql.org/message-id/CAPmGK15NkuaVo0Fu_0TfoCpPPJaJi4OMLzEQtkE6Bt6YT52fPQ%40mail.gma...
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-10 13:46 Zhihong Yu <zyu@yugabyte.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
1 sibling, 0 replies; 33+ messages in thread
From: Zhihong Yu @ 2022-04-10 13:46 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Sun, Apr 10, 2022 at 3:42 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:
> On Sat, Apr 9, 2022 at 1:58 AM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> > On Fri, Apr 8, 2022 at 9:43 PM Justin Pryzby <pryzby@telsasoft.com>
> wrote:
> > > This patch seems to be causing the planner to crash.
> > > Here's a query reduced from sqlsmith.
> > >
> > > | explain SELECT 1 FROM information_schema.constraint_column_usage
> WHERE 1 <= pg_trigger_depth();
> > >
> > > Program terminated with signal SIGSEGV, Segmentation fault.
> >
> > Reproduced. Will look into this.
>
> I think the cause of this is that mark_async_capable_plan() failed to
> take into account that when the given path is a SubqueryScanPath or
> ForeignPath, the given corresponding plan might include a gating
> Result node that evaluates pseudoconstant quals. My oversight. :-(
> Attached is a patch for fixing that. I think v14 has the same issue,
> so I think we need backpatching.
>
> Best regards,
> Etsuro Fujita
>
Hi,
Looking at the second hunk of the patch:
FdwRoutine *fdwroutine = path->parent->fdwroutine;
...
+ if (IsA(plan, Result))
+ return false;
It seems the check of whether plan is a Result node can be lifted ahead of
the switch statement (i.e. to the beginning of mark_async_capable_plan).
This way, we don't have to check for every case in the switch statement.
Cheers
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-11 02:41 Justin Pryzby <pryzby@telsasoft.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
1 sibling, 1 reply; 33+ messages in thread
From: Justin Pryzby @ 2022-04-11 02:41 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; Zhihong Yu <zyu@yugabyte.com>; +Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-hackers@lists.postgresql.org
On Sun, Apr 10, 2022 at 07:43:48PM +0900, Etsuro Fujita wrote:
> On Sat, Apr 9, 2022 at 1:58 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > On Fri, Apr 8, 2022 at 9:43 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > This patch seems to be causing the planner to crash.
> > > Here's a query reduced from sqlsmith.
> > >
> > > | explain SELECT 1 FROM information_schema.constraint_column_usage WHERE 1 <= pg_trigger_depth();
> > >
> > > Program terminated with signal SIGSEGV, Segmentation fault.
> >
> > Reproduced. Will look into this.
>
> I think the cause of this is that mark_async_capable_plan() failed to
> take into account that when the given path is a SubqueryScanPath or
> ForeignPath, the given corresponding plan might include a gating
> Result node that evaluates pseudoconstant quals. My oversight. :-(
> Attached is a patch for fixing that. I think v14 has the same issue,
> so I think we need backpatching.
Thanks - this seems to resolve the issue.
On Sun, Apr 10, 2022 at 06:46:25AM -0700, Zhihong Yu wrote:
> Looking at the second hunk of the patch:
> FdwRoutine *fdwroutine = path->parent->fdwroutine;
> ...
> + if (IsA(plan, Result))
> + return false;
>
> It seems the check of whether plan is a Result node can be lifted ahead of
> the switch statement (i.e. to the beginning of mark_async_capable_plan).
>
> This way, we don't have to check for every case in the switch statement.
I think you misread it - the other branch says: if (*not* IsA())
--
Justin
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-11 02:48 Zhihong Yu <zyu@yugabyte.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 33+ messages in thread
From: Zhihong Yu @ 2022-04-11 02:48 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Etsuro Fujita <etsuro.fujita@gmail.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Sun, Apr 10, 2022 at 7:41 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Sun, Apr 10, 2022 at 07:43:48PM +0900, Etsuro Fujita wrote:
> > On Sat, Apr 9, 2022 at 1:58 AM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> > > On Fri, Apr 8, 2022 at 9:43 PM Justin Pryzby <pryzby@telsasoft.com>
> wrote:
> > > > This patch seems to be causing the planner to crash.
> > > > Here's a query reduced from sqlsmith.
> > > >
> > > > | explain SELECT 1 FROM information_schema.constraint_column_usage
> WHERE 1 <= pg_trigger_depth();
> > > >
> > > > Program terminated with signal SIGSEGV, Segmentation fault.
> > >
> > > Reproduced. Will look into this.
> >
> > I think the cause of this is that mark_async_capable_plan() failed to
> > take into account that when the given path is a SubqueryScanPath or
> > ForeignPath, the given corresponding plan might include a gating
> > Result node that evaluates pseudoconstant quals. My oversight. :-(
> > Attached is a patch for fixing that. I think v14 has the same issue,
> > so I think we need backpatching.
>
> Thanks - this seems to resolve the issue.
>
> On Sun, Apr 10, 2022 at 06:46:25AM -0700, Zhihong Yu wrote:
> > Looking at the second hunk of the patch:
> > FdwRoutine *fdwroutine = path->parent->fdwroutine;
> > ...
> > + if (IsA(plan, Result))
> > + return false;
> >
> > It seems the check of whether plan is a Result node can be lifted ahead
> of
> > the switch statement (i.e. to the beginning of mark_async_capable_plan).
> >
> > This way, we don't have to check for every case in the switch statement.
>
> I think you misread it - the other branch says: if (*not* IsA())
>
> No, I didn't misread:
if (!IsA(plan, Result) &&
mark_async_capable_plan(plan,
((ProjectionPath *) path)->subpath))
return true;
return false;
If the plan is Result node, false would be returned.
So the check can be lifted to the beginning of the func.
Cheers
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-17 08:49 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Zhihong Yu <zyu@yugabyte.com>
0 siblings, 1 reply; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-17 08:49 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Mon, Apr 11, 2022 at 11:44 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> On Sun, Apr 10, 2022 at 7:41 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>> On Sun, Apr 10, 2022 at 06:46:25AM -0700, Zhihong Yu wrote:
>> > Looking at the second hunk of the patch:
>> > FdwRoutine *fdwroutine = path->parent->fdwroutine;
>> > ...
>> > + if (IsA(plan, Result))
>> > + return false;
>> >
>> > It seems the check of whether plan is a Result node can be lifted ahead of
>> > the switch statement (i.e. to the beginning of mark_async_capable_plan).
>> >
>> > This way, we don't have to check for every case in the switch statement.
>>
>> I think you misread it - the other branch says: if (*not* IsA())
>>
> No, I didn't misread:
>
> if (!IsA(plan, Result) &&
> mark_async_capable_plan(plan,
> ((ProjectionPath *) path)->subpath))
> return true;
> return false;
>
> If the plan is Result node, false would be returned.
> So the check can be lifted to the beginning of the func.
I think we might support more cases in the switch statement in the
future. My concern about your proposal is that it might make it hard
to add new cases to the statement. I agree that what I proposed has a
bit of redundant code, but writing code inside each case independently
would make it easy to add them, making code consistent across branches
and thus making back-patching easy.
Thanks for reviewing!
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-17 10:34 Zhihong Yu <zyu@yugabyte.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 33+ messages in thread
From: Zhihong Yu @ 2022-04-17 10:34 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Sun, Apr 17, 2022 at 1:48 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:
> On Mon, Apr 11, 2022 at 11:44 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> > On Sun, Apr 10, 2022 at 7:41 PM Justin Pryzby <pryzby@telsasoft.com>
> wrote:
> >> On Sun, Apr 10, 2022 at 06:46:25AM -0700, Zhihong Yu wrote:
> >> > Looking at the second hunk of the patch:
> >> > FdwRoutine *fdwroutine = path->parent->fdwroutine;
> >> > ...
> >> > + if (IsA(plan, Result))
> >> > + return false;
> >> >
> >> > It seems the check of whether plan is a Result node can be lifted
> ahead of
> >> > the switch statement (i.e. to the beginning of
> mark_async_capable_plan).
> >> >
> >> > This way, we don't have to check for every case in the switch
> statement.
> >>
> >> I think you misread it - the other branch says: if (*not* IsA())
> >>
> > No, I didn't misread:
> >
> > if (!IsA(plan, Result) &&
> > mark_async_capable_plan(plan,
> > ((ProjectionPath *)
> path)->subpath))
> > return true;
> > return false;
> >
> > If the plan is Result node, false would be returned.
> > So the check can be lifted to the beginning of the func.
>
> I think we might support more cases in the switch statement in the
> future. My concern about your proposal is that it might make it hard
> to add new cases to the statement. I agree that what I proposed has a
> bit of redundant code, but writing code inside each case independently
> would make it easy to add them, making code consistent across branches
> and thus making back-patching easy.
>
> Thanks for reviewing!
>
> Best regards,
> Etsuro Fujita
>
Hi,
When a new case arises where the plan is not a Result node, this func can
be rewritten.
If there is only one such new case, the check at the beginning of the func
can be tuned to exclude that case.
I still think the check should be lifted to the beginning of the func
(given the current cases).
Cheers
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-19 09:01 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Zhihong Yu <zyu@yugabyte.com>
0 siblings, 1 reply; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-19 09:01 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Hi,
On Sun, Apr 17, 2022 at 7:30 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> On Sun, Apr 17, 2022 at 1:48 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> I think we might support more cases in the switch statement in the
>> future. My concern about your proposal is that it might make it hard
>> to add new cases to the statement. I agree that what I proposed has a
>> bit of redundant code, but writing code inside each case independently
>> would make it easy to add them, making code consistent across branches
>> and thus making back-patching easy.
> When a new case arises where the plan is not a Result node, this func can be rewritten.
> If there is only one such new case, the check at the beginning of the func can be tuned to exclude that case.
Sorry, I don't agree with you.
> I still think the check should be lifted to the beginning of the func (given the current cases).
The given path isn't limited to SubqueryScanPath, ForeignPath and
ProjectionPath, so another concern is extra cycles needed when the
path is other path type that is projection-capable (e.g., Path for
sequential scan, IndexPath, NestPath, ...). Assume that the given
path is a Path (that doesn't contain pseudoconstant quals). In that
case the given SeqScan plan node wouldn't contain a gating Result
node, so if we put the if test at the top of the function, we need to
execute not only the test but the switch statement for the given
path/plan nodes. But if we put the if test inside each case block, we
only need to execute the switch statement, without executing the test.
In the latter case I think we can save cycles for normal cases.
In short: I don't think it's a great idea to put the if test at the
top of the function.
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-19 17:08 Zhihong Yu <zyu@yugabyte.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 33+ messages in thread
From: Zhihong Yu @ 2022-04-19 17:08 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Tue, Apr 19, 2022 at 2:01 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:
> Hi,
>
> On Sun, Apr 17, 2022 at 7:30 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> > On Sun, Apr 17, 2022 at 1:48 AM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> >> I think we might support more cases in the switch statement in the
> >> future. My concern about your proposal is that it might make it hard
> >> to add new cases to the statement. I agree that what I proposed has a
> >> bit of redundant code, but writing code inside each case independently
> >> would make it easy to add them, making code consistent across branches
> >> and thus making back-patching easy.
>
> > When a new case arises where the plan is not a Result node, this func
> can be rewritten.
> > If there is only one such new case, the check at the beginning of the
> func can be tuned to exclude that case.
>
> Sorry, I don't agree with you.
>
> > I still think the check should be lifted to the beginning of the func
> (given the current cases).
>
> The given path isn't limited to SubqueryScanPath, ForeignPath and
> ProjectionPath, so another concern is extra cycles needed when the
> path is other path type that is projection-capable (e.g., Path for
> sequential scan, IndexPath, NestPath, ...). Assume that the given
> path is a Path (that doesn't contain pseudoconstant quals). In that
> case the given SeqScan plan node wouldn't contain a gating Result
> node, so if we put the if test at the top of the function, we need to
> execute not only the test but the switch statement for the given
> path/plan nodes. But if we put the if test inside each case block, we
> only need to execute the switch statement, without executing the test.
> In the latter case I think we can save cycles for normal cases.
>
> In short: I don't think it's a great idea to put the if test at the
> top of the function.
>
> Best regards,
> Etsuro Fujita
>
Hi,
It is okay to keep the formation in your patch.
Cheers
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-25 04:29 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Zhihong Yu <zyu@yugabyte.com>
0 siblings, 1 reply; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-25 04:29 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Hi,
On Wed, Apr 20, 2022 at 2:04 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> It is okay to keep the formation in your patch.
I modified mark_async_capable_plan() a bit further; 1) adjusted code
in the ProjectionPath case, just for consistency with other cases, and
2) tweaked/improved comments a bit. Attached is a new version of the
patch (“prevent-async-2.patch”).
As mentioned before, v14 has the same issue, so I created a fix for
v14, which I’m attaching as well (“prevent-async-2-v14.patch”). In
the fix I modified is_async_capable_path() the same way as
mark_async_capable_plan() in HEAD, renaming it to
is_async_capable_plan(), and updated some comments.
Barring objections, I’ll push/back-patch these.
Thanks!
Best regards,
Etsuro Fujita
Attachments:
[application/octet-stream] prevent-async-2.patch (6.7K, ../../CAPmGK16EKa_v_viZ5yqbbMSU00=GUgX2PYyqDZKgmagwaY6i7Q@mail.gmail.com/2-prevent-async-2.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 477de09a87..3ebe7df89f 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10734,6 +10734,72 @@ SELECT * FROM result_tbl ORDER BY a;
(12 rows)
DELETE FROM result_tbl;
+-- Prevent async execution if we use gating Result nodes for pseudoconstant
+-- quals
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM async_pt WHERE CURRENT_USER = SESSION_USER;
+ QUERY PLAN
+----------------------------------------------------------------
+ Append
+ -> Result
+ Output: async_pt_1.a, async_pt_1.b, async_pt_1.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p1 async_pt_1
+ Output: async_pt_1.a, async_pt_1.b, async_pt_1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1
+ -> Result
+ Output: async_pt_2.a, async_pt_2.b, async_pt_2.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p2 async_pt_2
+ Output: async_pt_2.a, async_pt_2.b, async_pt_2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2
+ -> Result
+ Output: async_pt_3.a, async_pt_3.b, async_pt_3.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Seq Scan on public.async_p3 async_pt_3
+ Output: async_pt_3.a, async_pt_3.b, async_pt_3.c
+(18 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF)
+(SELECT * FROM async_p1 WHERE CURRENT_USER = SESSION_USER)
+UNION ALL
+(SELECT * FROM async_p2 WHERE CURRENT_USER = SESSION_USER);
+ QUERY PLAN
+----------------------------------------------------------------
+ Append
+ -> Result
+ Output: async_p1.a, async_p1.b, async_p1.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1
+ -> Result
+ Output: async_p2.a, async_p2.b, async_p2.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2
+(13 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ((SELECT * FROM async_p1 WHERE b < 10) UNION ALL (SELECT * FROM async_p2 WHERE b < 10)) s WHERE CURRENT_USER = SESSION_USER;
+ QUERY PLAN
+---------------------------------------------------------------------------------
+ Append
+ -> Result
+ Output: async_p1.a, async_p1.b, async_p1.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p1
+ Output: async_p1.a, async_p1.b, async_p1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1 WHERE ((b < 10))
+ -> Result
+ Output: async_p2.a, async_p2.b, async_p2.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p2
+ Output: async_p2.a, async_p2.b, async_p2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2 WHERE ((b < 10))
+(13 rows)
+
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index ed181dedff..310ac72788 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3410,6 +3410,19 @@ UNION ALL
SELECT * FROM result_tbl ORDER BY a;
DELETE FROM result_tbl;
+-- Prevent async execution if we use gating Result nodes for pseudoconstant
+-- quals
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM async_pt WHERE CURRENT_USER = SESSION_USER;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+(SELECT * FROM async_p1 WHERE CURRENT_USER = SESSION_USER)
+UNION ALL
+(SELECT * FROM async_p2 WHERE CURRENT_USER = SESSION_USER);
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ((SELECT * FROM async_p1 WHERE b < 10) UNION ALL (SELECT * FROM async_p2 WHERE b < 10)) s WHERE CURRENT_USER = SESSION_USER;
+
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 7905bc4654..db11936efe 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1112,9 +1112,9 @@ create_join_plan(PlannerInfo *root, JoinPath *best_path)
/*
* mark_async_capable_plan
- * Check whether a given Path node is async-capable, and if so, mark the
- * Plan node created from it as such and return true, otherwise return
- * false.
+ * Check whether the Plan node created from a Path node is async-capable,
+ * and if so, mark the Plan node as such and return true, otherwise
+ * return false.
*/
static bool
mark_async_capable_plan(Plan *plan, Path *path)
@@ -1125,6 +1125,13 @@ mark_async_capable_plan(Plan *plan, Path *path)
{
SubqueryScan *scan_plan = (SubqueryScan *) plan;
+ /*
+ * If the generated plan node includes a gating Result node,
+ * we can't execute it asynchronously.
+ */
+ if (IsA(plan, Result))
+ return false;
+
/*
* If a SubqueryScan node atop of an async-capable plan node
* is deletable, consider it as async-capable.
@@ -1139,6 +1146,13 @@ mark_async_capable_plan(Plan *plan, Path *path)
{
FdwRoutine *fdwroutine = path->parent->fdwroutine;
+ /*
+ * If the generated plan node includes a gating Result node,
+ * we can't execute it asynchronously.
+ */
+ if (IsA(plan, Result))
+ return false;
+
Assert(fdwroutine != NULL);
if (fdwroutine->IsForeignPathAsyncCapable != NULL &&
fdwroutine->IsForeignPathAsyncCapable((ForeignPath *) path))
@@ -1148,11 +1162,17 @@ mark_async_capable_plan(Plan *plan, Path *path)
case T_ProjectionPath:
/*
- * If the generated plan node doesn't include a Result node,
- * consider it as async-capable if the subpath is async-capable.
+ * If the generated plan node includes a Result node for
+ * the projection, we can't execute it asynchronously.
+ */
+ if (IsA(plan, Result))
+ return false;
+
+ /*
+ * create_projection_plan() would have pulled up the subplan, so
+ * check the capability using the subpath.
*/
- if (!IsA(plan, Result) &&
- mark_async_capable_plan(plan,
+ if (mark_async_capable_plan(plan,
((ProjectionPath *) path)->subpath))
return true;
return false;
[application/octet-stream] prevent-async-2-v14.patch (4.3K, ../../CAPmGK16EKa_v_viZ5yqbbMSU00=GUgX2PYyqDZKgmagwaY6i7Q@mail.gmail.com/3-prevent-async-2-v14.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 8bcc18eca6..e22cc871f3 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10424,6 +10424,32 @@ DROP TABLE local_tbl;
DROP INDEX base_tbl1_idx;
DROP INDEX base_tbl2_idx;
DROP INDEX async_p3_idx;
+-- Prevent async execution if we use gating Result nodes for pseudoconstant
+-- quals
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM async_pt WHERE CURRENT_USER = SESSION_USER;
+ QUERY PLAN
+----------------------------------------------------------------
+ Append
+ -> Result
+ Output: async_pt_1.a, async_pt_1.b, async_pt_1.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p1 async_pt_1
+ Output: async_pt_1.a, async_pt_1.b, async_pt_1.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl1
+ -> Result
+ Output: async_pt_2.a, async_pt_2.b, async_pt_2.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Foreign Scan on public.async_p2 async_pt_2
+ Output: async_pt_2.a, async_pt_2.b, async_pt_2.c
+ Remote SQL: SELECT a, b, c FROM public.base_tbl2
+ -> Result
+ Output: async_pt_3.a, async_pt_3.b, async_pt_3.c
+ One-Time Filter: (CURRENT_USER = SESSION_USER)
+ -> Seq Scan on public.async_p3 async_pt_3
+ Output: async_pt_3.a, async_pt_3.b, async_pt_3.c
+(18 rows)
+
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 855c7ea70e..fede8de1fb 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3317,6 +3317,11 @@ DROP INDEX base_tbl1_idx;
DROP INDEX base_tbl2_idx;
DROP INDEX async_p3_idx;
+-- Prevent async execution if we use gating Result nodes for pseudoconstant
+-- quals
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM async_pt WHERE CURRENT_USER = SESSION_USER;
+
-- Test that pending requests are processed properly
SET enable_mergejoin TO false;
SET enable_hashjoin TO false;
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index a2101fb3fc..0ed858f305 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -82,7 +82,7 @@ static List *get_gating_quals(PlannerInfo *root, List *quals);
static Plan *create_gating_plan(PlannerInfo *root, Path *path, Plan *plan,
List *gating_quals);
static Plan *create_join_plan(PlannerInfo *root, JoinPath *best_path);
-static bool is_async_capable_path(Path *path);
+static bool is_async_capable_plan(Plan *plan, Path *path);
static Plan *create_append_plan(PlannerInfo *root, AppendPath *best_path,
int flags);
static Plan *create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path,
@@ -1109,11 +1109,11 @@ create_join_plan(PlannerInfo *root, JoinPath *best_path)
}
/*
- * is_async_capable_path
- * Check whether a given Path node is async-capable.
+ * is_async_capable_plan
+ * Check whether the Plan node created from a Path node is async-capable.
*/
static bool
-is_async_capable_path(Path *path)
+is_async_capable_plan(Plan *plan, Path *path)
{
switch (nodeTag(path))
{
@@ -1121,6 +1121,13 @@ is_async_capable_path(Path *path)
{
FdwRoutine *fdwroutine = path->parent->fdwroutine;
+ /*
+ * If the generated plan node includes a gating Result node,
+ * we can't execute it asynchronously.
+ */
+ if (IsA(plan, Result))
+ return false;
+
Assert(fdwroutine != NULL);
if (fdwroutine->IsForeignPathAsyncCapable != NULL &&
fdwroutine->IsForeignPathAsyncCapable((ForeignPath *) path))
@@ -1295,8 +1302,8 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path, int flags)
subplans = lappend(subplans, subplan);
- /* Check to see if subplan can be executed asynchronously */
- if (consider_async && is_async_capable_path(subpath))
+ /* If needed, check to see if subplan can be executed asynchronously */
+ if (consider_async && is_async_capable_plan(subplan, subpath))
{
subplan->async_capable = true;
++nasyncplans;
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-04-28 06:32 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 0 replies; 33+ messages in thread
From: Etsuro Fujita @ 2022-04-28 06:32 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Mon, Apr 25, 2022 at 1:29 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> I modified mark_async_capable_plan() a bit further; 1) adjusted code
> in the ProjectionPath case, just for consistency with other cases, and
> 2) tweaked/improved comments a bit. Attached is a new version of the
> patch (“prevent-async-2.patch”).
>
> As mentioned before, v14 has the same issue, so I created a fix for
> v14, which I’m attaching as well (“prevent-async-2-v14.patch”). In
> the fix I modified is_async_capable_path() the same way as
> mark_async_capable_plan() in HEAD, renaming it to
> is_async_capable_plan(), and updated some comments.
>
> Barring objections, I’ll push/back-patch these.
Done.
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-06-02 12:08 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
1 sibling, 1 reply; 33+ messages in thread
From: Etsuro Fujita @ 2022-06-02 12:08 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Apr 6, 2022 at 3:58 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> I have committed the patch after modifying it as such.
The patch calls trivial_subqueryscan() during create_append_plan() to
determine the triviality of a SubqueryScan that is a child of an
Append node. Unlike when calling it from
set_subqueryscan_references(), this is done before some
post-processing such as set_plan_references() on the subquery. The
reason why this is safe wouldn't be that obvious, so I added to
trivial_subqueryscan() comments explaining this. Attached is a patch
for that.
Best regards,
Etsuro Fujita
Attachments:
[application/octet-stream] Improve-comments-for-trivial_subqueryscan.patch (2.0K, ../../CAPmGK17+GiJBthC6va7+9n6t75e-M1N0U18YB2G1B+E5OdrNTA@mail.gmail.com/2-Improve-comments-for-trivial_subqueryscan.patch)
download | inline diff:
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index d95fd89807..5108dbaf81 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1349,8 +1349,22 @@ set_subqueryscan_references(PlannerInfo *root,
* We can delete it if it has no qual to check and the targetlist just
* regurgitates the output of the child plan.
*
- * This might be called repeatedly on a SubqueryScan node, so we cache the
- * result in the SubqueryScan node to avoid repeated computation.
+ * This can be called from mark_async_capable_plan(), a helper function for
+ * create_append_plan(), before set_subqueryscan_references(), to determine
+ * triviality of a SubqueryScan that is a child of an Append node. So we
+ * cache the result in the SubqueryScan node to avoid repeated computation.
+ *
+ * Note: when called from mark_async_capable_plan(), we determine the result
+ * before running finalize_plan() on the SubqueryScan node (if needed) and
+ * set_plan_references() on the subplan tree, but this would be safe because
+ * 1) finalize_plan() doesn't modify the tlist or quals for the SubqueryScan
+ * node (or that for any plan node in the subplan tree), 2)
+ * set_plan_references() modifies the tlist for every plan node in the
+ * subplan tree, but keeps const/resjunk columns as const/resjunk ones and
+ * preserves the length and order of the tlist, and 3) set_plan_references()
+ * might delete the topmost plan node like an Append or MergeAppend from the
+ * subplan tree and pull up the child plan node, but in that case, the tlist
+ * for the child plan node exactly matches the parent.
*/
bool
trivial_subqueryscan(SubqueryScan *plan)
@@ -1359,7 +1373,7 @@ trivial_subqueryscan(SubqueryScan *plan)
ListCell *lp,
*lc;
- /* We might have detected this already (see mark_async_capable_plan) */
+ /* We might have detected this already; in which case reuse the result */
if (plan->scanstatus == SUBQUERY_SCAN_TRIVIAL)
return true;
if (plan->scanstatus == SUBQUERY_SCAN_NONTRIVIAL)
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-06-02 16:09 Zhihong Yu <zyu@yugabyte.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 33+ messages in thread
From: Zhihong Yu @ 2022-06-02 16:09 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Jun 2, 2022 at 5:08 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:
> On Wed, Apr 6, 2022 at 3:58 PM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> > I have committed the patch after modifying it as such.
>
> The patch calls trivial_subqueryscan() during create_append_plan() to
> determine the triviality of a SubqueryScan that is a child of an
> Append node. Unlike when calling it from
> set_subqueryscan_references(), this is done before some
> post-processing such as set_plan_references() on the subquery. The
> reason why this is safe wouldn't be that obvious, so I added to
> trivial_subqueryscan() comments explaining this. Attached is a patch
> for that.
>
> Best regards,
> Etsuro Fujita
>
Hi,
Suggestion on formatting the comment:
+ * node (or that for any plan node in the subplan tree), 2)
+ * set_plan_references() modifies the tlist for every plan node in the
It would be more readable if `2)` is put at the beginning of the second
line above.
+ * preserves the length and order of the tlist, and 3)
set_plan_references()
+ * might delete the topmost plan node like an Append or MergeAppend from
the
Similarly you can move `3) set_plan_references()` to the beginning of the
next line.
Cheers
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-06-08 10:18 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Zhihong Yu <zyu@yugabyte.com>
0 siblings, 1 reply; 33+ messages in thread
From: Etsuro Fujita @ 2022-06-08 10:18 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri, Jun 3, 2022 at 1:03 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> Suggestion on formatting the comment:
>
> + * node (or that for any plan node in the subplan tree), 2)
> + * set_plan_references() modifies the tlist for every plan node in the
>
> It would be more readable if `2)` is put at the beginning of the second line above.
>
> + * preserves the length and order of the tlist, and 3) set_plan_references()
> + * might delete the topmost plan node like an Append or MergeAppend from the
>
> Similarly you can move `3) set_plan_references()` to the beginning of the next line.
Seems like a good idea, so I updated the patch as you suggest. I did
some indentation as well, which I think improves readability a bit
further. Attached is an updated version. If no objections, I’ll
commit the patch.
Thanks for reviewing!
Best regards,
Etsuro Fujita
Attachments:
[application/octet-stream] Improve-comments-for-trivial_subqueryscan-2.patch (2.1K, ../../CAPmGK16KtNZeTnRa1u16Jq00LDFPzRYDKKDtRJPLYaG3Oy0m9A@mail.gmail.com/2-Improve-comments-for-trivial_subqueryscan-2.patch)
download | inline diff:
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index d95fd89807..9cef92cab2 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1349,8 +1349,23 @@ set_subqueryscan_references(PlannerInfo *root,
* We can delete it if it has no qual to check and the targetlist just
* regurgitates the output of the child plan.
*
- * This might be called repeatedly on a SubqueryScan node, so we cache the
- * result in the SubqueryScan node to avoid repeated computation.
+ * This can be called from mark_async_capable_plan(), a helper function for
+ * create_append_plan(), before set_subqueryscan_references(), to determine
+ * triviality of a SubqueryScan that is a child of an Append node. So we
+ * cache the result in the SubqueryScan node to avoid repeated computation.
+ *
+ * Note: when called from mark_async_capable_plan(), we determine the result
+ * before running finalize_plan() on the SubqueryScan node (if needed) and
+ * set_plan_references() on the subplan tree, but this would be safe, because
+ * 1) finalize_plan() doesn't modify the tlist or quals for the SubqueryScan
+ * node (or that for any plan node in the subplan tree), and
+ * 2) set_plan_references() modifies the tlist for every plan node in the
+ * subplan tree, but keeps const/resjunk columns as const/resjunk ones and
+ * preserves the length and order of the tlist, and
+ * 3) set_plan_references() might delete the topmost plan node like an Append
+ * or MergeAppend from the subplan tree and pull up the child plan node,
+ * but in that case, the tlist for the child plan node exactly matches the
+ * parent.
*/
bool
trivial_subqueryscan(SubqueryScan *plan)
@@ -1359,7 +1374,7 @@ trivial_subqueryscan(SubqueryScan *plan)
ListCell *lp,
*lc;
- /* We might have detected this already (see mark_async_capable_plan) */
+ /* We might have detected this already; in which case reuse the result */
if (plan->scanstatus == SUBQUERY_SCAN_TRIVIAL)
return true;
if (plan->scanstatus == SUBQUERY_SCAN_NONTRIVIAL)
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Defer selection of asynchronous subplans until the executor initialization stage
@ 2022-06-09 10:39 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 0 replies; 33+ messages in thread
From: Etsuro Fujita @ 2022-06-09 10:39 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Jun 8, 2022 at 7:18 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Fri, Jun 3, 2022 at 1:03 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> > Suggestion on formatting the comment:
> >
> > + * node (or that for any plan node in the subplan tree), 2)
> > + * set_plan_references() modifies the tlist for every plan node in the
> >
> > It would be more readable if `2)` is put at the beginning of the second line above.
> >
> > + * preserves the length and order of the tlist, and 3) set_plan_references()
> > + * might delete the topmost plan node like an Append or MergeAppend from the
> >
> > Similarly you can move `3) set_plan_references()` to the beginning of the next line.
>
> Seems like a good idea, so I updated the patch as you suggest. I did
> some indentation as well, which I think improves readability a bit
> further. Attached is an updated version. If no objections, I’ll
> commit the patch.
Done.
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 33+ messages in thread
end of thread, other threads:[~2022-06-09 10:39 UTC | newest]
Thread overview: 33+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-05-11 03:45 Defer selection of asynchronous subplans until the executor initialization stage Andrey Lepikhov <a.lepikhov@postgrespro.ru>
2021-05-11 03:55 ` Zhihong Yu <zyu@yugabyte.com>
2021-05-11 07:06 ` Andrey Lepikhov <a.lepikhov@postgrespro.ru>
2021-06-30 04:50 ` Andrey Lepikhov <a.lepikhov@postgrespro.ru>
2021-08-23 09:18 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2021-08-30 08:36 ` Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>
2021-08-30 09:52 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2021-09-15 06:40 ` Alexander Pyhalov <a.pyhalov@postgrespro.ru>
2022-03-13 09:39 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-03 10:29 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-03 14:42 ` Zhihong Yu <zyu@yugabyte.com>
2022-04-04 04:06 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-06 06:58 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-08 12:43 ` Justin Pryzby <pryzby@telsasoft.com>
2022-04-08 16:28 ` Zhihong Yu <zyu@yugabyte.com>
2022-04-10 10:58 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-08 16:58 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-10 10:43 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-10 13:46 ` Zhihong Yu <zyu@yugabyte.com>
2022-04-11 02:41 ` Justin Pryzby <pryzby@telsasoft.com>
2022-04-11 02:48 ` Zhihong Yu <zyu@yugabyte.com>
2022-04-17 08:49 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-17 10:34 ` Zhihong Yu <zyu@yugabyte.com>
2022-04-19 09:01 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-19 17:08 ` Zhihong Yu <zyu@yugabyte.com>
2022-04-25 04:29 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-28 06:32 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-06-02 12:08 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-06-02 16:09 ` Zhihong Yu <zyu@yugabyte.com>
2022-06-08 10:18 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-06-09 10:39 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2022-04-04 09:30 ` Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>
2022-04-06 07:05 ` Etsuro Fujita <etsuro.fujita@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox