Re: fatal flex error in guc-file.l kills the postmaster

On Tue, Jan 17, 2012 at 1:23 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Mon, Jan 16, 2012 at 08:54:53PM -0500, Robert Haas wrote:
>> On Sun, Dec 18, 2011 at 11:53 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> > Here's a version that calls sigsetjmp() once per file. ?While postgresql.conf
>> > scanning hardly seems like the place to be shaving cycles, this does catch
>> > fatal errors in functions other than yylex(), notably yy_create_buffer().
>>
>> This strikes me as more clever than necessary:
>>
>> #define fprintf(file, fmt, msg) \
>>     0; /* eat cast to void */ \
>>     GUC_flex_fatal_errmsg = msg; \
>>     siglongjmp(*GUC_flex_fatal_jmp, 1)
>>
>> Can't we just define a function jumpoutoftheparser() here and call
>> that function rather than doing that /* eat cast to void */ hack?
>> That would also involve fewer assumptions about the coding style
>> emitted by flex.  For example, if flex chose to do something like:
>>
>>   if (bumpity) fprintf(__FILE__, "%s", "dinglewump");
>>
>> ...the proposed definition would be a disaster.  I doubt that inlining
>> is a material performance benefit here; siglongjmp() can't be all that
>> cheap, and the error path shouldn't be all that frequent.
>>
>> Instead of making ParseConfigFp responsible for restoring
>> GUC_flex_fatal_jmp after calling anything that might recursively call
>> ParseConfigFp, I think it would make more sense to define it to stash
>> away the previous value and restore it on exit.  That way it wouldn't
>> need to know which of the things that it calls might in turn
>> recursively call it, which seems likely to reduce the chances of
>> present or future bugs.  A few comments about whichever way we go with
>> it seem like a good idea, too.
>
> Agreed on all points.  I also changed the save/restore of ConfigFileLineno to
> work the same way.  New version attached.

OK, committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company