From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Julien Rouhaud <julien.rouhaud@dalibo.com>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: segfault in HEAD when too many nested functions call
In-reply-to: <20170721114023.fj6cvr6og46yogei@alap3.anarazel.de>
References: <b0af9eaa-130c-60d0-9e4e-7a135b1e0c76@dalibo.com>
 <12302.1500132157@sss.pgh.pa.us>
 <20170717072629.dqykbsot2irdob7c@alap3.anarazel.de>
 <20170717145710.cm32m4yn555hsoxd@alap3.anarazel.de>
 <6687.1500411223@sss.pgh.pa.us>
 <20170721114023.fj6cvr6og46yogei@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Fri, 21 Jul 2017 04:40:23 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <19898.1500682674.1@sss.pgh.pa.us>
Date: Fri, 21 Jul 2017 20:17:54 -0400
Message-ID: <19899.1500682674@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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