Re: BUG #11335: an invalid prepare statement causes crash at log_statement = 'mod' or 'ddl'.

Fujii Masao <masao.fujii@gmail.com> writes:
> On Thu, Sep 4, 2014 at 3:55 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> Thanks for reporting the bug! This segmentation fault could reproduce
>> even on my machine. Barring any objection, I will apply the change that
>> you suggested.

> Done.

I don't think this fix is either appropriate or adequate.  Stylistically,
it's bogus that this function should be prepared for a null argument when
similar command-tag-related functions right next to it are not.  Moreover,
what is the reasoning why the correct answer for a null argument is
LOGSTMT_ALL and not something else?  It'd be at least as legit to make
it LOGSTMT_NONE, for instance.

As far as adequacy goes, the actual bug seems to be that this bit of code
is not expecting a plansource's raw_parse_tree to ever be NULL:

                /* Look through an EXECUTE to the referenced stmt */
                ps = FetchPreparedStatement(stmt->name, false);
                if (ps)
                    lev = GetCommandLogLevel(ps->plansource->raw_parse_tree);
                else
                    lev = LOGSTMT_ALL;

and it is not terribly hard to find other places making the same
assumption, which is probably reasonable because the comments around
the plansource files don't suggest it's legal either.  So I don't think
this has exhausted the number of ways to crash the backend after creating
such a prepared statement.

I think we should revert the patch as applied and instead think about
whether it's really legit to have a null raw_parse_tree in a cached plan.
If it is, the plansource comments need adjustment and so do several other
places in the code.  I'd also say that the right place to adjust
GetCommandLogLevel is in the bit I quoted above, where we know that
what we're talking about is EXECUTE on an empty prepared query, and
so there's a more principled basis for determining what its logging
level ought to be.

It might be that we'd be better off inventing some kind of EmptyStmt
parse tree for this case.  Or maybe we should reconsider whether
exec_parse_message should allow the case at all.  It's not unreasonable
that Parse should require exactly one SQL statement, not just "at most
one".

            regards, tom lane