Re: BUG #15668: Server crash in transformPartitionRangeBounds

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, 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.

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).

+    * 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.

 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.

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, 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.

What do you think?
--
Michael