segfault in HEAD when too many nested functions call

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]/messages/by-id/22833.1490390175@sss.pgh.pa.us. 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 int
LANGUAGE plpgsql
AS $$
DECLARE
rec 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]: /messages/by-id/22833.1490390175@sss.pgh.pa.us

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com 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]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

--
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-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 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).

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

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

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

--
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-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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

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

--
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-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 to actually 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
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-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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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:
/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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:
/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

I sent out a note fleshed out patch last week, which Tom reviewed. Planning to update it to address that review today or tomorrow.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

--
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-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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers