planstate_tree_walker oversight CustomScan

Started by Kouhei Kaigaiover 10 years ago4 messages

kaigai@ak.jp.nec.com

over 10 years ago

1 attachment(s)

Hi,

The planstate_tree_walker() oversight custom_ps of CustomScanState;
that should be a list of underlying PlanState object if any.

ExplainPreScanNode() treated ForeignScan and CustomScan in special
way (it is sufficient for ExplainPreScanNode() purpose), thus, it
didn't implement its recursive portion originally.

The job of ExplainPreScanNode() is know all the relids involved
in a particular subquery execution. On the other hands, fs_relids
of ForeignScan and custom_relids of CustomScan informs a set of
relids to be scanned by this Scan node without recursive, so it
did not have recursive walks on the underlying sub-plans.

However, planstate_tree_walker() will have different expectation.
It is general walker routine, thus, it is natural users to expect
the callback is also kicked towards the underlying planstate of
CustomScan (and ForeignScan; once EPQ recheck gets solved).

The attached patch adds support of CustomScan on the walker.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

Attachments:

pgsql-planstate_tree_walker-oversight-custom-scan.v1.patchapplication/octet-stream; name=pgsql-planstate_tree_walker-oversight-custom-scan.v1.patchDownload

 src/backend/nodes/nodeFuncs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 4a24474..a11cb9f 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -3428,6 +3428,7 @@ bool
 planstate_tree_walker(PlanState *planstate, bool (*walker) (), void *context)
 {
 	Plan	   *plan = planstate->plan;
+	ListCell   *lc;
 
 	/* initPlan-s */
 	if (planstate_walk_subplans(planstate->initPlan, walker, context))
@@ -3484,6 +3485,13 @@ planstate_tree_walker(PlanState *planstate, bool (*walker) (), void *context)
 			if (walker(((SubqueryScanState *) planstate)->subplan, context))
 				return true;
 			break;
+		case T_CustomScan:
+			foreach (lc, ((CustomScanState *) planstate)->custom_ps)
+			{
+				if (walker((PlanState *) lfirst(lc), context))
+					return true;
+			}
+			break;
 		default:
 			break;
 	}

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Kouhei Kaigai (#1)

Re: planstate_tree_walker oversight CustomScan

On Mon, Sep 21, 2015 at 9:54 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

The planstate_tree_walker() oversight custom_ps of CustomScanState;
that should be a list of underlying PlanState object if any.

ExplainPreScanNode() treated ForeignScan and CustomScan in special
way (it is sufficient for ExplainPreScanNode() purpose), thus, it
didn't implement its recursive portion originally.

The job of ExplainPreScanNode() is know all the relids involved
in a particular subquery execution. On the other hands, fs_relids
of ForeignScan and custom_relids of CustomScan informs a set of
relids to be scanned by this Scan node without recursive, so it
did not have recursive walks on the underlying sub-plans.

However, planstate_tree_walker() will have different expectation.
It is general walker routine, thus, it is natural users to expect
the callback is also kicked towards the underlying planstate of
CustomScan (and ForeignScan; once EPQ recheck gets solved).

The attached patch adds support of CustomScan on the walker.

Do you need to add something to ExplainPreScanNode for this as well,
like if (scanrelid != 0) *rels_used = bms_add_member(*rels_used,
scanrelid)?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Kouhei Kaigai

kaigai@ak.jp.nec.com

over 10 years ago

In reply to: Robert Haas (#2)

Re: planstate_tree_walker oversight CustomScan

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas
Sent: Wednesday, September 23, 2015 10:15 AM
To: Kaigai Kouhei(海外浩平)
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] planstate_tree_walker oversight CustomScan

On Mon, Sep 21, 2015 at 9:54 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

The planstate_tree_walker() oversight custom_ps of CustomScanState;
that should be a list of underlying PlanState object if any.

ExplainPreScanNode() treated ForeignScan and CustomScan in special
way (it is sufficient for ExplainPreScanNode() purpose), thus, it
didn't implement its recursive portion originally.

The job of ExplainPreScanNode() is know all the relids involved
in a particular subquery execution. On the other hands, fs_relids
of ForeignScan and custom_relids of CustomScan informs a set of
relids to be scanned by this Scan node without recursive, so it
did not have recursive walks on the underlying sub-plans.

However, planstate_tree_walker() will have different expectation.
It is general walker routine, thus, it is natural users to expect
the callback is also kicked towards the underlying planstate of
CustomScan (and ForeignScan; once EPQ recheck gets solved).

The attached patch adds support of CustomScan on the walker.

Do you need to add something to ExplainPreScanNode for this as well,
like if (scanrelid != 0) *rels_used = bms_add_member(*rels_used,
scanrelid)?

The latest ExplainPreScanNode is sufficient. Regardless of scanrelid
(even if it is zero), fs_relids and custom_relids shall be set properly
to introduce which relations are represented by this ForeignScan and
CustomScan node. So, additional planstate_tree_walker() call might be
a bit redundant, but harmless.

The reason why ForeignScan/CustomScan node have these bitmap is, we
cannot guarantee they always have underlying scan node. For example,
ForeignScan that kicks remote join query will not have local underlying
scan node on the foreign tables to be involved.
So, we had to inform ExplainPreScanNode which relids are represented
by this Foreign/CustomScan node.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Kouhei Kaigai (#3)

Re: planstate_tree_walker oversight CustomScan

On Tue, Sep 22, 2015 at 9:30 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

The latest ExplainPreScanNode is sufficient. Regardless of scanrelid
(even if it is zero), fs_relids and custom_relids shall be set properly
to introduce which relations are represented by this ForeignScan and
CustomScan node. So, additional planstate_tree_walker() call might be
a bit redundant, but harmless.

The reason why ForeignScan/CustomScan node have these bitmap is, we
cannot guarantee they always have underlying scan node. For example,
ForeignScan that kicks remote join query will not have local underlying
scan node on the foreign tables to be involved.
So, we had to inform ExplainPreScanNode which relids are represented
by this Foreign/CustomScan node.

OK, I've just committed the patch the way it is for now, then.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers