Обсуждение: 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