Thread: Use outerPlanState() consistently in executor code
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
Attachment
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
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. +
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
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
Attachment
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
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
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
Attachment
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
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