Re: [HACKERS] Use outerPlanState() consistently in executor code
On Mon, May 4, 2015 at 1:23 PM, Robert Haas robertmh...@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
Re: [HACKERS] Use outerPlanState() consistently in executor code
On Fri, May 1, 2015 at 1:05 PM, Qingqing Zhou zhouqq.postg...@gmail.com wrote: On Thu, Apr 30, 2015 at 5:04 PM, Tom Lane t...@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
Re: [HACKERS] Use outerPlanState() consistently in executor code
On Thu, Apr 30, 2015 at 5:04 PM, Tom Lane t...@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 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);
Re: [HACKERS] 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.postg...@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 br...@momjian.ushttp://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
Re: [HACKERS] Use outerPlanState() consistently in executor code
On Wed, Apr 15, 2015 at 3:38 PM, Qingqing Zhou zhouqq.postg...@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
Re: [HACKERS] Use outerPlanState() consistently in executor code
On Thu, Apr 30, 2015 at 9:02 AM, Bruce Momjian br...@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.postg...@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
Re: [HACKERS] Use outerPlanState() consistently in executor code
On Thu, Apr 30, 2015 at 8:02 AM, Robert Haas robertmh...@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 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;
Re: [HACKERS] Use outerPlanState() consistently in executor code
On Thu, Apr 30, 2015 at 1:44 PM, Qingqing Zhou zhouqq.postg...@gmail.com wrote: On Thu, Apr 30, 2015 at 8:02 AM, Robert Haas robertmh...@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
Re: [HACKERS] Use outerPlanState() consistently in executor code
Robert Haas robertmh...@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