Thread: [HACKERS] segfault in HEAD when too many nested functions call
Hello,
Since b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755 (Andres in Cc), if you
write queries which result in infinite recursion (or just too many
nested function calls), execution ends with segfault instead of intended
exhausted max_stack_depth:
Program received signal SIGSEGV, Segmentation fault.
0x0000000000df851e in DirectFunctionCall1Coll (
func=<error reading variable: Cannot access memory at address
0x7ffef5972f98>,
collation=<error reading variable: Cannot access memory at address
0x7ffef5972f94>,
arg1=<error reading variable: Cannot access memory at address
0x7ffef5972f88>) at fmgr.c:708
Please find attached a trivial patch to fix this. I'm not sure
ExecMakeTableFunctionResult() is the best or only place that needs to
check the stack depth.
I also attached a simple sql file to reproduce the issue if needed.
Should I add a regression test based on it?
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Julien Rouhaud <julien.rouhaud@dalibo.com> writes:
> Since b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755 (Andres in Cc), if you
> write queries which result in infinite recursion (or just too many
> nested function calls), execution ends with segfault instead of intended
> exhausted max_stack_depth:
Yes. We discussed this before the patch went in [1]. I wanted to put
a stack depth check in ExecProcNode(), and still do. Andres demurred,
claiming that that was too much overhead, but didn't really provide a
credible alternative. The thread drifted off without any resolution,
but clearly we need to do something before 10.0 final.
> Please find attached a trivial patch to fix this. I'm not sure
> ExecMakeTableFunctionResult() is the best or only place that needs to
> check the stack depth.
I don't think that that's adequate at all.
I experimented with a variant that doesn't go through
ExecMakeTableFunctionResult:
CREATE OR REPLACE FUNCTION so()RETURNS intLANGUAGE plpgsql
AS $$
DECLARErec RECORD;
BEGIN FOR rec IN SELECT so() as x LOOP RETURN rec.x; END LOOP; RETURN NULL;
END;
$$;
SELECT so();
This manages not to crash, but I think the reason it does not is purely
accidental: "SELECT so()" has a nontrivial targetlist so we end up running
ExecBuildProjectionInfo on that, meaning that a fresh expression
compilation happens at every nesting depth, and there are
check_stack_depth calls in expression compilation. Surely that's
something we'd try to improve someday. Or it could accidentally get
broken by unrelated changes in the way plpgsql sets up queries to be
executed.
I still think that we really need to add a check in ExecProcNode().
Even if there's an argument to be made that every recursion would
somewhere go through ExecMakeTableFunctionResult, very large/complex
queries could result in substantial stack getting chewed up before
we get to that --- and we don't have an infinite amount of stack slop.
regards, tom lane
[1] https://www.postgresql.org/message-id/22833.1490390175@sss.pgh.pa.us
I wrote:
> I still think that we really need to add a check in ExecProcNode().
Actually ... to what extent could a check in ExecInitNode() substitute
for that? Or do we need both? How about ExecEndNode() and ExecReScan()?
You could argue that the latter two tree traversals are unlikely either to
consume more stack than ExecInitNode() or to be called from a more deeply
nested point. So maybe they're okay. But I'm not sure I believe that
initialization stack needs always exceed execution stack needs, though
sometimes they might.
regards, tom lane
On 15/07/2017 17:22, Tom Lane wrote: > Julien Rouhaud <julien.rouhaud@dalibo.com> writes: >> Since b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755 (Andres in Cc), if you >> write queries which result in infinite recursion (or just too many >> nested function calls), execution ends with segfault instead of intended >> exhausted max_stack_depth: > > Yes. We discussed this before the patch went in [1]. Ah, I totally forgot about it, sorry. > I wanted to put > a stack depth check in ExecProcNode(), and still do. Andres demurred, > claiming that that was too much overhead, but didn't really provide a > credible alternative. The thread drifted off without any resolution, > but clearly we need to do something before 10.0 final. > I wanted to add an open item but I see you already did, thanks! >> Please find attached a trivial patch to fix this. I'm not sure >> ExecMakeTableFunctionResult() is the best or only place that needs to >> check the stack depth. > > I don't think that that's adequate at all. > > I experimented with a variant that doesn't go through > ExecMakeTableFunctionResult: > > [...] > This manages not to crash, but I think the reason it does not is purely > accidental: "SELECT so()" has a nontrivial targetlist so we end up running > ExecBuildProjectionInfo on that, meaning that a fresh expression > compilation happens at every nesting depth, and there are > check_stack_depth calls in expression compilation. Surely that's > something we'd try to improve someday. Or it could accidentally get > broken by unrelated changes in the way plpgsql sets up queries to be > executed. > > I still think that we really need to add a check in ExecProcNode(). > Even if there's an argument to be made that every recursion would > somewhere go through ExecMakeTableFunctionResult, very large/complex > queries could result in substantial stack getting chewed up before > we get to that --- and we don't have an infinite amount of stack slop. I was pretty sceptical about checking depth in ExecMakeTableFunctionResult() only vs ExecProcNode(), and this has finished convincing me so I definitely agree. -- Julien Rouhaud http://dalibo.com - http://dalibo.org
On Sat, Jul 15, 2017 at 11:22:37AM -0400, Tom Lane wrote: > Julien Rouhaud <julien.rouhaud@dalibo.com> writes: > > Since b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755 (Andres in Cc), if you > > write queries which result in infinite recursion (or just too many > > nested function calls), execution ends with segfault instead of intended > > exhausted max_stack_depth: > > Yes. We discussed this before the patch went in [1]. I wanted to put > a stack depth check in ExecProcNode(), and still do. Andres demurred, > claiming that that was too much overhead, but didn't really provide a > credible alternative. The thread drifted off without any resolution, > but clearly we need to do something before 10.0 final. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Andres, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
Hi, On 2017-07-15 11:22:37 -0400, Tom Lane wrote: > The thread drifted off without any resolution, but clearly we need to > do something before 10.0 final. We need to do something, I'm less convinced that it's really v10 specific :/ > "SELECT so()" has a nontrivial targetlist so we end up running > ExecBuildProjectionInfo on that, meaning that a fresh expression > compilation happens at every nesting depth, and there are > check_stack_depth calls in expression compilation. Surely that's > something we'd try to improve someday. Or it could accidentally get > broken by unrelated changes in the way plpgsql sets up queries to be > executed. Independent of $subject: What are you thinking of here? You want to avoid the ExecBuildProjectionInfo() in more cases - I'm unconvinced that's actually helpful. In my WIP JIT compilation patch the ExecBuildProjectionInfo() ends up being a good bit faster than paths avoiding it. > I still think that we really need to add a check in ExecProcNode(). > Even if there's an argument to be made that every recursion would > somewhere go through ExecMakeTableFunctionResult, very large/complex > queries could result in substantial stack getting chewed up before > we get to that --- and we don't have an infinite amount of stack slop. I'm less convinced of that, due to the overhead argument. I think there's a couple ways around that however: 1) We could move ExecProcNode() to be callback based. The first time a node is executed a "wrapper" function is called thatdoes the stack and potentially other checks. That also makes ExecProcNode() small enough to be inlined, which endsup being good for jump target prediction. I've done something similar for v11 for expression evaluation, gettingrid of EEOP_*_FIRST duplication etc, and it seems to work well. The big disadvantage to that is that it's a bit invasive for v10, and very likely too invasive to backpatch. 2) I think there's some fair argument to be made that ExecInitNode()'s stack-space needs are similar enough to ExecProcNode()'sallowing us to put a check_stack_depth() into the former. That seems like it's required anyway, sincein many cases that's going to trigger stack-depth exhaustion first anyway (unless we hit it in parse analysis, whichalso seems quite common). Greetings, Andres Freund
Hi, On 2017-07-17 00:26:29 -0700, Andres Freund wrote: > I'm less convinced of that, due to the overhead argument. I think > there's a couple ways around that however: > > 1) We could move ExecProcNode() to be callback based. The first time a > node is executed a "wrapper" function is called that does the stack > and potentially other checks. That also makes ExecProcNode() small > enough to be inlined, which ends up being good for jump target > prediction. I've done something similar for v11 for expression > evaluation, getting rid of EEOP_*_FIRST duplication etc, and it seems > to work well. The big disadvantage to that is that it's a bit > invasive for v10, and very likely too invasive to backpatch. > 2) I think there's some fair argument to be made that ExecInitNode()'s > stack-space needs are similar enough to ExecProcNode()'s allowing us > to put a check_stack_depth() into the former. That seems like it's > required anyway, since in many cases that's going to trigger > stack-depth exhaustion first anyway (unless we hit it in parse > analysis, which also seems quite common). Attached is a trivial patch implementing 1) and a proof-of-concept hack for 2). The latter obviously isn't ready, but might make clearer what I'm thinking about. If we were to go this route we'd have to probably move the callback assignment into the ExecInit* routines, and possibly replace the switch in ExecInitNode() with another callback, assigned in make_*, and implement callbacks for ExecShutdown, MultiExecProcNode etc. This results in a good speedup in tpc-h btw: master total min: 46434.897 cb min: 44778.228 [diff -3.70] - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 17/07/2017 16:57, Andres Freund wrote: > The latter obviously isn't ready, but might make clearer what I'm > thinking about. It did for me, and FWIW I like this approach. > If we were to go this route we'd have to probably move > the callback assignment into the ExecInit* routines, and possibly > replace the switch in ExecInitNode() with another callback, assigned in > make_*, and implement callbacks for ExecShutdown, MultiExecProcNode etc. > > This results in a good speedup in tpc-h btw: > master total min: 46434.897 cb min: 44778.228 [diff -3.70] Is it v11 material or is there any chance to make it in v10? -- Julien Rouhaud http://dalibo.com - http://dalibo.org
On 2017-07-17 23:04:43 +0200, Julien Rouhaud wrote: > On 17/07/2017 16:57, Andres Freund wrote: > > The latter obviously isn't ready, but might make clearer what I'm > > thinking about. > > It did for me, and FWIW I like this approach. Cool. > > If we were to go this route we'd have to probably move > > the callback assignment into the ExecInit* routines, and possibly > > replace the switch in ExecInitNode() with another callback, assigned in > > make_*, and implement callbacks for ExecShutdown, MultiExecProcNode etc. > > > > This results in a good speedup in tpc-h btw: > > master total min: 46434.897 cb min: 44778.228 [diff -3.70] > > Is it v11 material or is there any chance to make it in v10? I think it'd make sense to apply the first to v10 (and earlier), and the second to v11. I think this isn't a terribly risky patch, but it's still a relatively large change for this point in the development cycle. I'm willing to reconsider, but that's my default. - Andres
On 18/07/2017 14:04, Andres Freund wrote: > On 2017-07-17 23:04:43 +0200, Julien Rouhaud wrote: >> Is it v11 material or is there any chance to make it in v10? > > I think it'd make sense to apply the first to v10 (and earlier), and the > second to v11. I think this isn't a terribly risky patch, but it's > still a relatively large change for this point in the development > cycle. I'm willing to reconsider, but that's my default. Agreed. -- Julien Rouhaud
Andres Freund <andres@anarazel.de> writes:
> Attached is a trivial patch implementing 1) and a proof-of-concept hack
> for 2).
The comment you made previously, as well as the proposed commit message
for 0001, suggest that you've forgotten that pre-v10 execQual.c had
several check_stack_depth() calls. Per its header comment,
* During expression evaluation, we check_stack_depth only in* ExecMakeFunctionResult (and substitute
routines)rather than at every* single node. This is a compromise that trades off precision of the* stack
limitsetting to gain speed.
and there was also a check in the recursive ExecInitExpr routine.
While it doesn't seem to be documented anywhere, I believe that we'd
argued that ExecProcNode and friends didn't need their own stack depth
checks because any nontrivial node would surely do expression evaluation
which would contain a check.
So the basic issue here is that (1) expression eval, per se, no longer
has any check and (2) it's not clear that we can rely on expression
compilation to substitute for that, since at least in principle
recompilation could be skipped during recursive use of a plan node.
I agree that adding a check to ExecInitNode() is really necessary,
but I'm not convinced that it's a sufficient substitute for checking
in ExecProcNode(). The two flaws I see in that argument are
(a) you've provided no hard evidence backing up the argument that node
initialization will take strictly more stack space than node execution;
as far as I can see that's just wishful thinking.
(b) there's also an implicit assumption that ExecutorRun is called from
a point not significantly more deeply nested than the corresponding
call to ExecutorStart. This seems also to depend on nothing much better
than wishful thinking. Certainly, many ExecutorRun calls are right next
to ExecutorStart, but several of them aren't; the portal code and
SQL-function code are both scary in this respect.
> The latter obviously isn't ready, but might make clearer what I'm
> thinking about. If we were to go this route we'd have to probably move
> the callback assignment into the ExecInit* routines, and possibly
> replace the switch in ExecInitNode() with another callback, assigned in
> make_*, and implement callbacks for ExecShutdown, MultiExecProcNode etc.
While I'm uncomfortable with making such a change post-beta2, I'm one
heck of a lot *more* uncomfortable with shipping v10 with no stack depth
checks in the executor mainline. Fleshing this out and putting it in
v10 might be an acceptable compromise.
regards, tom lane
On 2017-07-18 15:45:53 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Attached is a trivial patch implementing 1) and a proof-of-concept hack > > for 2). > > The comment you made previously, as well as the proposed commit message > for 0001, suggest that you've forgotten that pre-v10 execQual.c had > several check_stack_depth() calls. Per its header comment, > * During expression evaluation, we check_stack_depth only in > * ExecMakeFunctionResult (and substitute routines) rather than at every > * single node. This is a compromise that trades off precision of the > * stack limit setting to gain speed. No, I do remember that fact. But a) unfortunately that's not really that useful because by far not all function calls go through ExecMakeFunctionResult() (e.g. ExecEvalDistinct() and a bunch of other FunctionCallInvoke() containing functions). b) deeply nested executor nodes - and that's what the commit's about to a good degree - aren't necessarily guaranteed toactually evaluate expressions. There's no guarantee there's any expressions (you could just nest joins without conditions),and a bunch of nodes like hashjoins invoke functions themselves. > and there was also a check in the recursive ExecInitExpr routine. Which still is there. > While it doesn't seem to be documented anywhere, I believe that we'd > argued that ExecProcNode and friends didn't need their own stack depth > checks because any nontrivial node would surely do expression evaluation > which would contain a check. Yea, I don't buy that at all. > I agree that adding a check to ExecInitNode() is really necessary, > but I'm not convinced that it's a sufficient substitute for checking > in ExecProcNode(). The two flaws I see in that argument are > > (a) you've provided no hard evidence backing up the argument that node > initialization will take strictly more stack space than node execution; > as far as I can see that's just wishful thinking. I think it's pretty likely to be roughly (within slop) the case in realistic scenarios, but I do feel fairly uncomfortable about the assumption. That's why I really want to do something like the what I'm proposing in the second patch. I just don't think we can realistically add the check to the back branches, given that it's quite measurable performancewise. > (b) there's also an implicit assumption that ExecutorRun is called from > a point not significantly more deeply nested than the corresponding > call to ExecutorStart. This seems also to depend on nothing much better > than wishful thinking. Certainly, many ExecutorRun calls are right next > to ExecutorStart, but several of them aren't; the portal code and > SQL-function code are both scary in this respect. :/ > > The latter obviously isn't ready, but might make clearer what I'm > > thinking about. If we were to go this route we'd have to probably move > > the callback assignment into the ExecInit* routines, and possibly > > replace the switch in ExecInitNode() with another callback, assigned in > > make_*, and implement callbacks for ExecShutdown, MultiExecProcNode etc. > > While I'm uncomfortable with making such a change post-beta2, I'm one > heck of a lot *more* uncomfortable with shipping v10 with no stack depth > checks in the executor mainline. Fleshing this out and putting it in > v10 might be an acceptable compromise. Ok, I'll flesh out the patch till Thursday. But I do think we're going to have to do something about the back branches, too. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2017-07-18 15:45:53 -0400, Tom Lane wrote:
>> While I'm uncomfortable with making such a change post-beta2, I'm one
>> heck of a lot *more* uncomfortable with shipping v10 with no stack depth
>> checks in the executor mainline. Fleshing this out and putting it in
>> v10 might be an acceptable compromise.
> Ok, I'll flesh out the patch till Thursday. But I do think we're going
> to have to do something about the back branches, too.
I'm not worried about the back branches. The stack depth checks have
been in those same places for ten years at least, and there are no field
reports of trouble.
regards, tom lane
Andres Freund <andres@anarazel.de> writes:
> ... If we were to go this route we'd have to probably move
> the callback assignment into the ExecInit* routines, and possibly
> replace the switch in ExecInitNode() with another callback, assigned in
> make_*, and implement callbacks for ExecShutdown, MultiExecProcNode etc.
BTW, I don't see why you really need to mess with anything except
ExecProcNode? Surely the other cases such as MultiExecProcNode are
not called often enough to justify changing them away from the
switch technology. Yeah, maybe it would be a bit cleaner if they
all looked alike ... but if you're trying to make a patch that's
as little invasive as possible for v10, I'd suggest converting just
ExecProcNode to this style.
regards, tom lane
On 2017-07-18 16:53:43 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > ... If we were to go this route we'd have to probably move > > the callback assignment into the ExecInit* routines, and possibly > > replace the switch in ExecInitNode() with another callback, assigned in > > make_*, and implement callbacks for ExecShutdown, MultiExecProcNode etc. > > BTW, I don't see why you really need to mess with anything except > ExecProcNode? Surely the other cases such as MultiExecProcNode are > not called often enough to justify changing them away from the > switch technology. Yeah, maybe it would be a bit cleaner if they > all looked alike ... but if you're trying to make a patch that's > as little invasive as possible for v10, I'd suggest converting just > ExecProcNode to this style. Yea, I was making that statement when not aiming for v10. Attached is a patch that converts just ExecProcNode. The big change in comparison to the earlier patch is that the assignment of the callback is now done in the respective ExecInit* routines. As a consequence the ExecProcNode callbacks now are static. I think we should do this fully in v11, I removing dispatch routines like ExecInitNode() is a good idea, both because it moves concerns more towards the nodes themselves - given the growth of executor nodes that strikes me as a good idea. I've also added stack depth checks to ExecEndNode(), MultiExecProcNode(), ExecShutdownNode(), because it's not guaranteed that ExecProcNode is called for every node... I dislike having the miscadmin.h include in executor.h - but I don't quite see a better alternative. I want to run this through pgindent before merging, otherwise we'll presumably end up with a lot of noise. I still think we should backpatch at least the check_stack_depth() calls in ExecInitNode(), ExecEndNode(). Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 21/07/2017 13:40, Andres Freund wrote: > Attached is a > patch that converts just ExecProcNode. The big change in comparison to > the earlier patch is that the assignment of the callback is now done in > the respective ExecInit* routines. As a consequence the ExecProcNode > callbacks now are static. Thanks for working on it. Just in case, I reviewed the patch and didn't find any issue with it. -- Julien Rouhaud
Andres Freund <andres@anarazel.de> writes:
> On 2017-07-18 16:53:43 -0400, Tom Lane wrote:
>> BTW, I don't see why you really need to mess with anything except
>> ExecProcNode? Surely the other cases such as MultiExecProcNode are
>> not called often enough to justify changing them away from the
>> switch technology. Yeah, maybe it would be a bit cleaner if they
>> all looked alike ... but if you're trying to make a patch that's
>> as little invasive as possible for v10, I'd suggest converting just
>> ExecProcNode to this style.
> Yea, I was making that statement when not aiming for v10. Attached is a
> patch that converts just ExecProcNode.
I read through this patch (didn't test it at all yet).
> I dislike having the miscadmin.h include in executor.h - but I don't
> quite see a better alternative.
I seriously, seriously, seriously dislike that. You practically might as
well put miscadmin.h into postgres.h. Instead, what do you think of
requiring the individual ExecProcNode functions to perform
CHECK_FOR_INTERRUPTS? Since they're already responsible for doing that
if they have any long-running internal loops, this doesn't seem like a
modularity violation. It is a risk for bugs-of-omission, sure, but so
are a lot of other things that the per-node code has to do.
There might be something to be said for handling the chgParam/rescan tests
similarly. That would reduce the ExecProcNode macro to a triviality,
which would be a good thing for understandability of the code I think.
Some other thoughts:
* I think the comments need more work. Am willing to make a pass over
that if you want.
* In most places, if there's an immediate return-if-trivial-case test,
we check stack depth only after that. There's no point in checking
and then returning; either you already crashed, or you're at peak
stack so far as this code path is concerned.
* Can we redefine the ExecCustomScan function pointer as type
ExecProcNodeCB, eliminating the unsightly cast in nodeCustom.c?
* The various callers of ExecScan() are pretty inconsistently coded.
I don't care that much whether they use castNode() or just forcibly
cast to ScanState*, but let's make them all look the same.
* I believe the usual term for what these function pointers are is
"methods", not "callbacks". Certainly we'd call them that if we
were working in C++.
> I still think we should backpatch at least the check_stack_depth() calls
> in ExecInitNode(), ExecEndNode().
No big objection, although I'm not sure it's necessary.
regards, tom lane
On Tue, Jul 18, 2017 at 01:04:10PM -0700, Andres Freund wrote: > Ok, I'll flesh out the patch till Thursday. But I do think we're going > to have to do something about the back branches, too. This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On July 24, 2017 7:10:19 AM GMT+01:00, Noah Misch <noah@leadboat.com> wrote: >On Tue, Jul 18, 2017 at 01:04:10PM -0700, Andres Freund wrote: >> Ok, I'll flesh out the patch till Thursday. But I do think we're >going >> to have to do something about the back branches, too. > >This PostgreSQL 10 open item is past due for your status update. >Kindly send >a status update within 24 hours, and include a date for your subsequent >status >update. Refer to the policy on open item ownership: >https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com I sent out a note fleshed out patch last week, which Tom reviewed. Planning to update it to address that review today ortomorrow. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi, On 2017-07-21 20:17:54 -0400, Tom Lane wrote: > > I dislike having the miscadmin.h include in executor.h - but I don't > > quite see a better alternative. > > I seriously, seriously, seriously dislike that. You practically might as > well put miscadmin.h into postgres.h. Instead, what do you think of > requiring the individual ExecProcNode functions to perform > CHECK_FOR_INTERRUPTS? Since they're already responsible for doing that > if they have any long-running internal loops, this doesn't seem like a > modularity violation. It is a risk for bugs-of-omission, sure, but so > are a lot of other things that the per-node code has to do. That'd work. Another alternative would be to move the inline definition of ExecProcNode() (and probably a bunch of other related functions) into a more internals oriented header. It seems likely that we're going to add more inline functions to the executor, and that'd reduce the coupling of external and internal users a bit. > * I think the comments need more work. Am willing to make a pass over > that if you want. That'd be good, but let's wait till we have something more final. > * In most places, if there's an immediate return-if-trivial-case test, > we check stack depth only after that. There's no point in checking > and then returning; either you already crashed, or you're at peak > stack so far as this code path is concerned. I went back/forth a bit on that one. The calling code might call other functions that go deeper on the stack, which won't have the checks. Fine with moving, just wanted to explain why I got there. > * Can we redefine the ExecCustomScan function pointer as type > ExecProcNodeCB, eliminating the unsightly cast in nodeCustom.c? That'd change an "extension API", which is why I skipped it at this point of the release cycle. It's not like we didn't have this type of cast all over before. Ok, with changing it, but that's where I came down. > * The various callers of ExecScan() are pretty inconsistently coded. > I don't care that much whether they use castNode() or just forcibly > cast to ScanState*, but let's make them all look the same. I tried changed the minimum, perfectly fine to move to castNode in a wholesale manner. Btw, I really want to get rid of ExecScan(), at least as an external function. Does a lot of unnecessary things in a lot of cases, and makes branch prediction a lot worse. Not v10 stuff tho. > * I believe the usual term for what these function pointers are is > "methods", not "callbacks". Certainly we'd call them that if we > were working in C++. K. - Andres
Andres Freund <andres@anarazel.de> writes:
> On 2017-07-21 20:17:54 -0400, Tom Lane wrote:
>>> I dislike having the miscadmin.h include in executor.h - but I don't
>>> quite see a better alternative.
>> I seriously, seriously, seriously dislike that. You practically might as
>> well put miscadmin.h into postgres.h. Instead, what do you think of
>> requiring the individual ExecProcNode functions to perform
>> CHECK_FOR_INTERRUPTS? Since they're already responsible for doing that
>> if they have any long-running internal loops, this doesn't seem like a
>> modularity violation. It is a risk for bugs-of-omission, sure, but so
>> are a lot of other things that the per-node code has to do.
> That'd work. Another alternative would be to move the inline definition
> of ExecProcNode() (and probably a bunch of other related functions) into
> a more internals oriented header. It seems likely that we're going to
> add more inline functions to the executor, and that'd reduce the
> coupling of external and internal users a bit.
Well, it still ends up that the callers of ExecProcNode need to include
miscadmin.h, whereas if we move it into the per-node functions, then the
per-node files need to include miscadmin.h. I think the latter is better
because those files may need to have other CHECK_FOR_INTERRUPTS calls
anyway. It's less clear from a modularity standpoint that executor
callers should need miscadmin.h. (Or in short, I'm not really okay
with *any* header file including miscadmin.h.)
>> * I think the comments need more work. Am willing to make a pass over
>> that if you want.
> That'd be good, but let's wait till we have something more final.
Agreed, I'll wait till you produce another version.
>> * Can we redefine the ExecCustomScan function pointer as type
>> ExecProcNodeCB, eliminating the unsightly cast in nodeCustom.c?
> That'd change an "extension API", which is why I skipped it at this
> point of the release cycle. It's not like we didn't have this type of
> cast all over before. Ok, with changing it, but that's where I came
> down.
Is this patch really not changing anything else that a custom-scan
extension would touch? If not, I'm okay with postponing this bit
of cleanup to v11.
regards, tom lane
On 2017-07-24 13:27:58 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> >> I seriously, seriously, seriously dislike that. You practically might as
> >> well put miscadmin.h into postgres.h. Instead, what do you think of
> >> requiring the individual ExecProcNode functions to perform
> >> CHECK_FOR_INTERRUPTS? Since they're already responsible for doing that
> >> if they have any long-running internal loops, this doesn't seem like a
> >> modularity violation. It is a risk for bugs-of-omission, sure, but so
> >> are a lot of other things that the per-node code has to do.
>
> > That'd work. Another alternative would be to move the inline definition
> > of ExecProcNode() (and probably a bunch of other related functions) into
> > a more internals oriented header. It seems likely that we're going to
> > add more inline functions to the executor, and that'd reduce the
> > coupling of external and internal users a bit.
>
> Well, it still ends up that the callers of ExecProcNode need to include
> miscadmin.h, whereas if we move it into the per-node functions, then the
> per-node files need to include miscadmin.h. I think the latter is better
> because those files may need to have other CHECK_FOR_INTERRUPTS calls
> anyway.
> It's less clear from a modularity standpoint that executor
> callers should need miscadmin.h.
Well, that's why I'm pondering an executor_internal.h or something -
there shouldn't be ExecProcNode() callers that don't also need CFI(),
and no executor callers should need ExecProcNode(). executor.h right now
really defines infrastructure to *use* the executor
(Executor{Start,Run,Finish,End,Rewind}), functions internal to the
executor (lots of initialization functions, EPQ, partition logic), some
things inbetween (e.g. expression related stuff), and some things that
really should be separate ExecOpenIndices etc, execReplication.c
functions. But that's not something we can easily clear up just now.
> (Or in short, I'm not really okay with *any* header file including
> miscadmin.h.)
Perhaps that's a sign that we should split it up? It's a weird grab bag
atm. utils/interrupt.h or such would e.g. make sense for for the
*INTERRUPTS, and *CRIT_SECTION macros, as well as ProcessInterrupts()
itself, which imo isn't super well placed in postgres.c
either. Including utils/interrupt.h in a header would be much less
odious in my opinion than including miscadmin.h.
> >> * Can we redefine the ExecCustomScan function pointer as type
> >> ExecProcNodeCB, eliminating the unsightly cast in nodeCustom.c?
>
> > That'd change an "extension API", which is why I skipped it at this
> > point of the release cycle. It's not like we didn't have this type of
> > cast all over before. Ok, with changing it, but that's where I came
> > down.
>
> Is this patch really not changing anything else that a custom-scan
> extension would touch? If not, I'm okay with postponing this bit
> of cleanup to v11.
Not that I can see - I've build & tested citus which uses custom scans
these days with and without patch without trouble. Nor do I see any
change in the current patch that'd be troublesome - after all the
API of ExecProcNode() stays the same.
- Andres
Hi, On 2017-07-24 13:27:58 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > >> * I think the comments need more work. Am willing to make a pass over > >> that if you want. > > > That'd be good, but let's wait till we have something more final. > > Agreed, I'll wait till you produce another version. Attached. Did a bunch of cleanup myself already. I've moved the CHECK_FOR_INTERRUPTS() to the callsites. That unsurprisingly ends up being somewhat verbose, and there's a bunch of minor judgement calls where exactly to place them. While doing so I've also added a few extra ones. Did this in a separate patch to make it easier to review. I'm pretty jetlagged right now, so I want to do another pass to make sure I didn't forget any CFI()s, but the general shape looks right. Tried to address the rest of your feedback too. > >> * Can we redefine the ExecCustomScan function pointer as type > >> ExecProcNodeCB, eliminating the unsightly cast in nodeCustom.c? > > > That'd change an "extension API", which is why I skipped it at this > > point of the release cycle. It's not like we didn't have this type of > > cast all over before. Ok, with changing it, but that's where I came > > down. > > Is this patch really not changing anything else that a custom-scan > extension would touch? If not, I'm okay with postponing this bit > of cleanup to v11. FWIW, I've reintroduced ExecCustomScan() which I'd previously removed, because it now contains a CHECK_FOR_INTERRUPTS(). So this seems moot. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Andres Freund <andres@anarazel.de> writes:
> I've moved the CHECK_FOR_INTERRUPTS() to the callsites. That
> unsurprisingly ends up being somewhat verbose, and there's a bunch of
> minor judgement calls where exactly to place them. While doing so I've
> also added a few extra ones. Did this in a separate patch to make it
> easier to review.
Hm, that seems kinda backwards to me; I was envisioning the checks
moving to the callees not the callers. I think it'd end up being
about the same number of occurrences of CHECK_FOR_INTERRUPTS(),
and there would be less of a judgment call about where to put them.
regards, tom lane
On 2017-07-26 15:03:37 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > I've moved the CHECK_FOR_INTERRUPTS() to the callsites. That > > unsurprisingly ends up being somewhat verbose, and there's a bunch of > > minor judgement calls where exactly to place them. While doing so I've > > also added a few extra ones. Did this in a separate patch to make it > > easier to review. > > Hm, that seems kinda backwards to me; I was envisioning the checks > moving to the callees not the callers. I think it'd end up being > about the same number of occurrences of CHECK_FOR_INTERRUPTS(), > and there would be less of a judgment call about where to put them. Hm, that seems a bit riskier - easy to forget one of the places where we might need a CFI(). We certainly are missing a bunch of them in various nodes - I tried to add ones I saw as missing, but it's quite some code. Keeping them close to ExecProcNode() makes that call easier. I'm not quite seing how solely putting them in callees removes the judgement call issue? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2017-07-26 15:03:37 -0400, Tom Lane wrote:
>> Hm, that seems kinda backwards to me; I was envisioning the checks
>> moving to the callees not the callers. I think it'd end up being
>> about the same number of occurrences of CHECK_FOR_INTERRUPTS(),
>> and there would be less of a judgment call about where to put them.
> Hm, that seems a bit riskier - easy to forget one of the places where we
> might need a CFI().
I would argue the contrary. If we put a CFI at the head of each node
execution function, then it's just boilerplate that you copy-and-paste
when you invent a new node type. The way you've coded it here, it
seems to involve a lot of judgment calls. That's very far from being
copy and paste, and the more different it looks from one node type
to another, the easier it will be to forget it.
> We certainly are missing a bunch of them in various nodes
It's certainly possible that there are long-running loops not involving
any ExecProcNode recursion at all, but that would be a bug independent
of this issue. The CFI in ExecProcNode itself can be replaced exactly
either by asking all callers to do it, or by asking all callees to do it.
I think the latter is going to be more uniform and harder to screw up.
regards, tom lane
On Mon, Jul 24, 2017 at 08:04:30AM +0100, Andres Freund wrote: > > > On July 24, 2017 7:10:19 AM GMT+01:00, Noah Misch <noah@leadboat.com> wrote: > >On Tue, Jul 18, 2017 at 01:04:10PM -0700, Andres Freund wrote: > >> Ok, I'll flesh out the patch till Thursday. But I do think we're > >going > >> to have to do something about the back branches, too. > > > >This PostgreSQL 10 open item is past due for your status update. > >Kindly send > >a status update within 24 hours, and include a date for your subsequent > >status > >update. Refer to the policy on open item ownership: > >https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > I sent out a note fleshed out patch last week, which Tom reviewed. Planning to update it to address that review today ortomorrow. This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On 2017-07-26 16:28:38 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2017-07-26 15:03:37 -0400, Tom Lane wrote: > >> Hm, that seems kinda backwards to me; I was envisioning the checks > >> moving to the callees not the callers. I think it'd end up being > >> about the same number of occurrences of CHECK_FOR_INTERRUPTS(), > >> and there would be less of a judgment call about where to put them. > > > Hm, that seems a bit riskier - easy to forget one of the places where we > > might need a CFI(). > > I would argue the contrary. If we put a CFI at the head of each node > execution function, then it's just boilerplate that you copy-and-paste > when you invent a new node type. The way you've coded it here, it > seems to involve a lot of judgment calls. That's very far from being > copy and paste, and the more different it looks from one node type > to another, the easier it will be to forget it. > > > We certainly are missing a bunch of them in various nodes > > It's certainly possible that there are long-running loops not involving > any ExecProcNode recursion at all, but that would be a bug independent > of this issue. The CFI in ExecProcNode itself can be replaced exactly > either by asking all callers to do it, or by asking all callees to do it. > I think the latter is going to be more uniform and harder to screw up. Looks a bit better. Still a lot of judgement-y calls tho, e.g. when one node function just calls the next, or when there's loops etc. I found a good number of missing CFIs... What do you think? - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Thu, Jul 27, 2017 at 02:29:32AM +0000, Noah Misch wrote: > On Mon, Jul 24, 2017 at 08:04:30AM +0100, Andres Freund wrote: > > > > > > On July 24, 2017 7:10:19 AM GMT+01:00, Noah Misch <noah@leadboat.com> wrote: > > >On Tue, Jul 18, 2017 at 01:04:10PM -0700, Andres Freund wrote: > > >> Ok, I'll flesh out the patch till Thursday. But I do think we're > > >going > > >> to have to do something about the back branches, too. > > > > > >This PostgreSQL 10 open item is past due for your status update. > > >Kindly send > > >a status update within 24 hours, and include a date for your subsequent > > >status > > >update. Refer to the policy on open item ownership: > > >https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > I sent out a note fleshed out patch last week, which Tom reviewed. Planning to update it to address that review todayor tomorrow. > > This PostgreSQL 10 open item is past due for your status update. Kindly send > a status update within 24 hours, and include a date for your subsequent status > update. Refer to the policy on open item ownership: > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due for your status update. Please reacquaint yourself with the policy on open item ownership[1] and then reply immediately. If I do not hear from you by 2017-07-29 05:00 UTC, I will transfer this item to release management team ownership without further notice. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On 2017-07-27 21:46:57 -0700, Noah Misch wrote: > On Thu, Jul 27, 2017 at 02:29:32AM +0000, Noah Misch wrote: > > On Mon, Jul 24, 2017 at 08:04:30AM +0100, Andres Freund wrote: > > > > > > > > > On July 24, 2017 7:10:19 AM GMT+01:00, Noah Misch <noah@leadboat.com> wrote: > > > >On Tue, Jul 18, 2017 at 01:04:10PM -0700, Andres Freund wrote: > > > >> Ok, I'll flesh out the patch till Thursday. But I do think we're > > > >going > > > >> to have to do something about the back branches, too. > > > > > > > >This PostgreSQL 10 open item is past due for your status update. > > > >Kindly send > > > >a status update within 24 hours, and include a date for your subsequent > > > >status > > > >update. Refer to the policy on open item ownership: > > > >https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > > > I sent out a note fleshed out patch last week, which Tom reviewed. Planning to update it to address that review todayor tomorrow. > > > > This PostgreSQL 10 open item is past due for your status update. Kindly send > > a status update within 24 hours, and include a date for your subsequent status > > update. Refer to the policy on open item ownership: > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due > for your status update. Please reacquaint yourself with the policy on open > item ownership[1] and then reply immediately. If I do not hear from you by > 2017-07-29 05:00 UTC, I will transfer this item to release management team > ownership without further notice. > > [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com I've updated the patch based on review (today). Awaiting new review. FWIW, I don't see the point of these messages when there is a new patch version posted today.
On Thu, Jul 27, 2017 at 09:49:18PM -0700, Andres Freund wrote: > On 2017-07-27 21:46:57 -0700, Noah Misch wrote: > > On Thu, Jul 27, 2017 at 02:29:32AM +0000, Noah Misch wrote: > > > On Mon, Jul 24, 2017 at 08:04:30AM +0100, Andres Freund wrote: > > > > > > > > > > > > On July 24, 2017 7:10:19 AM GMT+01:00, Noah Misch <noah@leadboat.com> wrote: > > > > >On Tue, Jul 18, 2017 at 01:04:10PM -0700, Andres Freund wrote: > > > > >> Ok, I'll flesh out the patch till Thursday. But I do think we're > > > > >going > > > > >> to have to do something about the back branches, too. > > > > > > > > > >This PostgreSQL 10 open item is past due for your status update. > > > > >Kindly send > > > > >a status update within 24 hours, and include a date for your subsequent > > > > >status > > > > >update. Refer to the policy on open item ownership: > > > > >https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > > > > > I sent out a note fleshed out patch last week, which Tom reviewed. Planning to update it to address that review todayor tomorrow. > > > > > > This PostgreSQL 10 open item is past due for your status update. Kindly send > > > a status update within 24 hours, and include a date for your subsequent status > > > update. Refer to the policy on open item ownership: > > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due > > for your status update. Please reacquaint yourself with the policy on open > > item ownership[1] and then reply immediately. If I do not hear from you by > > 2017-07-29 05:00 UTC, I will transfer this item to release management team > > ownership without further notice. > > > > [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > I've updated the patch based on review (today). Awaiting new review. > > FWIW, I don't see the point of these messages when there is a new patch > version posted today. The policy says, "Each update shall state a date when the community will receive another update". Nothing you've sent today specifies a deadline for your next update, so your ownership of this item remains out of compliance.
On 2017-07-27 22:04:59 -0700, Noah Misch wrote: > On Thu, Jul 27, 2017 at 09:49:18PM -0700, Andres Freund wrote: > > On 2017-07-27 21:46:57 -0700, Noah Misch wrote: > > > On Thu, Jul 27, 2017 at 02:29:32AM +0000, Noah Misch wrote: > > > > On Mon, Jul 24, 2017 at 08:04:30AM +0100, Andres Freund wrote: > > > > > > > > > > > > > > > On July 24, 2017 7:10:19 AM GMT+01:00, Noah Misch <noah@leadboat.com> wrote: > > > > > >On Tue, Jul 18, 2017 at 01:04:10PM -0700, Andres Freund wrote: > > > > > >> Ok, I'll flesh out the patch till Thursday. But I do think we're > > > > > >going > > > > > >> to have to do something about the back branches, too. > > > > > > > > > > > >This PostgreSQL 10 open item is past due for your status update. > > > > > >Kindly send > > > > > >a status update within 24 hours, and include a date for your subsequent > > > > > >status > > > > > >update. Refer to the policy on open item ownership: > > > > > >https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > > > > > > > I sent out a note fleshed out patch last week, which Tom reviewed. Planning to update it to address that reviewtoday or tomorrow. > > > > > > > > This PostgreSQL 10 open item is past due for your status update. Kindly send > > > > a status update within 24 hours, and include a date for your subsequent status > > > > update. Refer to the policy on open item ownership: > > > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > > > IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due > > > for your status update. Please reacquaint yourself with the policy on open > > > item ownership[1] and then reply immediately. If I do not hear from you by > > > 2017-07-29 05:00 UTC, I will transfer this item to release management team > > > ownership without further notice. > > > > > > [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > I've updated the patch based on review (today). Awaiting new review. > > > > FWIW, I don't see the point of these messages when there is a new patch > > version posted today. > > The policy says, "Each update shall state a date when the community will > receive another update". Nothing you've sent today specifies a deadline for > your next update, so your ownership of this item remains out of > compliance. For me that means the policy isn't quite right. It's not like I can force Tom to review the patch at a specific date. But the thread has been progressing steadily over the last days, so I'm not particularly concerned. Greetings, Andres Freund
On Thu, Jul 27, 2017 at 10:08:57PM -0700, Andres Freund wrote: > On 2017-07-27 22:04:59 -0700, Noah Misch wrote: > > On Thu, Jul 27, 2017 at 09:49:18PM -0700, Andres Freund wrote: > > > On 2017-07-27 21:46:57 -0700, Noah Misch wrote: > > > > On Thu, Jul 27, 2017 at 02:29:32AM +0000, Noah Misch wrote: > > > > > On Mon, Jul 24, 2017 at 08:04:30AM +0100, Andres Freund wrote: > > > > > > > > > > > > > > > > > > On July 24, 2017 7:10:19 AM GMT+01:00, Noah Misch <noah@leadboat.com> wrote: > > > > > > >On Tue, Jul 18, 2017 at 01:04:10PM -0700, Andres Freund wrote: > > > > > > >> Ok, I'll flesh out the patch till Thursday. But I do think we're > > > > > > >going > > > > > > >> to have to do something about the back branches, too. > > > > > > > > > > > > > >This PostgreSQL 10 open item is past due for your status update. > > > > > > >Kindly send > > > > > > >a status update within 24 hours, and include a date for your subsequent > > > > > > >status > > > > > > >update. Refer to the policy on open item ownership: > > > > > > >https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > > > > > > > > > I sent out a note fleshed out patch last week, which Tom reviewed. Planning to update it to address that reviewtoday or tomorrow. > > > > > > > > > > This PostgreSQL 10 open item is past due for your status update. Kindly send > > > > > a status update within 24 hours, and include a date for your subsequent status > > > > > update. Refer to the policy on open item ownership: > > > > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > > > > > IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due > > > > for your status update. Please reacquaint yourself with the policy on open > > > > item ownership[1] and then reply immediately. If I do not hear from you by > > > > 2017-07-29 05:00 UTC, I will transfer this item to release management team > > > > ownership without further notice. > > > > > > > > [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > > > I've updated the patch based on review (today). Awaiting new review. > > > > > > FWIW, I don't see the point of these messages when there is a new patch > > > version posted today. > > > > The policy says, "Each update shall state a date when the community will > > receive another update". Nothing you've sent today specifies a deadline for > > your next update, so your ownership of this item remains out of > > compliance. > > For me that means the policy isn't quite right. It's not like I can > force Tom to review the patch at a specific date. But the thread has > been progressing steadily over the last days, so I'm not particularly > concerned. Your colleagues achieve compliance despite uncertainty; for inspiration, I recommend examining Alvaro's status updates as examples of this. The policy currently governs your open items even if you disagree with it.
On 2017-07-28 09:29:58 -0700, Noah Misch wrote: > On Thu, Jul 27, 2017 at 10:08:57PM -0700, Andres Freund wrote: > > For me that means the policy isn't quite right. It's not like I can > > force Tom to review the patch at a specific date. But the thread has > > been progressing steadily over the last days, so I'm not particularly > > concerned. > > Your colleagues achieve compliance despite uncertainty; for inspiration, I > recommend examining Alvaro's status updates as examples of this. The policy > currently governs your open items even if you disagree with it. That's just process over substance. Publishing repeated "I'll look at it again in $small_n days" doesn't really provide something useful. Anyway, I'll commit it after another pass in ~1 week if it doesn't get a review till then, but I assume it'll. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Anyway, I'll commit it after another pass in ~1 week if it doesn't get a
> review till then, but I assume it'll.
FWIW, I intend to review it today, or tomorrow at the very latest.
(Right now I'm buried in perl droppings.)
regards, tom lane
On Fri, Jul 28, 2017 at 12:29 PM, Noah Misch <noah@leadboat.com> wrote: > Your colleagues achieve compliance despite uncertainty; for inspiration, I > recommend examining Alvaro's status updates as examples of this. The policy > currently governs your open items even if you disagree with it. I emphatically agree with that. If the RMT is to accomplish its purpose, it must be able to exert authority even when an individual contributor doesn't like the decisions it makes. On the other hand, nothing in the open item policy the current RMT has adopted prohibits you from using judgement about when and how vigorously to enforce that policy in any particular case, and I would encourage you to do so. It didn't make much sense to keep sending Kevin increasingly strident form letters about each individual item when he wasn't responding to any emails at all, and it makes equally little sense to me to nag someone over a technical failure to include a date when things are obviously progressing adequately. As Andres quite rightly says downthread: > That's just process over substance. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Andres Freund <andres@anarazel.de> writes:
> On 2017-07-26 16:28:38 -0400, Tom Lane wrote:
>> It's certainly possible that there are long-running loops not involving
>> any ExecProcNode recursion at all, but that would be a bug independent
>> of this issue. The CFI in ExecProcNode itself can be replaced exactly
>> either by asking all callers to do it, or by asking all callees to do it.
>> I think the latter is going to be more uniform and harder to screw up.
> Looks a bit better. Still a lot of judgement-y calls tho, e.g. when one
> node function just calls the next, or when there's loops etc. I found
> a good number of missing CFIs...
> What do you think?
Here's a reviewed version of this patch. Differences from yours:
* I think you put ExecScan's CFI in the wrong place; AFAICT yours
only covers its fast path.
* I think ExecAgg needs a CFI at the head, just to be sure it's hit
in any path through that.
* I agree that putting CFI inside ExecHashJoin's state machine loop
is a good idea, because it might have to trawl through quite a lot of
a batch file before finding a returnable tuple. But I think in merge
and nestloop joins it's sufficient to put one CFI at the head. Neither
of those cases can do very much processing without invoking a child
node, where a CFI will happen.
* You missed ExecLockRows altogether.
* I added some comments and cosmetic tweaks.
regards, tom lane
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index 294ad2c..20fd9f8 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -399,8 +399,6 @@ ExecProcNode(PlanState *node)
{
TupleTableSlot *result;
- CHECK_FOR_INTERRUPTS();
-
if (node->chgParam != NULL) /* something changed */
ExecReScan(node); /* let ReScan handle this */
diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index 4f131b3..6fde7cd 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -126,6 +126,8 @@ ExecScan(ScanState *node,
ExprState *qual;
ProjectionInfo *projInfo;
+ CHECK_FOR_INTERRUPTS();
+
/*
* Fetch data from node
*/
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index de9a18e..377916d 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -677,6 +677,8 @@ fetch_input_tuple(AggState *aggstate)
if (aggstate->sort_in)
{
+ /* make sure we check for interrupts in either path through here */
+ CHECK_FOR_INTERRUPTS();
if (!tuplesort_gettupleslot(aggstate->sort_in, true, false,
aggstate->sort_slot, NULL))
return NULL;
@@ -1414,6 +1416,8 @@ process_ordered_aggregate_multi(AggState *aggstate,
while (tuplesort_gettupleslot(pertrans->sortstates[aggstate->current_set],
true, true, slot1, &newAbbrevVal))
{
+ CHECK_FOR_INTERRUPTS();
+
/*
* Extract the first numTransInputs columns as datums to pass to the
* transfn. (This will help execTuplesMatch too, so we do it
@@ -2100,6 +2104,8 @@ ExecAgg(AggState *node)
{
TupleTableSlot *result = NULL;
+ CHECK_FOR_INTERRUPTS();
+
if (!node->agg_done)
{
/* Dispatch based on strategy */
@@ -2563,6 +2569,8 @@ agg_retrieve_hash_table(AggState *aggstate)
TupleTableSlot *hashslot = perhash->hashslot;
int i;
+ CHECK_FOR_INTERRUPTS();
+
/*
* Find the next entry in the hash table
*/
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index aae5e3f..58045e0 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -59,6 +59,7 @@
#include "executor/execdebug.h"
#include "executor/nodeAppend.h"
+#include "miscadmin.h"
static bool exec_append_initialize_next(AppendState *appendstate);
@@ -204,6 +205,8 @@ ExecAppend(AppendState *node)
PlanState *subnode;
TupleTableSlot *result;
+ CHECK_FOR_INTERRUPTS();
+
/*
* figure out which subplan we are currently processing
*/
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 7e0ba03..cf109d5 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -41,6 +41,7 @@
#include "access/transam.h"
#include "executor/execdebug.h"
#include "executor/nodeBitmapHeapscan.h"
+#include "miscadmin.h"
#include "pgstat.h"
#include "storage/bufmgr.h"
#include "storage/predicate.h"
@@ -192,6 +193,8 @@ BitmapHeapNext(BitmapHeapScanState *node)
Page dp;
ItemId lp;
+ CHECK_FOR_INTERRUPTS();
+
/*
* Get next page of results if needed
*/
diff --git a/src/backend/executor/nodeCustom.c b/src/backend/executor/nodeCustom.c
index 69e2704..fc15974 100644
--- a/src/backend/executor/nodeCustom.c
+++ b/src/backend/executor/nodeCustom.c
@@ -15,6 +15,7 @@
#include "executor/nodeCustom.h"
#include "nodes/execnodes.h"
#include "nodes/plannodes.h"
+#include "miscadmin.h"
#include "parser/parsetree.h"
#include "utils/hsearch.h"
#include "utils/memutils.h"
@@ -104,6 +105,8 @@ ExecInitCustomScan(CustomScan *cscan, EState *estate, int eflags)
TupleTableSlot *
ExecCustomScan(CustomScanState *node)
{
+ CHECK_FOR_INTERRUPTS();
+
Assert(node->methods->ExecCustomScan != NULL);
return node->methods->ExecCustomScan(node);
}
diff --git a/src/backend/executor/nodeGather.c b/src/backend/executor/nodeGather.c
index f83cd58..5dbe19c 100644
--- a/src/backend/executor/nodeGather.c
+++ b/src/backend/executor/nodeGather.c
@@ -128,6 +128,8 @@ ExecGather(GatherState *node)
TupleTableSlot *slot;
ExprContext *econtext;
+ CHECK_FOR_INTERRUPTS();
+
/*
* Initialize the parallel context and workers on first execution. We do
* this on first execution rather than during node initialization, as it
@@ -247,6 +249,8 @@ gather_getnext(GatherState *gatherstate)
while (gatherstate->reader != NULL || gatherstate->need_to_scan_locally)
{
+ CHECK_FOR_INTERRUPTS();
+
if (gatherstate->reader != NULL)
{
MemoryContext oldContext;
diff --git a/src/backend/executor/nodeGatherMerge.c b/src/backend/executor/nodeGatherMerge.c
index 80ee1fc..0aff379 100644
--- a/src/backend/executor/nodeGatherMerge.c
+++ b/src/backend/executor/nodeGatherMerge.c
@@ -164,6 +164,8 @@ ExecGatherMerge(GatherMergeState *node)
ExprContext *econtext;
int i;
+ CHECK_FOR_INTERRUPTS();
+
/*
* As with Gather, we don't launch workers until this node is actually
* executed.
@@ -393,6 +395,8 @@ gather_merge_init(GatherMergeState *gm_state)
reread:
for (i = 0; i < nreaders + 1; i++)
{
+ CHECK_FOR_INTERRUPTS();
+
if (!gm_state->gm_tuple_buffers[i].done &&
(TupIsNull(gm_state->gm_slots[i]) ||
gm_state->gm_slots[i]->tts_isempty))
diff --git a/src/backend/executor/nodeGroup.c b/src/backend/executor/nodeGroup.c
index af9ba49..fc5e0e5 100644
--- a/src/backend/executor/nodeGroup.c
+++ b/src/backend/executor/nodeGroup.c
@@ -24,6 +24,7 @@
#include "executor/executor.h"
#include "executor/nodeGroup.h"
+#include "miscadmin.h"
/*
@@ -40,6 +41,8 @@ ExecGroup(GroupState *node)
TupleTableSlot *firsttupleslot;
TupleTableSlot *outerslot;
+ CHECK_FOR_INTERRUPTS();
+
/*
* get state info from node
*/
diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 075f4ed..fbeb562 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -810,6 +810,9 @@ ExecHashIncreaseNumBuckets(HashJoinTable hashtable)
idx += MAXALIGN(HJTUPLE_OVERHEAD +
HJTUPLE_MINTUPLE(hashTuple)->t_len);
}
+
+ /* allow this loop to be cancellable */
+ CHECK_FOR_INTERRUPTS();
}
}
@@ -1192,6 +1195,9 @@ ExecScanHashTableForUnmatched(HashJoinState *hjstate, ExprContext *econtext)
hashTuple = hashTuple->next;
}
+
+ /* allow this loop to be cancellable */
+ CHECK_FOR_INTERRUPTS();
}
/*
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 668ed87..252960c 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -92,6 +92,14 @@ ExecHashJoin(HashJoinState *node)
*/
for (;;)
{
+ /*
+ * It's possible to iterate this loop many times before returning a
+ * tuple, in some pathological cases such as needing to move much of
+ * the current batch to a later batch. So let's check for interrupts
+ * each time through.
+ */
+ CHECK_FOR_INTERRUPTS();
+
switch (node->hj_JoinState)
{
case HJ_BUILD_HASHTABLE:
@@ -247,13 +255,6 @@ ExecHashJoin(HashJoinState *node)
case HJ_SCAN_BUCKET:
/*
- * We check for interrupts here because this corresponds to
- * where we'd fetch a row from a child plan node in other join
- * types.
- */
- CHECK_FOR_INTERRUPTS();
-
- /*
* Scan the selected hash bucket for matches to current outer
*/
if (!ExecScanHashBucket(node, econtext))
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index 890e544..e2000764 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -34,6 +34,7 @@
#include "executor/execdebug.h"
#include "executor/nodeIndexonlyscan.h"
#include "executor/nodeIndexscan.h"
+#include "miscadmin.h"
#include "storage/bufmgr.h"
#include "storage/predicate.h"
#include "utils/memutils.h"
@@ -117,6 +118,8 @@ IndexOnlyNext(IndexOnlyScanState *node)
{
HeapTuple tuple = NULL;
+ CHECK_FOR_INTERRUPTS();
+
/*
* We can skip the heap fetch if the TID references a heap page on
* which all tuples are known visible to everybody. In any case,
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index 75b1011..6704ede 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -34,6 +34,7 @@
#include "executor/execdebug.h"
#include "executor/nodeIndexscan.h"
#include "lib/pairingheap.h"
+#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/clauses.h"
#include "utils/array.h"
@@ -131,6 +132,8 @@ IndexNext(IndexScanState *node)
*/
while ((tuple = index_getnext(scandesc, direction)) != NULL)
{
+ CHECK_FOR_INTERRUPTS();
+
/*
* Store the scanned tuple in the scan tuple slot of the scan state.
* Note: we pass 'false' because tuples returned by amgetnext are
@@ -233,6 +236,8 @@ IndexNextWithReorder(IndexScanState *node)
for (;;)
{
+ CHECK_FOR_INTERRUPTS();
+
/*
* Check the reorder queue first. If the topmost tuple in the queue
* has an ORDER BY value smaller than (or equal to) the value last
@@ -299,6 +304,8 @@ next_indextuple:
{
/* Fails recheck, so drop it and loop back for another */
InstrCountFiltered2(node, 1);
+ /* allow this loop to be cancellable */
+ CHECK_FOR_INTERRUPTS();
goto next_indextuple;
}
}
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index abd060d..2ed3523 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -23,6 +23,7 @@
#include "executor/executor.h"
#include "executor/nodeLimit.h"
+#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
static void recompute_limits(LimitState *node);
@@ -43,6 +44,8 @@ ExecLimit(LimitState *node)
TupleTableSlot *slot;
PlanState *outerPlan;
+ CHECK_FOR_INTERRUPTS();
+
/*
* get information from the node
*/
diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c
index f519794..dd4e2c5 100644
--- a/src/backend/executor/nodeLockRows.c
+++ b/src/backend/executor/nodeLockRows.c
@@ -26,6 +26,7 @@
#include "executor/executor.h"
#include "executor/nodeLockRows.h"
#include "foreign/fdwapi.h"
+#include "miscadmin.h"
#include "storage/bufmgr.h"
#include "utils/rel.h"
#include "utils/tqual.h"
@@ -44,6 +45,8 @@ ExecLockRows(LockRowsState *node)
bool epq_needed;
ListCell *lc;
+ CHECK_FOR_INTERRUPTS();
+
/*
* get information from the node
*/
diff --git a/src/backend/executor/nodeMaterial.c b/src/backend/executor/nodeMaterial.c
index 32b7269..3342949 100644
--- a/src/backend/executor/nodeMaterial.c
+++ b/src/backend/executor/nodeMaterial.c
@@ -45,6 +45,8 @@ ExecMaterial(MaterialState *node)
bool eof_tuplestore;
TupleTableSlot *slot;
+ CHECK_FOR_INTERRUPTS();
+
/*
* get state info from node
*/
diff --git a/src/backend/executor/nodeMergeAppend.c b/src/backend/executor/nodeMergeAppend.c
index fef83db..d41def1 100644
--- a/src/backend/executor/nodeMergeAppend.c
+++ b/src/backend/executor/nodeMergeAppend.c
@@ -40,8 +40,8 @@
#include "executor/execdebug.h"
#include "executor/nodeMergeAppend.h"
-
#include "lib/binaryheap.h"
+#include "miscadmin.h"
/*
* We have one slot for each item in the heap array. We use SlotNumber
@@ -175,6 +175,8 @@ ExecMergeAppend(MergeAppendState *node)
TupleTableSlot *result;
SlotNumber i;
+ CHECK_FOR_INTERRUPTS();
+
if (!node->ms_initialized)
{
/*
diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
index 6a145ee..324b61b 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -95,6 +95,7 @@
#include "access/nbtree.h"
#include "executor/execdebug.h"
#include "executor/nodeMergejoin.h"
+#include "miscadmin.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -610,6 +611,8 @@ ExecMergeJoin(MergeJoinState *node)
bool doFillOuter;
bool doFillInner;
+ CHECK_FOR_INTERRUPTS();
+
/*
* get information from node
*/
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 77ba15d..637a582 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1551,6 +1551,8 @@ ExecModifyTable(ModifyTableState *node)
HeapTupleData oldtupdata;
HeapTuple oldtuple;
+ CHECK_FOR_INTERRUPTS();
+
/*
* This should NOT get called during EvalPlanQual; we should have passed a
* subplan tree to EvalPlanQual, instead. Use a runtime test not just
diff --git a/src/backend/executor/nodeNestloop.c b/src/backend/executor/nodeNestloop.c
index 0065fe6..bedc374 100644
--- a/src/backend/executor/nodeNestloop.c
+++ b/src/backend/executor/nodeNestloop.c
@@ -23,6 +23,7 @@
#include "executor/execdebug.h"
#include "executor/nodeNestloop.h"
+#include "miscadmin.h"
#include "utils/memutils.h"
@@ -69,6 +70,8 @@ ExecNestLoop(NestLoopState *node)
ExprContext *econtext;
ListCell *lc;
+ CHECK_FOR_INTERRUPTS();
+
/*
* get information from the node
*/
diff --git a/src/backend/executor/nodeProjectSet.c b/src/backend/executor/nodeProjectSet.c
index 01048cc..3b69c7a 100644
--- a/src/backend/executor/nodeProjectSet.c
+++ b/src/backend/executor/nodeProjectSet.c
@@ -24,6 +24,7 @@
#include "executor/executor.h"
#include "executor/nodeProjectSet.h"
+#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
#include "utils/memutils.h"
@@ -46,6 +47,8 @@ ExecProjectSet(ProjectSetState *node)
PlanState *outerPlan;
ExprContext *econtext;
+ CHECK_FOR_INTERRUPTS();
+
econtext = node->ps.ps_ExprContext;
/*
diff --git a/src/backend/executor/nodeRecursiveunion.c b/src/backend/executor/nodeRecursiveunion.c
index fc1c00d..2802fff 100644
--- a/src/backend/executor/nodeRecursiveunion.c
+++ b/src/backend/executor/nodeRecursiveunion.c
@@ -75,6 +75,8 @@ ExecRecursiveUnion(RecursiveUnionState *node)
TupleTableSlot *slot;
bool isnew;
+ CHECK_FOR_INTERRUPTS();
+
/* 1. Evaluate non-recursive term */
if (!node->recursing)
{
diff --git a/src/backend/executor/nodeResult.c b/src/backend/executor/nodeResult.c
index a753a53..f007f46 100644
--- a/src/backend/executor/nodeResult.c
+++ b/src/backend/executor/nodeResult.c
@@ -47,6 +47,7 @@
#include "executor/executor.h"
#include "executor/nodeResult.h"
+#include "miscadmin.h"
#include "utils/memutils.h"
@@ -70,6 +71,8 @@ ExecResult(ResultState *node)
PlanState *outerPlan;
ExprContext *econtext;
+ CHECK_FOR_INTERRUPTS();
+
econtext = node->ps.ps_ExprContext;
/*
diff --git a/src/backend/executor/nodeSetOp.c b/src/backend/executor/nodeSetOp.c
index 9c7812e..56c5643 100644
--- a/src/backend/executor/nodeSetOp.c
+++ b/src/backend/executor/nodeSetOp.c
@@ -47,6 +47,7 @@
#include "access/htup_details.h"
#include "executor/executor.h"
#include "executor/nodeSetOp.h"
+#include "miscadmin.h"
#include "utils/memutils.h"
@@ -185,6 +186,8 @@ ExecSetOp(SetOpState *node)
SetOp *plannode = (SetOp *) node->ps.plan;
TupleTableSlot *resultTupleSlot = node->ps.ps_ResultTupleSlot;
+ CHECK_FOR_INTERRUPTS();
+
/*
* If the previously-returned tuple needs to be returned more than once,
* keep returning it.
@@ -428,6 +431,8 @@ setop_retrieve_hash_table(SetOpState *setopstate)
*/
while (!setopstate->setop_done)
{
+ CHECK_FOR_INTERRUPTS();
+
/*
* Find the next entry in the hash table
*/
diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
index 924b458..799a4e9 100644
--- a/src/backend/executor/nodeSort.c
+++ b/src/backend/executor/nodeSort.c
@@ -43,6 +43,8 @@ ExecSort(SortState *node)
Tuplesortstate *tuplesortstate;
TupleTableSlot *slot;
+ CHECK_FOR_INTERRUPTS();
+
/*
* get state info from node
*/
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index e8fa4c8..fe10e80 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -33,6 +33,7 @@
#include "executor/executor.h"
#include "executor/nodeSubplan.h"
#include "nodes/makefuncs.h"
+#include "miscadmin.h"
#include "optimizer/clauses.h"
#include "utils/array.h"
#include "utils/lsyscache.h"
@@ -65,6 +66,8 @@ ExecSubPlan(SubPlanState *node,
{
SubPlan *subplan = node->subplan;
+ CHECK_FOR_INTERRUPTS();
+
/* Set non-null as default */
*isNull = false;
@@ -618,6 +621,8 @@ findPartialMatch(TupleHashTable hashtable, TupleTableSlot *slot,
InitTupleHashIterator(hashtable, &hashiter);
while ((entry = ScanTupleHashTable(hashtable, &hashiter)) != NULL)
{
+ CHECK_FOR_INTERRUPTS();
+
ExecStoreMinimalTuple(entry->firstTuple, hashtable->tableslot, false);
if (!execTuplesUnequal(slot, hashtable->tableslot,
numCols, keyColIdx,
diff --git a/src/backend/executor/nodeTableFuncscan.c b/src/backend/executor/nodeTableFuncscan.c
index bb016ec..2859363 100644
--- a/src/backend/executor/nodeTableFuncscan.c
+++ b/src/backend/executor/nodeTableFuncscan.c
@@ -440,6 +440,8 @@ tfuncLoadRows(TableFuncScanState *tstate, ExprContext *econtext)
ListCell *cell = list_head(tstate->coldefexprs);
int colno;
+ CHECK_FOR_INTERRUPTS();
+
ExecClearTuple(tstate->ss.ss_ScanTupleSlot);
/*
diff --git a/src/backend/executor/nodeTidscan.c b/src/backend/executor/nodeTidscan.c
index 96af2d2..c122473 100644
--- a/src/backend/executor/nodeTidscan.c
+++ b/src/backend/executor/nodeTidscan.c
@@ -26,6 +26,7 @@
#include "catalog/pg_type.h"
#include "executor/execdebug.h"
#include "executor/nodeTidscan.h"
+#include "miscadmin.h"
#include "optimizer/clauses.h"
#include "storage/bufmgr.h"
#include "utils/array.h"
@@ -400,6 +401,8 @@ TidNext(TidScanState *node)
node->tss_TidPtr--;
else
node->tss_TidPtr++;
+
+ CHECK_FOR_INTERRUPTS();
}
/*
diff --git a/src/backend/executor/nodeUnique.c b/src/backend/executor/nodeUnique.c
index 28cc1e9..db78c88 100644
--- a/src/backend/executor/nodeUnique.c
+++ b/src/backend/executor/nodeUnique.c
@@ -35,6 +35,7 @@
#include "executor/executor.h"
#include "executor/nodeUnique.h"
+#include "miscadmin.h"
#include "utils/memutils.h"
@@ -50,6 +51,8 @@ ExecUnique(UniqueState *node)
TupleTableSlot *slot;
PlanState *outerPlan;
+ CHECK_FOR_INTERRUPTS();
+
/*
* get information from the node
*/
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 8f13fe0..9da35ac 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -1594,6 +1594,8 @@ ExecWindowAgg(WindowAggState *winstate)
int i;
int numfuncs;
+ CHECK_FOR_INTERRUPTS();
+
if (winstate->all_done)
return NULL;
@@ -2371,6 +2373,9 @@ window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot)
WindowAggState *winstate = winobj->winstate;
MemoryContext oldcontext;
+ /* often called repeatedly in a row */
+ CHECK_FOR_INTERRUPTS();
+
/* Don't allow passing -1 to spool_tuples here */
if (pos < 0)
return false;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-07-29 14:20:32 -0400, Tom Lane wrote: > Here's a reviewed version of this patch. Thanks! > * I think you put ExecScan's CFI in the wrong place; AFAICT yours > only covers its fast path. Sure - but the old path already has a CFI? And it has to be inside the loop, because well, the loop ;). > * I think ExecAgg needs a CFI at the head, just to be sure it's hit > in any path through that. Yep, makes esense. > * I agree that putting CFI inside ExecHashJoin's state machine loop > is a good idea, because it might have to trawl through quite a lot of > a batch file before finding a returnable tuple. But I think in merge > and nestloop joins it's sufficient to put one CFI at the head. Neither > of those cases can do very much processing without invoking a child > node, where a CFI will happen. Ok, I can live with that. > * You missed ExecLockRows altogether. Well, it directly calls the next ExecProcNode(), so I didn't think it was necessary. One of the aforementioned judgement calls. But I'm perfectly happy to have one there. Thanks, Andres
Andres Freund <andres@anarazel.de> writes:
> [ 0002-Move-ExecProcNode-from-dispatch-to-function-pointer-.patch ]
Here's a reviewed version of this patch.
I added dummy ExecProcNodeMtd functions to the various node types that
lacked them because they expect to be called through MultiExecProcNode
instead. In the old coding, trying to call ExecProcNode on one of those
node types would have led to a useful error message; as you had it,
it'd have dumped core, which is not an improvement.
Also, I removed the ExecReScan stanza from ExecProcNodeFirst; that
should surely be redundant, because we should only get to that function
through ExecProcNode(). If somehow it's not redundant, please add a
comment explaining why not.
Some other minor cosmetic changes, mostly comment wordsmithing.
I think this (and the previous one) are committable.
regards, tom lane
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index 20fd9f8..d338cfe 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -17,15 +17,10 @@
*-------------------------------------------------------------------------
*/
/*
- * INTERFACE ROUTINES
- * ExecInitNode - initialize a plan node and its subplans
- * ExecProcNode - get a tuple by executing the plan node
- * ExecEndNode - shut down a plan node and its subplans
- *
* NOTES
* This used to be three files. It is now all combined into
- * one file so that it is easier to keep ExecInitNode, ExecProcNode,
- * and ExecEndNode in sync when new nodes are added.
+ * one file so that it is easier to keep the dispatch routines
+ * in sync when new nodes are added.
*
* EXAMPLE
* Suppose we want the age of the manager of the shoe department and
@@ -122,6 +117,10 @@
#include "miscadmin.h"
+static TupleTableSlot *ExecProcNodeFirst(PlanState *node);
+static TupleTableSlot *ExecProcNodeInstr(PlanState *node);
+
+
/* ------------------------------------------------------------------------
* ExecInitNode
*
@@ -149,6 +148,13 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
if (node == NULL)
return NULL;
+ /*
+ * Make sure there's enough stack available. Need to check here, in
+ * addition to ExecProcNode() (via ExecProcNodeFirst()), to ensure the
+ * stack isn't overrun while initializing the node tree.
+ */
+ check_stack_depth();
+
switch (nodeTag(node))
{
/*
@@ -365,6 +371,13 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
}
/*
+ * Add a wrapper around the ExecProcNode callback that checks stack depth
+ * during the first execution.
+ */
+ result->ExecProcNodeReal = result->ExecProcNode;
+ result->ExecProcNode = ExecProcNodeFirst;
+
+ /*
* Initialize any initPlans present in this node. The planner put them in
* a separate list for us.
*/
@@ -388,195 +401,51 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
}
-/* ----------------------------------------------------------------
- * ExecProcNode
- *
- * Execute the given node to return a(nother) tuple.
- * ----------------------------------------------------------------
+/*
+ * ExecProcNode wrapper that performs some one-time checks, before calling
+ * the relevant node method (possibly via an instrumentation wrapper).
*/
-TupleTableSlot *
-ExecProcNode(PlanState *node)
+static TupleTableSlot *
+ExecProcNodeFirst(PlanState *node)
{
- TupleTableSlot *result;
-
- if (node->chgParam != NULL) /* something changed */
- ExecReScan(node); /* let ReScan handle this */
+ /*
+ * Perform stack depth check during the first execution of the node. We
+ * only do so the first time round because it turns out to not be cheap on
+ * some common architectures (eg. x86). This relies on an assumption that
+ * ExecProcNode calls for a given plan node will always be made at roughly
+ * the same stack depth.
+ */
+ check_stack_depth();
+ /*
+ * If instrumentation is required, change the wrapper to one that just
+ * does instrumentation. Otherwise we can dispense with all wrappers and
+ * have ExecProcNode() directly call the relevant function from now on.
+ */
if (node->instrument)
- InstrStartNode(node->instrument);
-
- switch (nodeTag(node))
- {
- /*
- * control nodes
- */
- case T_ResultState:
- result = ExecResult((ResultState *) node);
- break;
-
- case T_ProjectSetState:
- result = ExecProjectSet((ProjectSetState *) node);
- break;
-
- case T_ModifyTableState:
- result = ExecModifyTable((ModifyTableState *) node);
- break;
-
- case T_AppendState:
- result = ExecAppend((AppendState *) node);
- break;
-
- case T_MergeAppendState:
- result = ExecMergeAppend((MergeAppendState *) node);
- break;
-
- case T_RecursiveUnionState:
- result = ExecRecursiveUnion((RecursiveUnionState *) node);
- break;
-
- /* BitmapAndState does not yield tuples */
-
- /* BitmapOrState does not yield tuples */
-
- /*
- * scan nodes
- */
- case T_SeqScanState:
- result = ExecSeqScan((SeqScanState *) node);
- break;
-
- case T_SampleScanState:
- result = ExecSampleScan((SampleScanState *) node);
- break;
-
- case T_IndexScanState:
- result = ExecIndexScan((IndexScanState *) node);
- break;
-
- case T_IndexOnlyScanState:
- result = ExecIndexOnlyScan((IndexOnlyScanState *) node);
- break;
-
- /* BitmapIndexScanState does not yield tuples */
-
- case T_BitmapHeapScanState:
- result = ExecBitmapHeapScan((BitmapHeapScanState *) node);
- break;
-
- case T_TidScanState:
- result = ExecTidScan((TidScanState *) node);
- break;
-
- case T_SubqueryScanState:
- result = ExecSubqueryScan((SubqueryScanState *) node);
- break;
-
- case T_FunctionScanState:
- result = ExecFunctionScan((FunctionScanState *) node);
- break;
-
- case T_TableFuncScanState:
- result = ExecTableFuncScan((TableFuncScanState *) node);
- break;
-
- case T_ValuesScanState:
- result = ExecValuesScan((ValuesScanState *) node);
- break;
-
- case T_CteScanState:
- result = ExecCteScan((CteScanState *) node);
- break;
-
- case T_NamedTuplestoreScanState:
- result = ExecNamedTuplestoreScan((NamedTuplestoreScanState *) node);
- break;
-
- case T_WorkTableScanState:
- result = ExecWorkTableScan((WorkTableScanState *) node);
- break;
-
- case T_ForeignScanState:
- result = ExecForeignScan((ForeignScanState *) node);
- break;
-
- case T_CustomScanState:
- result = ExecCustomScan((CustomScanState *) node);
- break;
-
- /*
- * join nodes
- */
- case T_NestLoopState:
- result = ExecNestLoop((NestLoopState *) node);
- break;
-
- case T_MergeJoinState:
- result = ExecMergeJoin((MergeJoinState *) node);
- break;
-
- case T_HashJoinState:
- result = ExecHashJoin((HashJoinState *) node);
- break;
-
- /*
- * materialization nodes
- */
- case T_MaterialState:
- result = ExecMaterial((MaterialState *) node);
- break;
-
- case T_SortState:
- result = ExecSort((SortState *) node);
- break;
-
- case T_GroupState:
- result = ExecGroup((GroupState *) node);
- break;
+ node->ExecProcNode = ExecProcNodeInstr;
+ else
+ node->ExecProcNode = node->ExecProcNodeReal;
- case T_AggState:
- result = ExecAgg((AggState *) node);
- break;
-
- case T_WindowAggState:
- result = ExecWindowAgg((WindowAggState *) node);
- break;
-
- case T_UniqueState:
- result = ExecUnique((UniqueState *) node);
- break;
-
- case T_GatherState:
- result = ExecGather((GatherState *) node);
- break;
-
- case T_GatherMergeState:
- result = ExecGatherMerge((GatherMergeState *) node);
- break;
-
- case T_HashState:
- result = ExecHash((HashState *) node);
- break;
+ return node->ExecProcNode(node);
+}
- case T_SetOpState:
- result = ExecSetOp((SetOpState *) node);
- break;
- case T_LockRowsState:
- result = ExecLockRows((LockRowsState *) node);
- break;
+/*
+ * ExecProcNode wrapper that performs instrumentation calls. By keeping
+ * this a separate function, we avoid overhead in the normal case where
+ * no instrumentation is wanted.
+ */
+static TupleTableSlot *
+ExecProcNodeInstr(PlanState *node)
+{
+ TupleTableSlot *result;
- case T_LimitState:
- result = ExecLimit((LimitState *) node);
- break;
+ InstrStartNode(node->instrument);
- default:
- elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node));
- result = NULL;
- break;
- }
+ result = node->ExecProcNodeReal(node);
- if (node->instrument)
- InstrStopNode(node->instrument, TupIsNull(result) ? 0.0 : 1.0);
+ InstrStopNode(node->instrument, TupIsNull(result) ? 0.0 : 1.0);
return result;
}
@@ -600,6 +469,8 @@ MultiExecProcNode(PlanState *node)
{
Node *result;
+ check_stack_depth();
+
CHECK_FOR_INTERRUPTS();
if (node->chgParam != NULL) /* something changed */
@@ -657,6 +528,13 @@ ExecEndNode(PlanState *node)
if (node == NULL)
return;
+ /*
+ * Make sure there's enough stack available. Need to check here, in
+ * addition to ExecProcNode() (via ExecProcNodeFirst()), because it's not
+ * guaranteed that ExecProcNode() is reached for all nodes.
+ */
+ check_stack_depth();
+
if (node->chgParam != NULL)
{
bms_free(node->chgParam);
@@ -855,6 +733,8 @@ ExecShutdownNode(PlanState *node)
if (node == NULL)
return false;
+ check_stack_depth();
+
planstate_tree_walker(node, ExecShutdownNode, NULL);
switch (nodeTag(node))
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 377916d..6a26773 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2099,9 +2099,10 @@ lookup_hash_entries(AggState *aggstate)
* stored in the expression context to be used when ExecProject evaluates
* the result tuple.
*/
-TupleTableSlot *
-ExecAgg(AggState *node)
+static TupleTableSlot *
+ExecAgg(PlanState *pstate)
{
+ AggState *node = castNode(AggState, pstate);
TupleTableSlot *result = NULL;
CHECK_FOR_INTERRUPTS();
@@ -2695,6 +2696,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
aggstate = makeNode(AggState);
aggstate->ss.ps.plan = (Plan *) node;
aggstate->ss.ps.state = estate;
+ aggstate->ss.ps.ExecProcNode = ExecAgg;
aggstate->aggs = NIL;
aggstate->numaggs = 0;
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index 58045e0..bed9bb8 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -61,6 +61,7 @@
#include "executor/nodeAppend.h"
#include "miscadmin.h"
+static TupleTableSlot *ExecAppend(PlanState *pstate);
static bool exec_append_initialize_next(AppendState *appendstate);
@@ -147,6 +148,7 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
*/
appendstate->ps.plan = (Plan *) node;
appendstate->ps.state = estate;
+ appendstate->ps.ExecProcNode = ExecAppend;
appendstate->appendplans = appendplanstates;
appendstate->as_nplans = nplans;
@@ -197,9 +199,11 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
* Handles iteration over multiple subplans.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecAppend(AppendState *node)
+static TupleTableSlot *
+ExecAppend(PlanState *pstate)
{
+ AppendState *node = castNode(AppendState, pstate);
+
for (;;)
{
PlanState *subnode;
diff --git a/src/backend/executor/nodeBitmapAnd.c b/src/backend/executor/nodeBitmapAnd.c
index e4eb028..1c5c312 100644
--- a/src/backend/executor/nodeBitmapAnd.c
+++ b/src/backend/executor/nodeBitmapAnd.c
@@ -33,6 +33,19 @@
/* ----------------------------------------------------------------
+ * ExecBitmapAnd
+ *
+ * stub for pro forma compliance
+ * ----------------------------------------------------------------
+ */
+static TupleTableSlot *
+ExecBitmapAnd(PlanState *pstate)
+{
+ elog(ERROR, "BitmapAnd node does not support ExecProcNode call convention");
+ return NULL;
+}
+
+/* ----------------------------------------------------------------
* ExecInitBitmapAnd
*
* Begin all of the subscans of the BitmapAnd node.
@@ -63,6 +76,7 @@ ExecInitBitmapAnd(BitmapAnd *node, EState *estate, int eflags)
*/
bitmapandstate->ps.plan = (Plan *) node;
bitmapandstate->ps.state = estate;
+ bitmapandstate->ps.ExecProcNode = ExecBitmapAnd;
bitmapandstate->bitmapplans = bitmapplanstates;
bitmapandstate->nplans = nplans;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index cf109d5..79f534e 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -665,9 +665,11 @@ BitmapHeapRecheck(BitmapHeapScanState *node, TupleTableSlot *slot)
* ExecBitmapHeapScan(node)
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecBitmapHeapScan(BitmapHeapScanState *node)
+static TupleTableSlot *
+ExecBitmapHeapScan(PlanState *pstate)
{
+ BitmapHeapScanState *node = castNode(BitmapHeapScanState, pstate);
+
return ExecScan(&node->ss,
(ExecScanAccessMtd) BitmapHeapNext,
(ExecScanRecheckMtd) BitmapHeapRecheck);
@@ -815,6 +817,7 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
scanstate = makeNode(BitmapHeapScanState);
scanstate->ss.ps.plan = (Plan *) node;
scanstate->ss.ps.state = estate;
+ scanstate->ss.ps.ExecProcNode = ExecBitmapHeapScan;
scanstate->tbm = NULL;
scanstate->tbmiterator = NULL;
diff --git a/src/backend/executor/nodeBitmapIndexscan.c b/src/backend/executor/nodeBitmapIndexscan.c
index 2411a2e..6feb70f 100644
--- a/src/backend/executor/nodeBitmapIndexscan.c
+++ b/src/backend/executor/nodeBitmapIndexscan.c
@@ -29,6 +29,19 @@
/* ----------------------------------------------------------------
+ * ExecBitmapIndexScan
+ *
+ * stub for pro forma compliance
+ * ----------------------------------------------------------------
+ */
+static TupleTableSlot *
+ExecBitmapIndexScan(PlanState *pstate)
+{
+ elog(ERROR, "BitmapIndexScan node does not support ExecProcNode call convention");
+ return NULL;
+}
+
+/* ----------------------------------------------------------------
* MultiExecBitmapIndexScan(node)
* ----------------------------------------------------------------
*/
@@ -208,6 +221,7 @@ ExecInitBitmapIndexScan(BitmapIndexScan *node, EState *estate, int eflags)
indexstate = makeNode(BitmapIndexScanState);
indexstate->ss.ps.plan = (Plan *) node;
indexstate->ss.ps.state = estate;
+ indexstate->ss.ps.ExecProcNode = ExecBitmapIndexScan;
/* normally we don't make the result bitmap till runtime */
indexstate->biss_result = NULL;
diff --git a/src/backend/executor/nodeBitmapOr.c b/src/backend/executor/nodeBitmapOr.c
index 4f0ddc6..66a7a89 100644
--- a/src/backend/executor/nodeBitmapOr.c
+++ b/src/backend/executor/nodeBitmapOr.c
@@ -34,6 +34,19 @@
/* ----------------------------------------------------------------
+ * ExecBitmapOr
+ *
+ * stub for pro forma compliance
+ * ----------------------------------------------------------------
+ */
+static TupleTableSlot *
+ExecBitmapOr(PlanState *pstate)
+{
+ elog(ERROR, "BitmapOr node does not support ExecProcNode call convention");
+ return NULL;
+}
+
+/* ----------------------------------------------------------------
* ExecInitBitmapOr
*
* Begin all of the subscans of the BitmapOr node.
@@ -64,6 +77,7 @@ ExecInitBitmapOr(BitmapOr *node, EState *estate, int eflags)
*/
bitmaporstate->ps.plan = (Plan *) node;
bitmaporstate->ps.state = estate;
+ bitmaporstate->ps.ExecProcNode = ExecBitmapOr;
bitmaporstate->bitmapplans = bitmapplanstates;
bitmaporstate->nplans = nplans;
diff --git a/src/backend/executor/nodeCtescan.c b/src/backend/executor/nodeCtescan.c
index bed7949..79676ca 100644
--- a/src/backend/executor/nodeCtescan.c
+++ b/src/backend/executor/nodeCtescan.c
@@ -149,9 +149,11 @@ CteScanRecheck(CteScanState *node, TupleTableSlot *slot)
* access method functions.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecCteScan(CteScanState *node)
+static TupleTableSlot *
+ExecCteScan(PlanState *pstate)
{
+ CteScanState *node = castNode(CteScanState, pstate);
+
return ExecScan(&node->ss,
(ExecScanAccessMtd) CteScanNext,
(ExecScanRecheckMtd) CteScanRecheck);
@@ -191,6 +193,7 @@ ExecInitCteScan(CteScan *node, EState *estate, int eflags)
scanstate = makeNode(CteScanState);
scanstate->ss.ps.plan = (Plan *) node;
scanstate->ss.ps.state = estate;
+ scanstate->ss.ps.ExecProcNode = ExecCteScan;
scanstate->eflags = eflags;
scanstate->cte_table = NULL;
scanstate->eof_cte = false;
diff --git a/src/backend/executor/nodeCustom.c b/src/backend/executor/nodeCustom.c
index fc15974..fb7645b 100644
--- a/src/backend/executor/nodeCustom.c
+++ b/src/backend/executor/nodeCustom.c
@@ -21,6 +21,10 @@
#include "utils/memutils.h"
#include "utils/rel.h"
+
+static TupleTableSlot *ExecCustomScan(PlanState *pstate);
+
+
CustomScanState *
ExecInitCustomScan(CustomScan *cscan, EState *estate, int eflags)
{
@@ -45,6 +49,7 @@ ExecInitCustomScan(CustomScan *cscan, EState *estate, int eflags)
/* fill up fields of ScanState */
css->ss.ps.plan = &cscan->scan.plan;
css->ss.ps.state = estate;
+ css->ss.ps.ExecProcNode = ExecCustomScan;
/* create expression context for node */
ExecAssignExprContext(estate, &css->ss.ps);
@@ -102,9 +107,11 @@ ExecInitCustomScan(CustomScan *cscan, EState *estate, int eflags)
return css;
}
-TupleTableSlot *
-ExecCustomScan(CustomScanState *node)
+static TupleTableSlot *
+ExecCustomScan(PlanState *pstate)
{
+ CustomScanState *node = castNode(CustomScanState, pstate);
+
CHECK_FOR_INTERRUPTS();
Assert(node->methods->ExecCustomScan != NULL);
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 9cde112..140e82e 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -113,10 +113,12 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
* access method functions.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecForeignScan(ForeignScanState *node)
+static TupleTableSlot *
+ExecForeignScan(PlanState *pstate)
{
- return ExecScan((ScanState *) node,
+ ForeignScanState *node = castNode(ForeignScanState, pstate);
+
+ return ExecScan(&node->ss,
(ExecScanAccessMtd) ForeignNext,
(ExecScanRecheckMtd) ForeignRecheck);
}
@@ -144,6 +146,7 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
scanstate = makeNode(ForeignScanState);
scanstate->ss.ps.plan = (Plan *) node;
scanstate->ss.ps.state = estate;
+ scanstate->ss.ps.ExecProcNode = ExecForeignScan;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeFunctionscan.c b/src/backend/executor/nodeFunctionscan.c
index 3217d64..9f87a7e 100644
--- a/src/backend/executor/nodeFunctionscan.c
+++ b/src/backend/executor/nodeFunctionscan.c
@@ -262,9 +262,11 @@ FunctionRecheck(FunctionScanState *node, TupleTableSlot *slot)
* access method functions.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecFunctionScan(FunctionScanState *node)
+static TupleTableSlot *
+ExecFunctionScan(PlanState *pstate)
{
+ FunctionScanState *node = castNode(FunctionScanState, pstate);
+
return ExecScan(&node->ss,
(ExecScanAccessMtd) FunctionNext,
(ExecScanRecheckMtd) FunctionRecheck);
@@ -299,6 +301,7 @@ ExecInitFunctionScan(FunctionScan *node, EState *estate, int eflags)
scanstate = makeNode(FunctionScanState);
scanstate->ss.ps.plan = (Plan *) node;
scanstate->ss.ps.state = estate;
+ scanstate->ss.ps.ExecProcNode = ExecFunctionScan;
scanstate->eflags = eflags;
/*
diff --git a/src/backend/executor/nodeGather.c b/src/backend/executor/nodeGather.c
index 5dbe19c..e8d94ee 100644
--- a/src/backend/executor/nodeGather.c
+++ b/src/backend/executor/nodeGather.c
@@ -43,6 +43,7 @@
#include "utils/rel.h"
+static TupleTableSlot *ExecGather(PlanState *pstate);
static TupleTableSlot *gather_getnext(GatherState *gatherstate);
static HeapTuple gather_readnext(GatherState *gatherstate);
static void ExecShutdownGatherWorkers(GatherState *node);
@@ -69,6 +70,7 @@ ExecInitGather(Gather *node, EState *estate, int eflags)
gatherstate = makeNode(GatherState);
gatherstate->ps.plan = (Plan *) node;
gatherstate->ps.state = estate;
+ gatherstate->ps.ExecProcNode = ExecGather;
gatherstate->need_to_scan_locally = !node->single_copy;
/*
@@ -120,9 +122,10 @@ ExecInitGather(Gather *node, EState *estate, int eflags)
* the next qualifying tuple.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecGather(GatherState *node)
+static TupleTableSlot *
+ExecGather(PlanState *pstate)
{
+ GatherState *node = castNode(GatherState, pstate);
TupleTableSlot *fslot = node->funnel_slot;
int i;
TupleTableSlot *slot;
diff --git a/src/backend/executor/nodeGatherMerge.c b/src/backend/executor/nodeGatherMerge.c
index 0aff379..9a81e22 100644
--- a/src/backend/executor/nodeGatherMerge.c
+++ b/src/backend/executor/nodeGatherMerge.c
@@ -44,6 +44,7 @@ typedef struct GMReaderTupleBuffer
*/
#define MAX_TUPLE_STORE 10
+static TupleTableSlot *ExecGatherMerge(PlanState *pstate);
static int32 heap_compare_slots(Datum a, Datum b, void *arg);
static TupleTableSlot *gather_merge_getnext(GatherMergeState *gm_state);
static HeapTuple gm_readnext_tuple(GatherMergeState *gm_state, int nreader,
@@ -75,6 +76,7 @@ ExecInitGatherMerge(GatherMerge *node, EState *estate, int eflags)
gm_state = makeNode(GatherMergeState);
gm_state->ps.plan = (Plan *) node;
gm_state->ps.state = estate;
+ gm_state->ps.ExecProcNode = ExecGatherMerge;
/*
* Miscellaneous initialization
@@ -157,9 +159,10 @@ ExecInitGatherMerge(GatherMerge *node, EState *estate, int eflags)
* the next qualifying tuple.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecGatherMerge(GatherMergeState *node)
+static TupleTableSlot *
+ExecGatherMerge(PlanState *pstate)
{
+ GatherMergeState *node = castNode(GatherMergeState, pstate);
TupleTableSlot *slot;
ExprContext *econtext;
int i;
diff --git a/src/backend/executor/nodeGroup.c b/src/backend/executor/nodeGroup.c
index fc5e0e5..ab4ae24 100644
--- a/src/backend/executor/nodeGroup.c
+++ b/src/backend/executor/nodeGroup.c
@@ -32,9 +32,10 @@
*
* Return one tuple for each group of matching input tuples.
*/
-TupleTableSlot *
-ExecGroup(GroupState *node)
+static TupleTableSlot *
+ExecGroup(PlanState *pstate)
{
+ GroupState *node = castNode(GroupState, pstate);
ExprContext *econtext;
int numCols;
AttrNumber *grpColIdx;
@@ -175,6 +176,7 @@ ExecInitGroup(Group *node, EState *estate, int eflags)
grpstate = makeNode(GroupState);
grpstate->ss.ps.plan = (Plan *) node;
grpstate->ss.ps.state = estate;
+ grpstate->ss.ps.ExecProcNode = ExecGroup;
grpstate->grp_done = FALSE;
/*
diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index fbeb562..d10d94c 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -56,8 +56,8 @@ static void *dense_alloc(HashJoinTable hashtable, Size size);
* stub for pro forma compliance
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecHash(HashState *node)
+static TupleTableSlot *
+ExecHash(PlanState *pstate)
{
elog(ERROR, "Hash node does not support ExecProcNode call convention");
return NULL;
@@ -172,6 +172,7 @@ ExecInitHash(Hash *node, EState *estate, int eflags)
hashstate = makeNode(HashState);
hashstate->ps.plan = (Plan *) node;
hashstate->ps.state = estate;
+ hashstate->ps.ExecProcNode = ExecHash;
hashstate->hashtable = NULL;
hashstate->hashkeys = NIL; /* will be set by parent HashJoin */
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 252960c..ab1632c 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -58,9 +58,10 @@ static bool ExecHashJoinNewBatch(HashJoinState *hjstate);
* the other one is "outer".
* ----------------------------------------------------------------
*/
-TupleTableSlot * /* return: a tuple or NULL */
-ExecHashJoin(HashJoinState *node)
+static TupleTableSlot * /* return: a tuple or NULL */
+ExecHashJoin(PlanState *pstate)
{
+ HashJoinState *node = castNode(HashJoinState, pstate);
PlanState *outerNode;
HashState *hashNode;
ExprState *joinqual;
@@ -399,6 +400,7 @@ ExecInitHashJoin(HashJoin *node, EState *estate, int eflags)
hjstate = makeNode(HashJoinState);
hjstate->js.ps.plan = (Plan *) node;
hjstate->js.ps.state = estate;
+ hjstate->js.ps.ExecProcNode = ExecHashJoin;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index e2000764..fe7ba3f 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -306,9 +306,11 @@ IndexOnlyRecheck(IndexOnlyScanState *node, TupleTableSlot *slot)
* ExecIndexOnlyScan(node)
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecIndexOnlyScan(IndexOnlyScanState *node)
+static TupleTableSlot *
+ExecIndexOnlyScan(PlanState *pstate)
{
+ IndexOnlyScanState *node = castNode(IndexOnlyScanState, pstate);
+
/*
* If we have runtime keys and they've not already been set up, do it now.
*/
@@ -476,6 +478,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
indexstate = makeNode(IndexOnlyScanState);
indexstate->ss.ps.plan = (Plan *) node;
indexstate->ss.ps.state = estate;
+ indexstate->ss.ps.ExecProcNode = ExecIndexOnlyScan;
indexstate->ioss_HeapFetches = 0;
/*
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index 6704ede..404076d 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -542,9 +542,11 @@ reorderqueue_pop(IndexScanState *node)
* ExecIndexScan(node)
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecIndexScan(IndexScanState *node)
+static TupleTableSlot *
+ExecIndexScan(PlanState *pstate)
{
+ IndexScanState *node = castNode(IndexScanState, pstate);
+
/*
* If we have runtime keys and they've not already been set up, do it now.
*/
@@ -910,6 +912,7 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
indexstate = makeNode(IndexScanState);
indexstate->ss.ps.plan = (Plan *) node;
indexstate->ss.ps.state = estate;
+ indexstate->ss.ps.ExecProcNode = ExecIndexScan;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index 2ed3523..ac5a2ff 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -37,9 +37,10 @@ static void pass_down_bound(LimitState *node, PlanState *child_node);
* filtering on the stream of tuples returned by a subplan.
* ----------------------------------------------------------------
*/
-TupleTableSlot * /* return: a tuple or NULL */
-ExecLimit(LimitState *node)
+static TupleTableSlot * /* return: a tuple or NULL */
+ExecLimit(PlanState *pstate)
{
+ LimitState *node = castNode(LimitState, pstate);
ScanDirection direction;
TupleTableSlot *slot;
PlanState *outerPlan;
@@ -378,6 +379,7 @@ ExecInitLimit(Limit *node, EState *estate, int eflags)
limitstate = makeNode(LimitState);
limitstate->ps.plan = (Plan *) node;
limitstate->ps.state = estate;
+ limitstate->ps.ExecProcNode = ExecLimit;
limitstate->lstate = LIMIT_INITIAL;
diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c
index dd4e2c5..9389560 100644
--- a/src/backend/executor/nodeLockRows.c
+++ b/src/backend/executor/nodeLockRows.c
@@ -36,9 +36,10 @@
* ExecLockRows
* ----------------------------------------------------------------
*/
-TupleTableSlot * /* return: a tuple or NULL */
-ExecLockRows(LockRowsState *node)
+static TupleTableSlot * /* return: a tuple or NULL */
+ExecLockRows(PlanState *pstate)
{
+ LockRowsState *node = castNode(LockRowsState, pstate);
TupleTableSlot *slot;
EState *estate;
PlanState *outerPlan;
@@ -364,6 +365,7 @@ ExecInitLockRows(LockRows *node, EState *estate, int eflags)
lrstate = makeNode(LockRowsState);
lrstate->ps.plan = (Plan *) node;
lrstate->ps.state = estate;
+ lrstate->ps.ExecProcNode = ExecLockRows;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeMaterial.c b/src/backend/executor/nodeMaterial.c
index 3342949..91178f1 100644
--- a/src/backend/executor/nodeMaterial.c
+++ b/src/backend/executor/nodeMaterial.c
@@ -35,9 +35,10 @@
*
* ----------------------------------------------------------------
*/
-TupleTableSlot * /* result tuple from subplan */
-ExecMaterial(MaterialState *node)
+static TupleTableSlot * /* result tuple from subplan */
+ExecMaterial(PlanState *pstate)
{
+ MaterialState *node = castNode(MaterialState, pstate);
EState *estate;
ScanDirection dir;
bool forward;
@@ -173,6 +174,7 @@ ExecInitMaterial(Material *node, EState *estate, int eflags)
matstate = makeNode(MaterialState);
matstate->ss.ps.plan = (Plan *) node;
matstate->ss.ps.state = estate;
+ matstate->ss.ps.ExecProcNode = ExecMaterial;
/*
* We must have a tuplestore buffering the subplan output to do backward
diff --git a/src/backend/executor/nodeMergeAppend.c b/src/backend/executor/nodeMergeAppend.c
index d41def1..6bf490b 100644
--- a/src/backend/executor/nodeMergeAppend.c
+++ b/src/backend/executor/nodeMergeAppend.c
@@ -50,6 +50,7 @@
*/
typedef int32 SlotNumber;
+static TupleTableSlot *ExecMergeAppend(PlanState *pstate);
static int heap_compare_slots(Datum a, Datum b, void *arg);
@@ -89,6 +90,7 @@ ExecInitMergeAppend(MergeAppend *node, EState *estate, int eflags)
*/
mergestate->ps.plan = (Plan *) node;
mergestate->ps.state = estate;
+ mergestate->ps.ExecProcNode = ExecMergeAppend;
mergestate->mergeplans = mergeplanstates;
mergestate->ms_nplans = nplans;
@@ -169,9 +171,10 @@ ExecInitMergeAppend(MergeAppend *node, EState *estate, int eflags)
* Handles iteration over multiple subplans.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecMergeAppend(MergeAppendState *node)
+static TupleTableSlot *
+ExecMergeAppend(PlanState *pstate)
{
+ MergeAppendState *node = castNode(MergeAppendState, pstate);
TupleTableSlot *result;
SlotNumber i;
diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
index 324b61b..925b4cf 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -596,9 +596,10 @@ ExecMergeTupleDump(MergeJoinState *mergestate)
* ExecMergeJoin
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecMergeJoin(MergeJoinState *node)
+static TupleTableSlot *
+ExecMergeJoin(PlanState *pstate)
{
+ MergeJoinState *node = castNode(MergeJoinState, pstate);
ExprState *joinqual;
ExprState *otherqual;
bool qualResult;
@@ -1448,6 +1449,7 @@ ExecInitMergeJoin(MergeJoin *node, EState *estate, int eflags)
mergestate = makeNode(MergeJoinState);
mergestate->js.ps.plan = (Plan *) node;
mergestate->js.ps.state = estate;
+ mergestate->js.ps.ExecProcNode = ExecMergeJoin;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 637a582..0dde0ed 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1535,9 +1535,10 @@ ExecSetupTransitionCaptureState(ModifyTableState *mtstate, EState *estate)
* if needed.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecModifyTable(ModifyTableState *node)
+static TupleTableSlot *
+ExecModifyTable(PlanState *pstate)
{
+ ModifyTableState *node = castNode(ModifyTableState, pstate);
EState *estate = node->ps.state;
CmdType operation = node->operation;
ResultRelInfo *saved_resultRelInfo;
@@ -1806,6 +1807,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
mtstate = makeNode(ModifyTableState);
mtstate->ps.plan = (Plan *) node;
mtstate->ps.state = estate;
+ mtstate->ps.ExecProcNode = ExecModifyTable;
mtstate->operation = operation;
mtstate->canSetTag = node->canSetTag;
diff --git a/src/backend/executor/nodeNamedtuplestorescan.c b/src/backend/executor/nodeNamedtuplestorescan.c
index 6223486..3a65b9f5 100644
--- a/src/backend/executor/nodeNamedtuplestorescan.c
+++ b/src/backend/executor/nodeNamedtuplestorescan.c
@@ -63,9 +63,11 @@ NamedTuplestoreScanRecheck(NamedTuplestoreScanState *node, TupleTableSlot *slot)
* access method functions.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecNamedTuplestoreScan(NamedTuplestoreScanState *node)
+static TupleTableSlot *
+ExecNamedTuplestoreScan(PlanState *pstate)
{
+ NamedTuplestoreScanState *node = castNode(NamedTuplestoreScanState, pstate);
+
return ExecScan(&node->ss,
(ExecScanAccessMtd) NamedTuplestoreScanNext,
(ExecScanRecheckMtd) NamedTuplestoreScanRecheck);
@@ -97,6 +99,7 @@ ExecInitNamedTuplestoreScan(NamedTuplestoreScan *node, EState *estate, int eflag
scanstate = makeNode(NamedTuplestoreScanState);
scanstate->ss.ps.plan = (Plan *) node;
scanstate->ss.ps.state = estate;
+ scanstate->ss.ps.ExecProcNode = ExecNamedTuplestoreScan;
enr = get_ENR(estate->es_queryEnv, node->enrname);
if (!enr)
diff --git a/src/backend/executor/nodeNestloop.c b/src/backend/executor/nodeNestloop.c
index bedc374..4447b7c 100644
--- a/src/backend/executor/nodeNestloop.c
+++ b/src/backend/executor/nodeNestloop.c
@@ -57,9 +57,10 @@
* are prepared to return the first tuple.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecNestLoop(NestLoopState *node)
+static TupleTableSlot *
+ExecNestLoop(PlanState *pstate)
{
+ NestLoopState *node = castNode(NestLoopState, pstate);
NestLoop *nl;
PlanState *innerPlan;
PlanState *outerPlan;
@@ -275,6 +276,7 @@ ExecInitNestLoop(NestLoop *node, EState *estate, int eflags)
nlstate = makeNode(NestLoopState);
nlstate->js.ps.plan = (Plan *) node;
nlstate->js.ps.state = estate;
+ nlstate->js.ps.ExecProcNode = ExecNestLoop;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeProjectSet.c b/src/backend/executor/nodeProjectSet.c
index 3b69c7a..d93462c 100644
--- a/src/backend/executor/nodeProjectSet.c
+++ b/src/backend/executor/nodeProjectSet.c
@@ -39,9 +39,10 @@ static TupleTableSlot *ExecProjectSRF(ProjectSetState *node, bool continuing);
* returning functions).
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecProjectSet(ProjectSetState *node)
+static TupleTableSlot *
+ExecProjectSet(PlanState *pstate)
{
+ ProjectSetState *node = castNode(ProjectSetState, pstate);
TupleTableSlot *outerTupleSlot;
TupleTableSlot *resultSlot;
PlanState *outerPlan;
@@ -215,6 +216,7 @@ ExecInitProjectSet(ProjectSet *node, EState *estate, int eflags)
state = makeNode(ProjectSetState);
state->ps.plan = (Plan *) node;
state->ps.state = estate;
+ state->ps.ExecProcNode = ExecProjectSet;
state->pending_srf_tuples = false;
diff --git a/src/backend/executor/nodeRecursiveunion.c b/src/backend/executor/nodeRecursiveunion.c
index 2802fff..a64dd13 100644
--- a/src/backend/executor/nodeRecursiveunion.c
+++ b/src/backend/executor/nodeRecursiveunion.c
@@ -66,9 +66,10 @@ build_hash_table(RecursiveUnionState *rustate)
* 2.6 go back to 2.2
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecRecursiveUnion(RecursiveUnionState *node)
+static TupleTableSlot *
+ExecRecursiveUnion(PlanState *pstate)
{
+ RecursiveUnionState *node = castNode(RecursiveUnionState, pstate);
PlanState *outerPlan = outerPlanState(node);
PlanState *innerPlan = innerPlanState(node);
RecursiveUnion *plan = (RecursiveUnion *) node->ps.plan;
@@ -172,6 +173,7 @@ ExecInitRecursiveUnion(RecursiveUnion *node, EState *estate, int eflags)
rustate = makeNode(RecursiveUnionState);
rustate->ps.plan = (Plan *) node;
rustate->ps.state = estate;
+ rustate->ps.ExecProcNode = ExecRecursiveUnion;
rustate->eqfunctions = NULL;
rustate->hashfunctions = NULL;
diff --git a/src/backend/executor/nodeResult.c b/src/backend/executor/nodeResult.c
index f007f46..4c879d8 100644
--- a/src/backend/executor/nodeResult.c
+++ b/src/backend/executor/nodeResult.c
@@ -64,9 +64,10 @@
* 'nil' if the constant qualification is not satisfied.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecResult(ResultState *node)
+static TupleTableSlot *
+ExecResult(PlanState *pstate)
{
+ ResultState *node = castNode(ResultState, pstate);
TupleTableSlot *outerTupleSlot;
PlanState *outerPlan;
ExprContext *econtext;
@@ -191,6 +192,7 @@ ExecInitResult(Result *node, EState *estate, int eflags)
resstate = makeNode(ResultState);
resstate->ps.plan = (Plan *) node;
resstate->ps.state = estate;
+ resstate->ps.ExecProcNode = ExecResult;
resstate->rs_done = false;
resstate->rs_checkqual = (node->resconstantqual == NULL) ? false : true;
diff --git a/src/backend/executor/nodeSamplescan.c b/src/backend/executor/nodeSamplescan.c
index b710ef7..9c74a83 100644
--- a/src/backend/executor/nodeSamplescan.c
+++ b/src/backend/executor/nodeSamplescan.c
@@ -96,10 +96,12 @@ SampleRecheck(SampleScanState *node, TupleTableSlot *slot)
* access method functions.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecSampleScan(SampleScanState *node)
+static TupleTableSlot *
+ExecSampleScan(PlanState *pstate)
{
- return ExecScan((ScanState *) node,
+ SampleScanState *node = castNode(SampleScanState, pstate);
+
+ return ExecScan(&node->ss,
(ExecScanAccessMtd) SampleNext,
(ExecScanRecheckMtd) SampleRecheck);
}
@@ -153,6 +155,7 @@ ExecInitSampleScan(SampleScan *node, EState *estate, int eflags)
scanstate = makeNode(SampleScanState);
scanstate->ss.ps.plan = (Plan *) node;
scanstate->ss.ps.state = estate;
+ scanstate->ss.ps.ExecProcNode = ExecSampleScan;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeSeqscan.c b/src/backend/executor/nodeSeqscan.c
index 307df87..5c49d4c 100644
--- a/src/backend/executor/nodeSeqscan.c
+++ b/src/backend/executor/nodeSeqscan.c
@@ -121,10 +121,12 @@ SeqRecheck(SeqScanState *node, TupleTableSlot *slot)
* access method functions.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecSeqScan(SeqScanState *node)
+static TupleTableSlot *
+ExecSeqScan(PlanState *pstate)
{
- return ExecScan((ScanState *) node,
+ SeqScanState *node = castNode(SeqScanState, pstate);
+
+ return ExecScan(&node->ss,
(ExecScanAccessMtd) SeqNext,
(ExecScanRecheckMtd) SeqRecheck);
}
@@ -177,6 +179,7 @@ ExecInitSeqScan(SeqScan *node, EState *estate, int eflags)
scanstate = makeNode(SeqScanState);
scanstate->ss.ps.plan = (Plan *) node;
scanstate->ss.ps.state = estate;
+ scanstate->ss.ps.ExecProcNode = ExecSeqScan;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeSetOp.c b/src/backend/executor/nodeSetOp.c
index 56c5643..571cbf8 100644
--- a/src/backend/executor/nodeSetOp.c
+++ b/src/backend/executor/nodeSetOp.c
@@ -180,9 +180,10 @@ set_output_count(SetOpState *setopstate, SetOpStatePerGroup pergroup)
* ExecSetOp
* ----------------------------------------------------------------
*/
-TupleTableSlot * /* return: a tuple or NULL */
-ExecSetOp(SetOpState *node)
+static TupleTableSlot * /* return: a tuple or NULL */
+ExecSetOp(PlanState *pstate)
{
+ SetOpState *node = castNode(SetOpState, pstate);
SetOp *plannode = (SetOp *) node->ps.plan;
TupleTableSlot *resultTupleSlot = node->ps.ps_ResultTupleSlot;
@@ -485,6 +486,7 @@ ExecInitSetOp(SetOp *node, EState *estate, int eflags)
setopstate = makeNode(SetOpState);
setopstate->ps.plan = (Plan *) node;
setopstate->ps.state = estate;
+ setopstate->ps.ExecProcNode = ExecSetOp;
setopstate->eqfunctions = NULL;
setopstate->hashfunctions = NULL;
diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
index 799a4e9..aae4150 100644
--- a/src/backend/executor/nodeSort.c
+++ b/src/backend/executor/nodeSort.c
@@ -35,9 +35,10 @@
* -- the outer child is prepared to return the first tuple.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecSort(SortState *node)
+static TupleTableSlot *
+ExecSort(PlanState *pstate)
{
+ SortState *node = castNode(SortState, pstate);
EState *estate;
ScanDirection dir;
Tuplesortstate *tuplesortstate;
@@ -165,6 +166,7 @@ ExecInitSort(Sort *node, EState *estate, int eflags)
sortstate = makeNode(SortState);
sortstate->ss.ps.plan = (Plan *) node;
sortstate->ss.ps.state = estate;
+ sortstate->ss.ps.ExecProcNode = ExecSort;
/*
* We must have random access to the sort output to do backward scan or
diff --git a/src/backend/executor/nodeSubqueryscan.c b/src/backend/executor/nodeSubqueryscan.c
index ae18470..088c929 100644
--- a/src/backend/executor/nodeSubqueryscan.c
+++ b/src/backend/executor/nodeSubqueryscan.c
@@ -79,9 +79,11 @@ SubqueryRecheck(SubqueryScanState *node, TupleTableSlot *slot)
* access method functions.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecSubqueryScan(SubqueryScanState *node)
+static TupleTableSlot *
+ExecSubqueryScan(PlanState *pstate)
{
+ SubqueryScanState *node = castNode(SubqueryScanState, pstate);
+
return ExecScan(&node->ss,
(ExecScanAccessMtd) SubqueryNext,
(ExecScanRecheckMtd) SubqueryRecheck);
@@ -109,6 +111,7 @@ ExecInitSubqueryScan(SubqueryScan *node, EState *estate, int eflags)
subquerystate = makeNode(SubqueryScanState);
subquerystate->ss.ps.plan = (Plan *) node;
subquerystate->ss.ps.state = estate;
+ subquerystate->ss.ps.ExecProcNode = ExecSubqueryScan;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeTableFuncscan.c b/src/backend/executor/nodeTableFuncscan.c
index 2859363..b03d2ef 100644
--- a/src/backend/executor/nodeTableFuncscan.c
+++ b/src/backend/executor/nodeTableFuncscan.c
@@ -93,9 +93,11 @@ TableFuncRecheck(TableFuncScanState *node, TupleTableSlot *slot)
* access method functions.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecTableFuncScan(TableFuncScanState *node)
+static TupleTableSlot *
+ExecTableFuncScan(PlanState *pstate)
{
+ TableFuncScanState *node = castNode(TableFuncScanState, pstate);
+
return ExecScan(&node->ss,
(ExecScanAccessMtd) TableFuncNext,
(ExecScanRecheckMtd) TableFuncRecheck);
@@ -128,6 +130,7 @@ ExecInitTableFuncScan(TableFuncScan *node, EState *estate, int eflags)
scanstate = makeNode(TableFuncScanState);
scanstate->ss.ps.plan = (Plan *) node;
scanstate->ss.ps.state = estate;
+ scanstate->ss.ps.ExecProcNode = ExecTableFuncScan;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeTidscan.c b/src/backend/executor/nodeTidscan.c
index c122473..0ee76e7 100644
--- a/src/backend/executor/nodeTidscan.c
+++ b/src/backend/executor/nodeTidscan.c
@@ -445,9 +445,11 @@ TidRecheck(TidScanState *node, TupleTableSlot *slot)
* -- tidPtr is -1.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecTidScan(TidScanState *node)
+static TupleTableSlot *
+ExecTidScan(PlanState *pstate)
{
+ TidScanState *node = castNode(TidScanState, pstate);
+
return ExecScan(&node->ss,
(ExecScanAccessMtd) TidNext,
(ExecScanRecheckMtd) TidRecheck);
@@ -519,6 +521,7 @@ ExecInitTidScan(TidScan *node, EState *estate, int eflags)
tidstate = makeNode(TidScanState);
tidstate->ss.ps.plan = (Plan *) node;
tidstate->ss.ps.state = estate;
+ tidstate->ss.ps.ExecProcNode = ExecTidScan;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeUnique.c b/src/backend/executor/nodeUnique.c
index db78c88..621fdd9 100644
--- a/src/backend/executor/nodeUnique.c
+++ b/src/backend/executor/nodeUnique.c
@@ -43,9 +43,10 @@
* ExecUnique
* ----------------------------------------------------------------
*/
-TupleTableSlot * /* return: a tuple or NULL */
-ExecUnique(UniqueState *node)
+static TupleTableSlot * /* return: a tuple or NULL */
+ExecUnique(PlanState *pstate)
{
+ UniqueState *node = castNode(UniqueState, pstate);
Unique *plannode = (Unique *) node->ps.plan;
TupleTableSlot *resultTupleSlot;
TupleTableSlot *slot;
@@ -125,6 +126,7 @@ ExecInitUnique(Unique *node, EState *estate, int eflags)
uniquestate = makeNode(UniqueState);
uniquestate->ps.plan = (Plan *) node;
uniquestate->ps.state = estate;
+ uniquestate->ps.ExecProcNode = ExecUnique;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeValuesscan.c b/src/backend/executor/nodeValuesscan.c
index 9ee776c..6eacaed 100644
--- a/src/backend/executor/nodeValuesscan.c
+++ b/src/backend/executor/nodeValuesscan.c
@@ -185,9 +185,11 @@ ValuesRecheck(ValuesScanState *node, TupleTableSlot *slot)
* access method functions.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecValuesScan(ValuesScanState *node)
+static TupleTableSlot *
+ExecValuesScan(PlanState *pstate)
{
+ ValuesScanState *node = castNode(ValuesScanState, pstate);
+
return ExecScan(&node->ss,
(ExecScanAccessMtd) ValuesNext,
(ExecScanRecheckMtd) ValuesRecheck);
@@ -218,6 +220,7 @@ ExecInitValuesScan(ValuesScan *node, EState *estate, int eflags)
scanstate = makeNode(ValuesScanState);
scanstate->ss.ps.plan = (Plan *) node;
scanstate->ss.ps.state = estate;
+ scanstate->ss.ps.ExecProcNode = ExecValuesScan;
/*
* Miscellaneous initialization
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 9da35ac..80be460 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -1587,9 +1587,10 @@ update_frametailpos(WindowObject winobj, TupleTableSlot *slot)
* returned rows is exactly the same as its outer subplan's result.
* -----------------
*/
-TupleTableSlot *
-ExecWindowAgg(WindowAggState *winstate)
+static TupleTableSlot *
+ExecWindowAgg(PlanState *pstate)
{
+ WindowAggState *winstate = castNode(WindowAggState, pstate);
ExprContext *econtext;
int i;
int numfuncs;
@@ -1790,6 +1791,7 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
winstate = makeNode(WindowAggState);
winstate->ss.ps.plan = (Plan *) node;
winstate->ss.ps.state = estate;
+ winstate->ss.ps.ExecProcNode = ExecWindowAgg;
/*
* Create expression contexts. We need two, one for per-input-tuple
diff --git a/src/backend/executor/nodeWorktablescan.c b/src/backend/executor/nodeWorktablescan.c
index d7616be..d5ffadd 100644
--- a/src/backend/executor/nodeWorktablescan.c
+++ b/src/backend/executor/nodeWorktablescan.c
@@ -77,9 +77,11 @@ WorkTableScanRecheck(WorkTableScanState *node, TupleTableSlot *slot)
* access method functions.
* ----------------------------------------------------------------
*/
-TupleTableSlot *
-ExecWorkTableScan(WorkTableScanState *node)
+static TupleTableSlot *
+ExecWorkTableScan(PlanState *pstate)
{
+ WorkTableScanState *node = castNode(WorkTableScanState, pstate);
+
/*
* On the first call, find the ancestor RecursiveUnion's state via the
* Param slot reserved for it. (We can't do this during node init because
@@ -144,6 +146,7 @@ ExecInitWorkTableScan(WorkTableScan *node, EState *estate, int eflags)
scanstate = makeNode(WorkTableScanState);
scanstate->ss.ps.plan = (Plan *) node;
scanstate->ss.ps.state = estate;
+ scanstate->ss.ps.ExecProcNode = ExecWorkTableScan;
scanstate->rustate = NULL; /* we'll set this later */
/*
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 59c28b7..60326f9 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -225,14 +225,31 @@ extern void EvalPlanQualBegin(EPQState *epqstate, EState *parentestate);
extern void EvalPlanQualEnd(EPQState *epqstate);
/*
- * prototypes from functions in execProcnode.c
+ * functions in execProcnode.c
*/
extern PlanState *ExecInitNode(Plan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecProcNode(PlanState *node);
extern Node *MultiExecProcNode(PlanState *node);
extern void ExecEndNode(PlanState *node);
extern bool ExecShutdownNode(PlanState *node);
+
+/* ----------------------------------------------------------------
+ * ExecProcNode
+ *
+ * Execute the given node to return a(nother) tuple.
+ * ----------------------------------------------------------------
+ */
+#ifndef FRONTEND
+static inline TupleTableSlot *
+ExecProcNode(PlanState *node)
+{
+ if (node->chgParam != NULL) /* something changed? */
+ ExecReScan(node); /* let ReScan handle this */
+
+ return node->ExecProcNode(node);
+}
+#endif
+
/*
* prototypes from functions in execExpr.c
*/
diff --git a/src/include/executor/nodeAgg.h b/src/include/executor/nodeAgg.h
index fa11ba9..eff5af9 100644
--- a/src/include/executor/nodeAgg.h
+++ b/src/include/executor/nodeAgg.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern AggState *ExecInitAgg(Agg *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecAgg(AggState *node);
extern void ExecEndAgg(AggState *node);
extern void ExecReScanAgg(AggState *node);
diff --git a/src/include/executor/nodeAppend.h b/src/include/executor/nodeAppend.h
index ee0b6ad..4e38a13 100644
--- a/src/include/executor/nodeAppend.h
+++ b/src/include/executor/nodeAppend.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern AppendState *ExecInitAppend(Append *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecAppend(AppendState *node);
extern void ExecEndAppend(AppendState *node);
extern void ExecReScanAppend(AppendState *node);
diff --git a/src/include/executor/nodeBitmapHeapscan.h b/src/include/executor/nodeBitmapHeapscan.h
index f477d1c..c77694c 100644
--- a/src/include/executor/nodeBitmapHeapscan.h
+++ b/src/include/executor/nodeBitmapHeapscan.h
@@ -18,7 +18,6 @@
#include "access/parallel.h"
extern BitmapHeapScanState *ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecBitmapHeapScan(BitmapHeapScanState *node);
extern void ExecEndBitmapHeapScan(BitmapHeapScanState *node);
extern void ExecReScanBitmapHeapScan(BitmapHeapScanState *node);
extern void ExecBitmapHeapEstimate(BitmapHeapScanState *node,
diff --git a/src/include/executor/nodeCtescan.h b/src/include/executor/nodeCtescan.h
index 397bdfd..d2fbcbd 100644
--- a/src/include/executor/nodeCtescan.h
+++ b/src/include/executor/nodeCtescan.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern CteScanState *ExecInitCteScan(CteScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecCteScan(CteScanState *node);
extern void ExecEndCteScan(CteScanState *node);
extern void ExecReScanCteScan(CteScanState *node);
diff --git a/src/include/executor/nodeCustom.h b/src/include/executor/nodeCustom.h
index e81bcf7..a1cc63a 100644
--- a/src/include/executor/nodeCustom.h
+++ b/src/include/executor/nodeCustom.h
@@ -21,7 +21,6 @@
*/
extern CustomScanState *ExecInitCustomScan(CustomScan *custom_scan,
EState *estate, int eflags);
-extern TupleTableSlot *ExecCustomScan(CustomScanState *node);
extern void ExecEndCustomScan(CustomScanState *node);
extern void ExecReScanCustomScan(CustomScanState *node);
diff --git a/src/include/executor/nodeForeignscan.h b/src/include/executor/nodeForeignscan.h
index 3ff4ecd..0b66259 100644
--- a/src/include/executor/nodeForeignscan.h
+++ b/src/include/executor/nodeForeignscan.h
@@ -18,7 +18,6 @@
#include "nodes/execnodes.h"
extern ForeignScanState *ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecForeignScan(ForeignScanState *node);
extern void ExecEndForeignScan(ForeignScanState *node);
extern void ExecReScanForeignScan(ForeignScanState *node);
diff --git a/src/include/executor/nodeFunctionscan.h b/src/include/executor/nodeFunctionscan.h
index 5e830eb..aaa9d8c 100644
--- a/src/include/executor/nodeFunctionscan.h
+++ b/src/include/executor/nodeFunctionscan.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern FunctionScanState *ExecInitFunctionScan(FunctionScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecFunctionScan(FunctionScanState *node);
extern void ExecEndFunctionScan(FunctionScanState *node);
extern void ExecReScanFunctionScan(FunctionScanState *node);
diff --git a/src/include/executor/nodeGather.h b/src/include/executor/nodeGather.h
index b000693..189bd70 100644
--- a/src/include/executor/nodeGather.h
+++ b/src/include/executor/nodeGather.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern GatherState *ExecInitGather(Gather *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecGather(GatherState *node);
extern void ExecEndGather(GatherState *node);
extern void ExecShutdownGather(GatherState *node);
extern void ExecReScanGather(GatherState *node);
diff --git a/src/include/executor/nodeGatherMerge.h b/src/include/executor/nodeGatherMerge.h
index 14b31a0..0154d73 100644
--- a/src/include/executor/nodeGatherMerge.h
+++ b/src/include/executor/nodeGatherMerge.h
@@ -19,7 +19,6 @@
extern GatherMergeState *ExecInitGatherMerge(GatherMerge *node,
EState *estate,
int eflags);
-extern TupleTableSlot *ExecGatherMerge(GatherMergeState *node);
extern void ExecEndGatherMerge(GatherMergeState *node);
extern void ExecReScanGatherMerge(GatherMergeState *node);
extern void ExecShutdownGatherMerge(GatherMergeState *node);
diff --git a/src/include/executor/nodeGroup.h b/src/include/executor/nodeGroup.h
index 7358b61..b0d7e31 100644
--- a/src/include/executor/nodeGroup.h
+++ b/src/include/executor/nodeGroup.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern GroupState *ExecInitGroup(Group *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecGroup(GroupState *node);
extern void ExecEndGroup(GroupState *node);
extern void ExecReScanGroup(GroupState *node);
diff --git a/src/include/executor/nodeHash.h b/src/include/executor/nodeHash.h
index 8052f27..3ae556f 100644
--- a/src/include/executor/nodeHash.h
+++ b/src/include/executor/nodeHash.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern HashState *ExecInitHash(Hash *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecHash(HashState *node);
extern Node *MultiExecHash(HashState *node);
extern void ExecEndHash(HashState *node);
extern void ExecReScanHash(HashState *node);
diff --git a/src/include/executor/nodeHashjoin.h b/src/include/executor/nodeHashjoin.h
index 541c81e..7469bfb 100644
--- a/src/include/executor/nodeHashjoin.h
+++ b/src/include/executor/nodeHashjoin.h
@@ -18,7 +18,6 @@
#include "storage/buffile.h"
extern HashJoinState *ExecInitHashJoin(HashJoin *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecHashJoin(HashJoinState *node);
extern void ExecEndHashJoin(HashJoinState *node);
extern void ExecReScanHashJoin(HashJoinState *node);
diff --git a/src/include/executor/nodeIndexonlyscan.h b/src/include/executor/nodeIndexonlyscan.h
index cf227da..c8a709c 100644
--- a/src/include/executor/nodeIndexonlyscan.h
+++ b/src/include/executor/nodeIndexonlyscan.h
@@ -18,7 +18,6 @@
#include "access/parallel.h"
extern IndexOnlyScanState *ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecIndexOnlyScan(IndexOnlyScanState *node);
extern void ExecEndIndexOnlyScan(IndexOnlyScanState *node);
extern void ExecIndexOnlyMarkPos(IndexOnlyScanState *node);
extern void ExecIndexOnlyRestrPos(IndexOnlyScanState *node);
diff --git a/src/include/executor/nodeIndexscan.h b/src/include/executor/nodeIndexscan.h
index 0118234..1668e34 100644
--- a/src/include/executor/nodeIndexscan.h
+++ b/src/include/executor/nodeIndexscan.h
@@ -18,7 +18,6 @@
#include "nodes/execnodes.h"
extern IndexScanState *ExecInitIndexScan(IndexScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecIndexScan(IndexScanState *node);
extern void ExecEndIndexScan(IndexScanState *node);
extern void ExecIndexMarkPos(IndexScanState *node);
extern void ExecIndexRestrPos(IndexScanState *node);
diff --git a/src/include/executor/nodeLimit.h b/src/include/executor/nodeLimit.h
index 7bb20d9..db65b55 100644
--- a/src/include/executor/nodeLimit.h
+++ b/src/include/executor/nodeLimit.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern LimitState *ExecInitLimit(Limit *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecLimit(LimitState *node);
extern void ExecEndLimit(LimitState *node);
extern void ExecReScanLimit(LimitState *node);
diff --git a/src/include/executor/nodeLockRows.h b/src/include/executor/nodeLockRows.h
index 6b90756..c9d05b8 100644
--- a/src/include/executor/nodeLockRows.h
+++ b/src/include/executor/nodeLockRows.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern LockRowsState *ExecInitLockRows(LockRows *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecLockRows(LockRowsState *node);
extern void ExecEndLockRows(LockRowsState *node);
extern void ExecReScanLockRows(LockRowsState *node);
diff --git a/src/include/executor/nodeMaterial.h b/src/include/executor/nodeMaterial.h
index f69abbc..4b3c257 100644
--- a/src/include/executor/nodeMaterial.h
+++ b/src/include/executor/nodeMaterial.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern MaterialState *ExecInitMaterial(Material *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecMaterial(MaterialState *node);
extern void ExecEndMaterial(MaterialState *node);
extern void ExecMaterialMarkPos(MaterialState *node);
extern void ExecMaterialRestrPos(MaterialState *node);
diff --git a/src/include/executor/nodeMergeAppend.h b/src/include/executor/nodeMergeAppend.h
index 3cc6ef5..a0ccbae 100644
--- a/src/include/executor/nodeMergeAppend.h
+++ b/src/include/executor/nodeMergeAppend.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern MergeAppendState *ExecInitMergeAppend(MergeAppend *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecMergeAppend(MergeAppendState *node);
extern void ExecEndMergeAppend(MergeAppendState *node);
extern void ExecReScanMergeAppend(MergeAppendState *node);
diff --git a/src/include/executor/nodeMergejoin.h b/src/include/executor/nodeMergejoin.h
index 32df25a..d20e415 100644
--- a/src/include/executor/nodeMergejoin.h
+++ b/src/include/executor/nodeMergejoin.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern MergeJoinState *ExecInitMergeJoin(MergeJoin *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecMergeJoin(MergeJoinState *node);
extern void ExecEndMergeJoin(MergeJoinState *node);
extern void ExecReScanMergeJoin(MergeJoinState *node);
diff --git a/src/include/executor/nodeModifyTable.h b/src/include/executor/nodeModifyTable.h
index 5a406f2..a2e7af9 100644
--- a/src/include/executor/nodeModifyTable.h
+++ b/src/include/executor/nodeModifyTable.h
@@ -16,7 +16,6 @@
#include "nodes/execnodes.h"
extern ModifyTableState *ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecModifyTable(ModifyTableState *node);
extern void ExecEndModifyTable(ModifyTableState *node);
extern void ExecReScanModifyTable(ModifyTableState *node);
diff --git a/src/include/executor/nodeNamedtuplestorescan.h b/src/include/executor/nodeNamedtuplestorescan.h
index 7f72fbe..395d978 100644
--- a/src/include/executor/nodeNamedtuplestorescan.h
+++ b/src/include/executor/nodeNamedtuplestorescan.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern NamedTuplestoreScanState *ExecInitNamedTuplestoreScan(NamedTuplestoreScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecNamedTuplestoreScan(NamedTuplestoreScanState *node);
extern void ExecEndNamedTuplestoreScan(NamedTuplestoreScanState *node);
extern void ExecReScanNamedTuplestoreScan(NamedTuplestoreScanState *node);
diff --git a/src/include/executor/nodeNestloop.h b/src/include/executor/nodeNestloop.h
index 8e0fcc1..0d6486c 100644
--- a/src/include/executor/nodeNestloop.h
+++ b/src/include/executor/nodeNestloop.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern NestLoopState *ExecInitNestLoop(NestLoop *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecNestLoop(NestLoopState *node);
extern void ExecEndNestLoop(NestLoopState *node);
extern void ExecReScanNestLoop(NestLoopState *node);
diff --git a/src/include/executor/nodeProjectSet.h b/src/include/executor/nodeProjectSet.h
index 2f6999e..a0b0521 100644
--- a/src/include/executor/nodeProjectSet.h
+++ b/src/include/executor/nodeProjectSet.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern ProjectSetState *ExecInitProjectSet(ProjectSet *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecProjectSet(ProjectSetState *node);
extern void ExecEndProjectSet(ProjectSetState *node);
extern void ExecReScanProjectSet(ProjectSetState *node);
diff --git a/src/include/executor/nodeRecursiveunion.h b/src/include/executor/nodeRecursiveunion.h
index f0eba05..e6ce1b4 100644
--- a/src/include/executor/nodeRecursiveunion.h
+++ b/src/include/executor/nodeRecursiveunion.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern RecursiveUnionState *ExecInitRecursiveUnion(RecursiveUnion *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecRecursiveUnion(RecursiveUnionState *node);
extern void ExecEndRecursiveUnion(RecursiveUnionState *node);
extern void ExecReScanRecursiveUnion(RecursiveUnionState *node);
diff --git a/src/include/executor/nodeResult.h b/src/include/executor/nodeResult.h
index 61d3cb2..20e0063 100644
--- a/src/include/executor/nodeResult.h
+++ b/src/include/executor/nodeResult.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern ResultState *ExecInitResult(Result *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecResult(ResultState *node);
extern void ExecEndResult(ResultState *node);
extern void ExecResultMarkPos(ResultState *node);
extern void ExecResultRestrPos(ResultState *node);
diff --git a/src/include/executor/nodeSamplescan.h b/src/include/executor/nodeSamplescan.h
index ed06e77..607bbd9 100644
--- a/src/include/executor/nodeSamplescan.h
+++ b/src/include/executor/nodeSamplescan.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern SampleScanState *ExecInitSampleScan(SampleScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecSampleScan(SampleScanState *node);
extern void ExecEndSampleScan(SampleScanState *node);
extern void ExecReScanSampleScan(SampleScanState *node);
diff --git a/src/include/executor/nodeSeqscan.h b/src/include/executor/nodeSeqscan.h
index 06e0686..0fba79f 100644
--- a/src/include/executor/nodeSeqscan.h
+++ b/src/include/executor/nodeSeqscan.h
@@ -18,7 +18,6 @@
#include "nodes/execnodes.h"
extern SeqScanState *ExecInitSeqScan(SeqScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecSeqScan(SeqScanState *node);
extern void ExecEndSeqScan(SeqScanState *node);
extern void ExecReScanSeqScan(SeqScanState *node);
diff --git a/src/include/executor/nodeSetOp.h b/src/include/executor/nodeSetOp.h
index af85977..c15f945 100644
--- a/src/include/executor/nodeSetOp.h
+++ b/src/include/executor/nodeSetOp.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern SetOpState *ExecInitSetOp(SetOp *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecSetOp(SetOpState *node);
extern void ExecEndSetOp(SetOpState *node);
extern void ExecReScanSetOp(SetOpState *node);
diff --git a/src/include/executor/nodeSort.h b/src/include/executor/nodeSort.h
index 1d2b713..ed0e9db 100644
--- a/src/include/executor/nodeSort.h
+++ b/src/include/executor/nodeSort.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern SortState *ExecInitSort(Sort *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecSort(SortState *node);
extern void ExecEndSort(SortState *node);
extern void ExecSortMarkPos(SortState *node);
extern void ExecSortRestrPos(SortState *node);
diff --git a/src/include/executor/nodeSubqueryscan.h b/src/include/executor/nodeSubqueryscan.h
index c852e29..710e050 100644
--- a/src/include/executor/nodeSubqueryscan.h
+++ b/src/include/executor/nodeSubqueryscan.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern SubqueryScanState *ExecInitSubqueryScan(SubqueryScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecSubqueryScan(SubqueryScanState *node);
extern void ExecEndSubqueryScan(SubqueryScanState *node);
extern void ExecReScanSubqueryScan(SubqueryScanState *node);
diff --git a/src/include/executor/nodeTableFuncscan.h b/src/include/executor/nodeTableFuncscan.h
index c58156e..c4672c0 100644
--- a/src/include/executor/nodeTableFuncscan.h
+++ b/src/include/executor/nodeTableFuncscan.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern TableFuncScanState *ExecInitTableFuncScan(TableFuncScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecTableFuncScan(TableFuncScanState *node);
extern void ExecEndTableFuncScan(TableFuncScanState *node);
extern void ExecReScanTableFuncScan(TableFuncScanState *node);
diff --git a/src/include/executor/nodeTidscan.h b/src/include/executor/nodeTidscan.h
index d07ed7c..e68aaf3 100644
--- a/src/include/executor/nodeTidscan.h
+++ b/src/include/executor/nodeTidscan.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern TidScanState *ExecInitTidScan(TidScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecTidScan(TidScanState *node);
extern void ExecEndTidScan(TidScanState *node);
extern void ExecReScanTidScan(TidScanState *node);
diff --git a/src/include/executor/nodeUnique.h b/src/include/executor/nodeUnique.h
index 3d0ac9d..008774a 100644
--- a/src/include/executor/nodeUnique.h
+++ b/src/include/executor/nodeUnique.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern UniqueState *ExecInitUnique(Unique *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecUnique(UniqueState *node);
extern void ExecEndUnique(UniqueState *node);
extern void ExecReScanUnique(UniqueState *node);
diff --git a/src/include/executor/nodeValuesscan.h b/src/include/executor/nodeValuesscan.h
index c28bb1a..772a5e9 100644
--- a/src/include/executor/nodeValuesscan.h
+++ b/src/include/executor/nodeValuesscan.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern ValuesScanState *ExecInitValuesScan(ValuesScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecValuesScan(ValuesScanState *node);
extern void ExecEndValuesScan(ValuesScanState *node);
extern void ExecReScanValuesScan(ValuesScanState *node);
diff --git a/src/include/executor/nodeWindowAgg.h b/src/include/executor/nodeWindowAgg.h
index db1ad60..1c17730 100644
--- a/src/include/executor/nodeWindowAgg.h
+++ b/src/include/executor/nodeWindowAgg.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern WindowAggState *ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecWindowAgg(WindowAggState *node);
extern void ExecEndWindowAgg(WindowAggState *node);
extern void ExecReScanWindowAgg(WindowAggState *node);
diff --git a/src/include/executor/nodeWorktablescan.h b/src/include/executor/nodeWorktablescan.h
index c222d9f..df05e75 100644
--- a/src/include/executor/nodeWorktablescan.h
+++ b/src/include/executor/nodeWorktablescan.h
@@ -17,7 +17,6 @@
#include "nodes/execnodes.h"
extern WorkTableScanState *ExecInitWorkTableScan(WorkTableScan *node, EState *estate, int eflags);
-extern TupleTableSlot *ExecWorkTableScan(WorkTableScanState *node);
extern void ExecEndWorkTableScan(WorkTableScanState *node);
extern void ExecReScanWorkTableScan(WorkTableScanState *node);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 85fac8a..35c28a6 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -818,6 +818,18 @@ typedef struct DomainConstraintState
* ----------------------------------------------------------------
*/
+struct PlanState;
+
+/* ----------------
+ * ExecProcNodeMtd
+ *
+ * This is the method called by ExecProcNode to return the next tuple
+ * from an executor node. It returns NULL, or an empty TupleTableSlot,
+ * if no more tuples are available.
+ * ----------------
+ */
+typedef TupleTableSlot *(*ExecProcNodeMtd) (struct PlanState *pstate);
+
/* ----------------
* PlanState node
*
@@ -835,6 +847,10 @@ typedef struct PlanState
* nodes point to one EState for the whole
* top-level plan */
+ ExecProcNodeMtd ExecProcNode; /* function to return next tuple */
+ ExecProcNodeMtd ExecProcNodeReal; /* actual function, if above is a
+ * wrapper */
+
Instrumentation *instrument; /* Optional runtime stats for this node */
WorkerInstrumentation *worker_instrument; /* per-worker instrumentation */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi, On 2017-07-29 16:14:08 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > [ 0002-Move-ExecProcNode-from-dispatch-to-function-pointer-.patch ] > > Here's a reviewed version of this patch. Thanks! I pushed both now. > I added dummy ExecProcNodeMtd functions to the various node types that > lacked them because they expect to be called through MultiExecProcNode > instead. In the old coding, trying to call ExecProcNode on one of those > node types would have led to a useful error message; as you had it, > it'd have dumped core, which is not an improvement. Ok, makes sense. > Also, I removed the ExecReScan stanza from ExecProcNodeFirst; that > should surely be redundant, because we should only get to that function > through ExecProcNode(). If somehow it's not redundant, please add a > comment explaining why not. Makes sense too. > Some other minor cosmetic changes, mostly comment wordsmithing. Thanks! Julien, could you quickly verify that everything's good for you now too? Regards, Andres
On 31/07/2017 01:47, Andres Freund wrote: > Julien, could you quickly verify that everything's good for you now too? > I just checked on current HEAD (cc9f08b6b813e30789100b6b34110d8be1090ba0) and everything's good for me. Thanks! -- Julien Rouhaud http://dalibo.com - http://dalibo.org
On Fri, Jul 28, 2017 at 02:42:06PM -0400, Robert Haas wrote: > On Fri, Jul 28, 2017 at 12:29 PM, Noah Misch <noah@leadboat.com> wrote: > > Your colleagues achieve compliance despite uncertainty; for inspiration, I > > recommend examining Alvaro's status updates as examples of this. The policy > > currently governs your open items even if you disagree with it. Thanks for resolving this open item. > I emphatically agree with that. If the RMT is to accomplish its > purpose, it must be able to exert authority even when an individual > contributor doesn't like the decisions it makes. > > On the other hand, nothing in the open item policy the current RMT has > adopted prohibits you from using judgement about when and how > vigorously to enforce that policy in any particular case, and I would > encourage you to do so. I understand. When it comes to open item regulation, the aspects that keep me up at night are completeness and fairness. Minimizing list traffic about non-compliant open items is third priority at best. Furthermore, it takes an expensive and subjective calculation to determine whether a policy-violating open item is progressing well. I will keep an eye out for minor policy violations that I can ignore cheaply and fairly.