From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: Julien Rouhaud <julien.rouhaud@dalibo.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: segfault in HEAD when too many nested functions call
Date: Fri, 21 Jul 2017 20:17:54 -0400
Message-ID: <19899.1500682674@sss.pgh.pa.us> (raw)
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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, julien.rouhaud@dalibo.com
Subject: Re: segfault in HEAD when too many nested functions call
In-Reply-To: <19899.1500682674@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox