Re: BUG #15668: Server crash in transformPartitionRangeBounds

Hi,

Thanks for splitting. It makes sense, because, as you know, the bug that
causes the crash is a separate problem from unintuitive error messages
which result from the way in which we parse expressions.

On 2019/03/22 14:09, Michael Paquier wrote:
> On Wed, Mar 20, 2019 at 06:17:27PM +0900, Michael Paquier wrote:
>> The thing is that in order to keep the tests for the crash, we finish
>> with the inintuitive RTE-related errors, so it is also inconsistent to
>> not group things..
>
> As I have seen no feedback from others regarding the changes in error
> messages depending on the parsing context, so I have been looking at
> splitting the fix for the crash and changing the error messages, and
> attached is the result of the split (minus the commit messages). The
> first patch fixes the crash, and includes test cases to cover the
> crash as well as extra cases for list and range strategies with
> partition bounds. Some of the error messages are confusing, but that
> fixes the issue. This is not the most elegant thing without the
> second patch, but well that could be worse.

A comment on this one:

+ if (cname == NULL)
+ {
+ /*
+ * No field names have been found, meaning that there
+ * is not much to do with special value handling. Instead
+ * let the expression transformation handle any errors and
+ * limitations.
+ */

This comment sounds a bit misleading. The code above this "did" find
field names, but there were too many. What this comment should mention is
that parsing didn't return a single field name, which is the format that
the code below this can do something useful with. I had proposed that
upthread, but maybe that feedback got lost in the discussion about other
related issues.

I had proposed this:

+ /*
+ * There should be a single field named "minvalue" or "maxvalue".
+ */
if (list_length(cref->fields) == 1 &&
IsA(linitial(cref->fields), String))
cname = strVal(linitial(cref->fields));

- Assert(cname != NULL);
- if (strcmp("minvalue", cname) == 0)
+ if (cname == NULL)
+ {
+ /*
+ * ColumnRef is not in the desired single-field-name form; for
+ * consistency, let transformExpr() report the error rather
+ * than doing it ourselves.
+ */
+ }

Maybe that could use few more tweaks, but hope I've made my point.

+CREATE TABLE part_bogus_expr_fail PARTITION OF range_parted
+ FOR VALUES FROM (sum(a)) TO ('2019-01-01');
+ERROR: function sum(date) does not exist
+LINE 2: FOR VALUES FROM (sum(a)) TO ('2019-01-01');

Maybe, we should add to this patch only the tests relevant to the cases
that would lead to crash without this patch.

Tests regarding error messages fine tuning can be added in the other patch.

> The second patch adds better error context for the different error
> messages, and includes tests for default expressions, which we could
> discuss in a separate thread. So I am not proposing to commit that
> without more feedback.

A separate thread will definitely attract more attention, at least in due
time. :)

Thanks,
Amit