Re: BUG #15668: Server crash in transformPartitionRangeBounds

Hi,

On 2019/03/20 11:07, Michael Paquier wrote:
> On Thu, Mar 14, 2019 at 01:23:08PM +0900, Michael Paquier wrote:
>> I actually think that what you propose here makes more sense than what
>> HEAD does because the most inner expression gets evaluated first.
>> This for example generates the same error as on HEAD:
>> =# create table foo (a int default (avg(1)));
>> ERROR: 42803: aggregate functions are not allowed in DEFAULT expressions
>
> I have been working on that,

Thanks a lot.

> and in the case of a non-existing column
> the patch would generate the following error on HEAD:
> ERROR: 42703: column "non_existent" does not exist
> But with the patch we get that:
> ERROR: cannot use column reference in DEFAULT expression
>
> Still I think that this looks right as we should not have any direct
> column reference anyway, and it keeps the code more simple. So I have
> added more tests to cover those grounds.

I agree that we should error out immediately, once we encounter an (sub-)
expression that's not supported by a given feature (default, partition
bounds, etc.)

Actually, doesn't that mean we should error out because of the aggregate
in the following example:

create table foo (a int default (avg(a));

because we can notice the aggregate before we look into its arguments.
Maybe, we should move the error-checking switch to a point before checking
the arguments? That looks slightly more drastic change to make though.

> The docs of CREATE TABLE are actually wrong, right? It mentions that
> "subqueries and cross-references to other columns in the current table
> are not allowed", but cookDefault() rejects any kind of column
> references anyway for default expressions, including references to the
> column which uses the default expression (or we talk about generated
> columns here still if I recall Peter Eisentraunt's patch correctly
> generated columns don't allow references to the column using the
> expression itself, which is logic by the way).

Yeah, the documentation in your patch looks correct at a first glance.

> + * transformExpr() should have already rejected column references,
> + * subqueries, aggregates, window functions, and SRFs, based on the
> + * EXPR_KIND_ for a default expression.
> */
> I would have added an assertion here, perhaps an elog(). Same remark
> for cookDefault(). The attached patch has an assertion.

+1

> CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES
> IN (sum(a));
> -ERROR: aggregate functions are not allowed in partition bound
> +ERROR: cannot use column reference in partition bound expression
> It would be nice to also test the case where an aggregate is
> forbidden, so I have added a test with sum(1) instead of a column
> reference.

As I said above, maybe we should try to rearrange things so that we get
the former error in both cases.

> We never actually tested in the tree the case of subqueries and SRFs
> used in default expressions, so added.
>
> The last patch you sent did not fix the original problem of the
> thread. That was intentional from your side I guess to show your
> point,

Yeah, that's right.

> still we are touching the same area of the code so I propose to
> fix everything together, and to improve the test coverage for list and
> range strategies. In order to achieve that, I have merged my previous
> proposal into your patch, and added more tests. The new tests for the
> range strategy reproduce the crash. The result is attached.

We may want to fix the crash first. It might be better to hear other
opinions before doing something about the error messages.

Thanks,
Amit