Thread: setting estate in ExecInitNode() itself
Hi, Looking at ExecInitXYZ() functions, we can observe that every such function has a statement like XYZstate->ps.state = estate; where it saves estate in the PlanState. I am wondering why don't we instead save estate in ExecInitNode() itself like result->estate = estate; Are there any cases when a node wants to set a different estate there? -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> Looking at ExecInitXYZ() functions, we can observe that every such
> function has a statement like
> XYZstate->ps.state = estate;
> where it saves estate in the PlanState.
Yeah ...
> I am wondering why don't we instead save estate in ExecInitNode() itself like
> result->estate = estate;
That would only work if there were no situation where the field needed to
be already valid during the node init function. I think that's probably
wrong already (check ExecInitExpr for instance) and it certainly might
be wrong in future.
regards, tom lane
On 2018-01-05 10:36:19 -0500, Tom Lane wrote: > Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes: > > Looking at ExecInitXYZ() functions, we can observe that every such > > function has a statement like > > XYZstate->ps.state = estate; > > where it saves estate in the PlanState. > > Yeah ... > > > I am wondering why don't we instead save estate in ExecInitNode() itself like > > result->estate = estate; > > That would only work if there were no situation where the field needed to > be already valid during the node init function. I think that's probably > wrong already (check ExecInitExpr for instance) and it certainly might > be wrong in future. Agreed on that. But I also think there's quite some room for centralizing some of the work done in the init routines. Not quite sure how, but I do dislike the amount of repetition both in code and comments. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2018-01-05 10:36:19 -0500, Tom Lane wrote:
>> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>>> I am wondering why don't we instead save estate in ExecInitNode() itself like
>>> result->estate = estate;
>> That would only work if there were no situation where the field needed to
>> be already valid during the node init function. I think that's probably
>> wrong already (check ExecInitExpr for instance) and it certainly might
>> be wrong in future.
> Agreed on that. But I also think there's quite some room for
> centralizing some of the work done in the init routines. Not quite sure
> how, but I do dislike the amount of repetition both in code and
> comments.
Yeah, there might be room for putting more of the common node init work
into standard macros or some such. Need to think bigger than just this
one point though, or it won't be worth it.
regards, tom lane
On Fri, Jan 5, 2018 at 2:50 PM, Andres Freund <andres@anarazel.de> wrote: > Agreed on that. But I also think there's quite some room for > centralizing some of the work done in the init routines. Not quite sure > how, but I do dislike the amount of repetition both in code and > comments. +1. I *assume* that if you really understand how all of this stuff works, adding new types of executor nodes is easy to do correctly. But, as the executor README says "XXX a great deal more documentation needs to be written here..." -- BTW, that comment dates to 2001 -- and I have found it not to be all that straightforward (cf. commits 8538a6307049590ddb5ba127b2ecac6308844d60, 7df2c1f8daeb361133ac8bdeaf59ceb0484e315a, 41b0dd987d44089dc48e9c70024277e253b396b7, 0510421ee091ee3ddabdd5bd7ed096563f6bf17f, b10967eddf964f8c0a11060cf3f366bbdd1235f6). Having similar, but often very briefly commented, initialization, rescan, and cleanup nodes in every file makes it hard to understand what actually needs to be done differently in each case, and why. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company