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