Use outerPlanState() consistently in executor code

Started by Qingqing Zhouover 10 years ago10 messages
#1Qingqing Zhou
zhouqq.postgres@gmail.com
1 attachment(s)

In executor context, outerPlanState(node) is the same as
node->ss.ps.lefttree. We follow this in most places except a few. This
patch clean up the outliers and might save us a few instructions by
removing indirection.

Most of changes are trivial. Except I take out an outerPlan nullable
check in grouping iterator - as a it surely has a left child.

I noticed that we mixed use "node" for plan node and plan state. While
changing it can make code clear, but back patching could be terrible.

Regards,
Qingqing

Attachments:

nodestate.difftext/plain; charset=UTF-16LE; name=nodestate.diffDownload
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 9ff0eff..672825a 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2054,10 +2054,11 @@ ExecReScanAgg(AggState *node)
 {
 	ExprContext *econtext = node->ss.ps.ps_ExprContext;
 	int			aggno;
+	PlanState	*outerPlan;
 
 	node->agg_done = false;
-
 	node->ss.ps.ps_TupFromTlist = false;
+	outerPlan = outerPlanState(node);
 
 	if (((Agg *) node->ss.ps.plan)->aggstrategy == AGG_HASHED)
 	{
@@ -2075,7 +2076,7 @@ ExecReScanAgg(AggState *node)
 		 * parameter changes, then we can just rescan the existing hash table;
 		 * no need to build it again.
 		 */
-		if (node->ss.ps.lefttree->chgParam == NULL)
+		if (outerPlan->chgParam == NULL)
 		{
 			ResetTupleHashIterator(node->hashtable, &node->hashiter);
 			return;
@@ -2133,8 +2134,8 @@ ExecReScanAgg(AggState *node)
 	 * if chgParam of subnode is not null then plan will be re-scanned by
 	 * first ExecProcNode.
 	 */
-	if (node->ss.ps.lefttree->chgParam == NULL)
-		ExecReScan(node->ss.ps.lefttree);
+	if (outerPlan->chgParam == NULL)
+		ExecReScan(outerPlan);
 }
 
 
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 8ea8b9f..6502841 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -449,6 +449,8 @@ ExecBitmapHeapScan(BitmapHeapScanState *node)
 void
 ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
 {
+	PlanState	*outerPlan;
+
 	/* rescan to release any page pin */
 	heap_rescan(node->ss.ss_currentScanDesc, NULL);
 
@@ -469,8 +471,9 @@ ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
 	 * if chgParam of subnode is not null then plan will be re-scanned by
 	 * first ExecProcNode.
 	 */
-	if (node->ss.ps.lefttree->chgParam == NULL)
-		ExecReScan(node->ss.ps.lefttree);
+	outerPlan = outerPlanState(node);
+	if (outerPlan->chgParam == NULL)
+		ExecReScan(outerPlan);
 }
 
 /* ----------------------------------------------------------------
diff --git a/src/backend/executor/nodeGroup.c b/src/backend/executor/nodeGroup.c
index 83d562e..b0d5442 100644
--- a/src/backend/executor/nodeGroup.c
+++ b/src/backend/executor/nodeGroup.c
@@ -280,6 +280,8 @@ ExecEndGroup(GroupState *node)
 void
 ExecReScanGroup(GroupState *node)
 {
+	PlanState	*outerPlan;
+	
 	node->grp_done = FALSE;
 	node->ss.ps.ps_TupFromTlist = false;
 	/* must clear first tuple */
@@ -289,7 +291,7 @@ ExecReScanGroup(GroupState *node)
 	 * if chgParam of subnode is not null then plan will be re-scanned by
 	 * first ExecProcNode.
 	 */
-	if (node->ss.ps.lefttree &&
-		node->ss.ps.lefttree->chgParam == NULL)
-		ExecReScan(node->ss.ps.lefttree);
+	outerPlan = outerPlanState(node);
+	if (outerPlan->chgParam == NULL)
+		ExecReScan(outerPlan);
 }
diff --git a/src/backend/executor/nodeMaterial.c b/src/backend/executor/nodeMaterial.c
index 1158825..41859e0 100644
--- a/src/backend/executor/nodeMaterial.c
+++ b/src/backend/executor/nodeMaterial.c
@@ -317,6 +317,9 @@ ExecMaterialRestrPos(MaterialState *node)
 void
 ExecReScanMaterial(MaterialState *node)
 {
+	PlanState	*outerPlan;
+
+	outerPlan = outerPlanState(node);
 	ExecClearTuple(node->ss.ps.ps_ResultTupleSlot);
 
 	if (node->eflags != 0)
@@ -339,13 +342,13 @@ ExecReScanMaterial(MaterialState *node)
 		 * Otherwise we can just rewind and rescan the stored output. The
 		 * state of the subnode does not change.
 		 */
-		if (node->ss.ps.lefttree->chgParam != NULL ||
+		if (outerPlan->chgParam != NULL ||
 			(node->eflags & EXEC_FLAG_REWIND) == 0)
 		{
 			tuplestore_end(node->tuplestorestate);
 			node->tuplestorestate = NULL;
-			if (node->ss.ps.lefttree->chgParam == NULL)
-				ExecReScan(node->ss.ps.lefttree);
+			if (outerPlan->chgParam == NULL)
+				ExecReScan(outerPlan);
 			node->eof_underlying = false;
 		}
 		else
@@ -359,8 +362,8 @@ ExecReScanMaterial(MaterialState *node)
 		 * if chgParam of subnode is not null then plan will be re-scanned by
 		 * first ExecProcNode.
 		 */
-		if (node->ss.ps.lefttree->chgParam == NULL)
-			ExecReScan(node->ss.ps.lefttree);
+		if (outerPlan->chgParam == NULL)
+			ExecReScan(outerPlan);
 		node->eof_underlying = false;
 	}
 }
diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
index a815cde..32e064e 100644
--- a/src/backend/executor/nodeSort.c
+++ b/src/backend/executor/nodeSort.c
@@ -290,6 +290,8 @@ ExecSortRestrPos(SortState *node)
 void
 ExecReScanSort(SortState *node)
 {
+	PlanState	*outerPlan;
+	
 	/*
 	 * If we haven't sorted yet, just return. If outerplan's chgParam is not
 	 * NULL then it will be re-scanned by ExecProcNode, else no reason to
@@ -299,6 +301,7 @@ ExecReScanSort(SortState *node)
 		return;
 
 	/* must drop pointer to sort result tuple */
+	outerPlan = outerPlanState(node);
 	ExecClearTuple(node->ss.ps.ps_ResultTupleSlot);
 
 	/*
@@ -308,7 +311,7 @@ ExecReScanSort(SortState *node)
 	 *
 	 * Otherwise we can just rewind and rescan the sorted output.
 	 */
-	if (node->ss.ps.lefttree->chgParam != NULL ||
+	if (outerPlan->chgParam != NULL ||
 		node->bounded != node->bounded_Done ||
 		node->bound != node->bound_Done ||
 		!node->randomAccess)
@@ -321,8 +324,8 @@ ExecReScanSort(SortState *node)
 		 * if chgParam of subnode is not null then plan will be re-scanned by
 		 * first ExecProcNode.
 		 */
-		if (node->ss.ps.lefttree->chgParam == NULL)
-			ExecReScan(node->ss.ps.lefttree);
+		if (outerPlan->chgParam == NULL)
+			ExecReScan(outerPlan);
 	}
 	else
 		tuplesort_rescan((Tuplesortstate *) node->tuplesortstate);
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index a06790d..520be66 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -2057,6 +2057,7 @@ ExecEndWindowAgg(WindowAggState *node)
 void
 ExecReScanWindowAgg(WindowAggState *node)
 {
+	PlanState	*outerPlan;
 	ExprContext *econtext = node->ss.ps.ps_ExprContext;
 
 	node->all_done = false;
@@ -2082,8 +2083,9 @@ ExecReScanWindowAgg(WindowAggState *node)
 	 * if chgParam of subnode is not null then plan will be re-scanned by
 	 * first ExecProcNode.
 	 */
-	if (node->ss.ps.lefttree->chgParam == NULL)
-		ExecReScan(node->ss.ps.lefttree);
+	outerPlan = outerPlanState(node);
+	if (outerPlan->chgParam == NULL)
+		ExecReScan(outerPlan);
 }
 
 /*
#2Robert Haas
robertmhaas@gmail.com
In reply to: Qingqing Zhou (#1)
Re: Use outerPlanState() consistently in executor code

On Wed, Apr 15, 2015 at 3:38 PM, Qingqing Zhou
<zhouqq.postgres@gmail.com> wrote:

In executor context, outerPlanState(node) is the same as
node->ss.ps.lefttree. We follow this in most places except a few. This
patch clean up the outliers and might save us a few instructions by
removing indirection.

Most of changes are trivial. Except I take out an outerPlan nullable
check in grouping iterator - as a it surely has a left child.

I don't see any particular reason not to do this.

The patch is weird, though. If I open it with "less", I get binary
garbage. My Mac's TextEdit app opens it OK though.

--
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

#3Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#2)
Re: Use outerPlanState() consistently in executor code

On Thu, Apr 30, 2015 at 08:46:55AM -0400, Robert Haas wrote:

On Wed, Apr 15, 2015 at 3:38 PM, Qingqing Zhou
<zhouqq.postgres@gmail.com> wrote:

In executor context, outerPlanState(node) is the same as
node->ss.ps.lefttree. We follow this in most places except a few. This
patch clean up the outliers and might save us a few instructions by
removing indirection.

Most of changes are trivial. Except I take out an outerPlan nullable
check in grouping iterator - as a it surely has a left child.

I don't see any particular reason not to do this.

The patch is weird, though. If I open it with "less", I get binary
garbage. My Mac's TextEdit app opens it OK though.

The patch is encoded as utf-16le, and has MSDOS newlines, ^M.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

#4Robert Haas
robertmhaas@gmail.com
In reply to: Bruce Momjian (#3)
Re: Use outerPlanState() consistently in executor code

On Thu, Apr 30, 2015 at 9:02 AM, Bruce Momjian <bruce@momjian.us> wrote:

On Thu, Apr 30, 2015 at 08:46:55AM -0400, Robert Haas wrote:

On Wed, Apr 15, 2015 at 3:38 PM, Qingqing Zhou
<zhouqq.postgres@gmail.com> wrote:

In executor context, outerPlanState(node) is the same as
node->ss.ps.lefttree. We follow this in most places except a few. This
patch clean up the outliers and might save us a few instructions by
removing indirection.

Most of changes are trivial. Except I take out an outerPlan nullable
check in grouping iterator - as a it surely has a left child.

I don't see any particular reason not to do this.

The patch is weird, though. If I open it with "less", I get binary
garbage. My Mac's TextEdit app opens it OK though.

The patch is encoded as utf-16le, and has MSDOS newlines, ^M.

I don't mind the MSDOS newlines, but the UTF-16le bit is inconvenient.
UTF-8 would be much better, so I don't have to figure out how to
convert.

--
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

#5Qingqing Zhou
zhouqq.postgres@gmail.com
In reply to: Robert Haas (#4)
1 attachment(s)
Re: Use outerPlanState() consistently in executor code

On Thu, Apr 30, 2015 at 8:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:

I don't mind the MSDOS newlines, but the UTF-16le bit is inconvenient.
UTF-8 would be much better, so I don't have to figure out how to
convert.

The patch is generated via github windows tool and that's possibly
why. I regenerated it in Linux box and see attached (sending this
email in Windows and I hope no magic happens in-between).

Please let me know if that works.

Thank you,
Qingqing

Attachments:

nodestate_txt.difftext/plain; charset=US-ASCII; name=nodestate_txt.diffDownload
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 9ff0eff..672825a 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2054,10 +2054,11 @@ ExecReScanAgg(AggState *node)
 {
        ExprContext *econtext = node->ss.ps.ps_ExprContext;
        int                     aggno;
+       PlanState       *outerPlan;

        node->agg_done = false;
-
        node->ss.ps.ps_TupFromTlist = false;
+       outerPlan = outerPlanState(node);

        if (((Agg *) node->ss.ps.plan)->aggstrategy == AGG_HASHED)
        {
@@ -2075,7 +2076,7 @@ ExecReScanAgg(AggState *node)
                 * parameter changes, then we can just rescan the existing hash table;
                 * no need to build it again.
                 */
-               if (node->ss.ps.lefttree->chgParam == NULL)
+               if (outerPlan->chgParam == NULL)
                {
                        ResetTupleHashIterator(node->hashtable, &node->hashiter);
                        return;
@@ -2133,8 +2134,8 @@ ExecReScanAgg(AggState *node)
         * if chgParam of subnode is not null then plan will be re-scanned by
         * first ExecProcNode.
         */
-       if (node->ss.ps.lefttree->chgParam == NULL)
-               ExecReScan(node->ss.ps.lefttree);
+       if (outerPlan->chgParam == NULL)
+               ExecReScan(outerPlan);
 }


diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 8ea8b9f..6502841 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -449,6 +449,8 @@ ExecBitmapHeapScan(BitmapHeapScanState *node)
 void
 ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
 {
+       PlanState       *outerPlan;
+
        /* rescan to release any page pin */
        heap_rescan(node->ss.ss_currentScanDesc, NULL);

@@ -469,8 +471,9 @@ ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
         * if chgParam of subnode is not null then plan will be re-scanned by
         * first ExecProcNode.
         */
-       if (node->ss.ps.lefttree->chgParam == NULL)
-               ExecReScan(node->ss.ps.lefttree);
+       outerPlan = outerPlanState(node);
+       if (outerPlan->chgParam == NULL)
+               ExecReScan(outerPlan);
 }

 /* ----------------------------------------------------------------
diff --git a/src/backend/executor/nodeGroup.c b/src/backend/executor/nodeGroup.c
index 83d562e..b0d5442 100644
--- a/src/backend/executor/nodeGroup.c
+++ b/src/backend/executor/nodeGroup.c
@@ -280,6 +280,8 @@ ExecEndGroup(GroupState *node)
 void
 ExecReScanGroup(GroupState *node)
 {
+       PlanState       *outerPlan;
+
        node->grp_done = FALSE;
        node->ss.ps.ps_TupFromTlist = false;
        /* must clear first tuple */
@@ -289,7 +291,7 @@ ExecReScanGroup(GroupState *node)
         * if chgParam of subnode is not null then plan will be re-scanned by
         * first ExecProcNode.
         */
-       if (node->ss.ps.lefttree &&
-               node->ss.ps.lefttree->chgParam == NULL)
-               ExecReScan(node->ss.ps.lefttree);
+       outerPlan = outerPlanState(node);
+       if (outerPlan->chgParam == NULL)
+               ExecReScan(outerPlan);
 }
diff --git a/src/backend/executor/nodeMaterial.c b/src/backend/executor/nodeMaterial.c
index 1158825..41859e0 100644
--- a/src/backend/executor/nodeMaterial.c
+++ b/src/backend/executor/nodeMaterial.c
@@ -317,6 +317,9 @@ ExecMaterialRestrPos(MaterialState *node)
 void
 ExecReScanMaterial(MaterialState *node)
 {
+       PlanState       *outerPlan;
+
+       outerPlan = outerPlanState(node);
        ExecClearTuple(node->ss.ps.ps_ResultTupleSlot);

        if (node->eflags != 0)
@@ -339,13 +342,13 @@ ExecReScanMaterial(MaterialState *node)
                 * Otherwise we can just rewind and rescan the stored output. The
                 * state of the subnode does not change.
                 */
-               if (node->ss.ps.lefttree->chgParam != NULL ||
+               if (outerPlan->chgParam != NULL ||
                        (node->eflags & EXEC_FLAG_REWIND) == 0)
                {
                        tuplestore_end(node->tuplestorestate);
                        node->tuplestorestate = NULL;
-                       if (node->ss.ps.lefttree->chgParam == NULL)
-                               ExecReScan(node->ss.ps.lefttree);
+                       if (outerPlan->chgParam == NULL)
+                               ExecReScan(outerPlan);
                        node->eof_underlying = false;
                }
                else
@@ -359,8 +362,8 @@ ExecReScanMaterial(MaterialState *node)
                 * if chgParam of subnode is not null then plan will be re-scanned by
                 * first ExecProcNode.
                 */
-               if (node->ss.ps.lefttree->chgParam == NULL)
-                       ExecReScan(node->ss.ps.lefttree);
+               if (outerPlan->chgParam == NULL)
+                       ExecReScan(outerPlan);
                node->eof_underlying = false;
        }
 }
diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
index a815cde..32e064e 100644
--- a/src/backend/executor/nodeSort.c
+++ b/src/backend/executor/nodeSort.c
@@ -290,6 +290,8 @@ ExecSortRestrPos(SortState *node)
 void
 ExecReScanSort(SortState *node)
 {
+       PlanState       *outerPlan;
+
        /*
         * If we haven't sorted yet, just return. If outerplan's chgParam is not
         * NULL then it will be re-scanned by ExecProcNode, else no reason to
@@ -299,6 +301,7 @@ ExecReScanSort(SortState *node)
                return;

        /* must drop pointer to sort result tuple */
+       outerPlan = outerPlanState(node);
        ExecClearTuple(node->ss.ps.ps_ResultTupleSlot);

        /*
@@ -308,7 +311,7 @@ ExecReScanSort(SortState *node)
         *
         * Otherwise we can just rewind and rescan the sorted output.
         */
-       if (node->ss.ps.lefttree->chgParam != NULL ||
+       if (outerPlan->chgParam != NULL ||
                node->bounded != node->bounded_Done ||
                node->bound != node->bound_Done ||
                !node->randomAccess)
@@ -321,8 +324,8 @@ ExecReScanSort(SortState *node)
                 * if chgParam of subnode is not null then plan will be re-scanned by
                 * first ExecProcNode.
                 */
-               if (node->ss.ps.lefttree->chgParam == NULL)
-                       ExecReScan(node->ss.ps.lefttree);
+               if (outerPlan->chgParam == NULL)
+                       ExecReScan(outerPlan);
        }
        else
                tuplesort_rescan((Tuplesortstate *) node->tuplesortstate);
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index a06790d..520be66 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -2057,6 +2057,7 @@ ExecEndWindowAgg(WindowAggState *node)
 void
 ExecReScanWindowAgg(WindowAggState *node)
 {
+       PlanState       *outerPlan;
        ExprContext *econtext = node->ss.ps.ps_ExprContext;

        node->all_done = false;
@@ -2082,8 +2083,9 @@ ExecReScanWindowAgg(WindowAggState *node)
         * if chgParam of subnode is not null then plan will be re-scanned by
         * first ExecProcNode.
         */
-       if (node->ss.ps.lefttree->chgParam == NULL)
-               ExecReScan(node->ss.ps.lefttree);
+       outerPlan = outerPlanState(node);
+       if (outerPlan->chgParam == NULL)
+               ExecReScan(outerPlan);
 }

 /*

#6Robert Haas
robertmhaas@gmail.com
In reply to: Qingqing Zhou (#5)
Re: Use outerPlanState() consistently in executor code

On Thu, Apr 30, 2015 at 1:44 PM, Qingqing Zhou
<zhouqq.postgres@gmail.com> wrote:

On Thu, Apr 30, 2015 at 8:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:

I don't mind the MSDOS newlines, but the UTF-16le bit is inconvenient.
UTF-8 would be much better, so I don't have to figure out how to
convert.

The patch is generated via github windows tool and that's possibly
why. I regenerated it in Linux box and see attached (sending this
email in Windows and I hope no magic happens in-between).

Please let me know if that works.

Yeah, that seems fine. Anyone want to object to this?

--
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

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#6)
Re: Use outerPlanState() consistently in executor code

Robert Haas <robertmhaas@gmail.com> writes:

Yeah, that seems fine. Anyone want to object to this?

This hunk:

@@ -299,6 +301,7 @@ ExecReScanSort(SortState *node)
return;

/* must drop pointer to sort result tuple */
+ outerPlan = outerPlanState(node);
ExecClearTuple(node->ss.ps.ps_ResultTupleSlot);

/*

seems to have involved throwing darts at the source code to decide where
to insert the variable initialization; certainly putting a totally
unrelated operation between a comment and the line it describes is not
an improvement to code clarity in my book.

I think I'd have done many of these as

+ PlanState *outerPlan = outerPlanState(node);

rather than finding assorted random places to initialize the variables.

regards, tom lane

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

#8Qingqing Zhou
zhouqq.postgres@gmail.com
In reply to: Tom Lane (#7)
1 attachment(s)
Re: Use outerPlanState() consistently in executor code

On Thu, Apr 30, 2015 at 5:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think I'd have done many of these as

+ PlanState *outerPlan = outerPlanState(node);

rather than finding assorted random places to initialize the variables.

Agreed. Attached patch is revision along this line. Except for a few
that delayed assignments does not look a random kludge, I moved most
of others together with the declaration.

Regards,
Qingqing

Attachments:

nodestate.difftext/plain; charset=US-ASCII; name=nodestate.diffDownload
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
old mode 100644
new mode 100755
index 9ff0eff..fe3920e
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2053,10 +2053,10 @@ void
 ExecReScanAgg(AggState *node)
 {
 	ExprContext *econtext = node->ss.ps.ps_ExprContext;
+	PlanState	*outerPlan = outerPlanState(node);
 	int			aggno;
 
 	node->agg_done = false;
-
 	node->ss.ps.ps_TupFromTlist = false;
 
 	if (((Agg *) node->ss.ps.plan)->aggstrategy == AGG_HASHED)
@@ -2075,7 +2075,7 @@ ExecReScanAgg(AggState *node)
 		 * parameter changes, then we can just rescan the existing hash table;
 		 * no need to build it again.
 		 */
-		if (node->ss.ps.lefttree->chgParam == NULL)
+		if (outerPlan->chgParam == NULL)
 		{
 			ResetTupleHashIterator(node->hashtable, &node->hashiter);
 			return;
@@ -2133,8 +2133,8 @@ ExecReScanAgg(AggState *node)
 	 * if chgParam of subnode is not null then plan will be re-scanned by
 	 * first ExecProcNode.
 	 */
-	if (node->ss.ps.lefttree->chgParam == NULL)
-		ExecReScan(node->ss.ps.lefttree);
+	if (outerPlan->chgParam == NULL)
+		ExecReScan(outerPlan);
 }
 
 
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
old mode 100644
new mode 100755
index 8ea8b9f..6502841
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -449,6 +449,8 @@ ExecBitmapHeapScan(BitmapHeapScanState *node)
 void
 ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
 {
+	PlanState	*outerPlan;
+
 	/* rescan to release any page pin */
 	heap_rescan(node->ss.ss_currentScanDesc, NULL);
 
@@ -469,8 +471,9 @@ ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
 	 * if chgParam of subnode is not null then plan will be re-scanned by
 	 * first ExecProcNode.
 	 */
-	if (node->ss.ps.lefttree->chgParam == NULL)
-		ExecReScan(node->ss.ps.lefttree);
+	outerPlan = outerPlanState(node);
+	if (outerPlan->chgParam == NULL)
+		ExecReScan(outerPlan);
 }
 
 /* ----------------------------------------------------------------
diff --git a/src/backend/executor/nodeGroup.c b/src/backend/executor/nodeGroup.c
old mode 100644
new mode 100755
index 83d562e..b0d5442
--- a/src/backend/executor/nodeGroup.c
+++ b/src/backend/executor/nodeGroup.c
@@ -280,6 +280,8 @@ ExecEndGroup(GroupState *node)
 void
 ExecReScanGroup(GroupState *node)
 {
+	PlanState	*outerPlan;
+	
 	node->grp_done = FALSE;
 	node->ss.ps.ps_TupFromTlist = false;
 	/* must clear first tuple */
@@ -289,7 +291,7 @@ ExecReScanGroup(GroupState *node)
 	 * if chgParam of subnode is not null then plan will be re-scanned by
 	 * first ExecProcNode.
 	 */
-	if (node->ss.ps.lefttree &&
-		node->ss.ps.lefttree->chgParam == NULL)
-		ExecReScan(node->ss.ps.lefttree);
+	outerPlan = outerPlanState(node);
+	if (outerPlan->chgParam == NULL)
+		ExecReScan(outerPlan);
 }
diff --git a/src/backend/executor/nodeMaterial.c b/src/backend/executor/nodeMaterial.c
old mode 100644
new mode 100755
index 1158825..8ff4352
--- a/src/backend/executor/nodeMaterial.c
+++ b/src/backend/executor/nodeMaterial.c
@@ -317,6 +317,8 @@ ExecMaterialRestrPos(MaterialState *node)
 void
 ExecReScanMaterial(MaterialState *node)
 {
+	PlanState	*outerPlan = outerPlanState(node);
+
 	ExecClearTuple(node->ss.ps.ps_ResultTupleSlot);
 
 	if (node->eflags != 0)
@@ -339,13 +341,13 @@ ExecReScanMaterial(MaterialState *node)
 		 * Otherwise we can just rewind and rescan the stored output. The
 		 * state of the subnode does not change.
 		 */
-		if (node->ss.ps.lefttree->chgParam != NULL ||
+		if (outerPlan->chgParam != NULL ||
 			(node->eflags & EXEC_FLAG_REWIND) == 0)
 		{
 			tuplestore_end(node->tuplestorestate);
 			node->tuplestorestate = NULL;
-			if (node->ss.ps.lefttree->chgParam == NULL)
-				ExecReScan(node->ss.ps.lefttree);
+			if (outerPlan->chgParam == NULL)
+				ExecReScan(outerPlan);
 			node->eof_underlying = false;
 		}
 		else
@@ -359,8 +361,8 @@ ExecReScanMaterial(MaterialState *node)
 		 * if chgParam of subnode is not null then plan will be re-scanned by
 		 * first ExecProcNode.
 		 */
-		if (node->ss.ps.lefttree->chgParam == NULL)
-			ExecReScan(node->ss.ps.lefttree);
+		if (outerPlan->chgParam == NULL)
+			ExecReScan(outerPlan);
 		node->eof_underlying = false;
 	}
 }
diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
old mode 100644
new mode 100755
index a815cde..8bdffe7
--- a/src/backend/executor/nodeSort.c
+++ b/src/backend/executor/nodeSort.c
@@ -290,6 +290,8 @@ ExecSortRestrPos(SortState *node)
 void
 ExecReScanSort(SortState *node)
 {
+	PlanState	*outerPlan = outerPlanState(node);
+	
 	/*
 	 * If we haven't sorted yet, just return. If outerplan's chgParam is not
 	 * NULL then it will be re-scanned by ExecProcNode, else no reason to
@@ -308,7 +310,7 @@ ExecReScanSort(SortState *node)
 	 *
 	 * Otherwise we can just rewind and rescan the sorted output.
 	 */
-	if (node->ss.ps.lefttree->chgParam != NULL ||
+	if (outerPlan->chgParam != NULL ||
 		node->bounded != node->bounded_Done ||
 		node->bound != node->bound_Done ||
 		!node->randomAccess)
@@ -321,8 +323,8 @@ ExecReScanSort(SortState *node)
 		 * if chgParam of subnode is not null then plan will be re-scanned by
 		 * first ExecProcNode.
 		 */
-		if (node->ss.ps.lefttree->chgParam == NULL)
-			ExecReScan(node->ss.ps.lefttree);
+		if (outerPlan->chgParam == NULL)
+			ExecReScan(outerPlan);
 	}
 	else
 		tuplesort_rescan((Tuplesortstate *) node->tuplesortstate);
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index a06790d..520be66 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -2057,6 +2057,7 @@ ExecEndWindowAgg(WindowAggState *node)
 void
 ExecReScanWindowAgg(WindowAggState *node)
 {
+	PlanState	*outerPlan;
 	ExprContext *econtext = node->ss.ps.ps_ExprContext;
 
 	node->all_done = false;
@@ -2082,8 +2083,9 @@ ExecReScanWindowAgg(WindowAggState *node)
 	 * if chgParam of subnode is not null then plan will be re-scanned by
 	 * first ExecProcNode.
 	 */
-	if (node->ss.ps.lefttree->chgParam == NULL)
-		ExecReScan(node->ss.ps.lefttree);
+	outerPlan = outerPlanState(node);
+	if (outerPlan->chgParam == NULL)
+		ExecReScan(outerPlan);
 }
 
 /*
#9Robert Haas
robertmhaas@gmail.com
In reply to: Qingqing Zhou (#8)
Re: Use outerPlanState() consistently in executor code

On Fri, May 1, 2015 at 1:05 PM, Qingqing Zhou <zhouqq.postgres@gmail.com> wrote:

On Thu, Apr 30, 2015 at 5:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think I'd have done many of these as

+ PlanState *outerPlan = outerPlanState(node);

rather than finding assorted random places to initialize the variables.

Agreed. Attached patch is revision along this line. Except for a few
that delayed assignments does not look a random kludge, I moved most
of others together with the declaration.

I fixed several whitespace errors, reverted the permissions changes
you included, adjusted the remaining call site to be the way Tom wants
(and I think he's right), and committed this.

--
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

#10Qingqing Zhou
zhouqq.postgres@gmail.com
In reply to: Robert Haas (#9)
Re: Use outerPlanState() consistently in executor code

On Mon, May 4, 2015 at 1:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I fixed several whitespace errors, reverted the permissions changes
you included

Sorry about the permission changes - didn't notice that bite.

Thanks,
Qingqing

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