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

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.

Thanks,
nm