Re: BUG #15668: Server crash in transformPartitionRangeBounds

On 2019/03/13 14:15, Amit Langote wrote:
> On 2019/03/11 16:21, Michael Paquier wrote:
>> On Mon, Mar 11, 2019 at 03:44:39PM +0900, Amit Langote wrote:
>>> We could make the error message more meaningful depending on the context,
>>> but maybe it'd better be pursue it as a separate project.
>>
>> Yeah, I noticed that stuff when working on it this afternoon.  The
>> error message does not completely feel right even in your produced
>> tests.  Out of curiosity I have been working on this thing myself,
>> and it is possible to have a context-related message.  Please see
>> attached, that's in my opinion less confusing, and of course
>> debatable.  Still this approach does not feel completely right either
>> as that means hijacking the code path which generates a generic
>> message for missing RTEs. :(
> 
> @@ -3259,6 +3259,9 @@ errorMissingRTE(ParseState *pstate, RangeVar
> +     *
> +     * Also, in the context of parsing a partition bound, produce a more
> +     * helpful error message.
>       */
>      if (rte && rte->alias &&
>          strcmp(rte->eref->aliasname, relation->relname) != 0 &&
> 
> 
> -    if (rte)
> +    if (pstate->p_expr_kind == EXPR_KIND_PARTITION_BOUND)
> +        ereport(ERROR,
> +                (errcode(ERRCODE_UNDEFINED_TABLE),
> +                 errmsg("invalid reference in partition bound expression for table
> \"%s\"",
> +                        relation->relname)));
> +    else if (rte)
> 
> Hmm, it seems odd to me that it's OK for default expressions to emit the
> "missing RTE" error, whereas partition bound expressions would emit this
> special error message?
> 
> create table foo (a int default (bar.a));
> ERROR:  missing FROM-clause entry for table "bar"

Looking into this a bit more, I wonder if it would be a good idea to to
have one of those error-emitting switch (pstate->p_expr_kind) blocks in
transformColumnRef(), as shown in the attached patch?

For example, the following error is emitted by one of such blocks that's
in transformSubLink():

create table foo (a int default (select * from non_existent_table));
ERROR:  cannot use subquery in DEFAULT expression

However, if we decide to go with this approach, we will start getting
different error messages from what HEAD gives in certain cases.  With the
patch, you will get the following error when trying to use an aggregate
function in for DEFAULT expression:

create table foo (a int default (avg(foo.a)));
ERROR:  cannot use column reference in DEFAULT expression

but on HEAD, you get:

create table foo (a int default (avg(foo.a)));
ERROR:  aggregate functions are not allowed in DEFAULT expressions

That's because, on HEAD, transformAggregateCall() (or something that calls
it) first calls transformColumnRef() to resolve 'foo.a', which checks that
foo.a is a valid column reference, but it doesn't concern itself with the
fact that the bigger expression it's part of is being used for DEFAULT
expression.  It's only after 'foo.a' has been resolved as a valid column
reference that check_agglevels_and_constraints(), via
transformAggregateCall, emits an error that the overall expression is
invalid to use as a DEFAULT expression.  With patches, error will be
emitted even before resolving 'foo.a'.

While transformAggregateCall() works like that, transformSubLink(), which
I mentioned in the first example, doesn't bother to analyze the query
first (select * from non_existent_table) to notice that the referenced
table doesn't exist.  If it had bothered to analyze the query first, we
would've most likely gotten an error from errorMissingRTE(), not what we
get today.  So, there are certainly some inconsistencies even today in how
these errors are emitted.

Thanks,
Amit