Re: BUG #15212: Default values in partition tables don't work as expected and allow NOT NULL violation

Thanks Ashutosh, and sorry that I somehow missed replying to this.

On 2018/07/13 22:50, Ashutosh Bapat wrote:
> On Thu, Jul 12, 2018 at 2:29 PM, Amit Langote wrote:
>> I have modified the comments around this code in the updated patch.
>
> + /*
> + * Each member in 'saved_schema' contains a ColumnDef containing
> + * partition-specific options for the column. Below, we merge the
> + * information from each into the ColumnDef of the same name found in
> + * the original 'schema' list before deleting it from the list. So,
> + * once we've finished processing all the columns from the original
> + * 'schema' list, there shouldn't be any ColumnDefs left that came
> + * from the 'saved_schema' list.
> + */
>
> This is conveyed by the prologue of the function.
>
>
> + /*
> + * Combine using OR so that the NOT NULL constraint
> + * in the parent's definition doesn't get lost.
> + */
> This too is specified in prologue as
> * Constraints (including NOT NULL constraints) for the child table
> * are the union of all relevant constraints, from both the child schema
> * and parent tables.
>
> So, I don't think we need any special mention of OR, "union" conveys
> the intention.

OK, I have removed both comments.

>>> On a side note, I see
>>> coldef->constraints = restdef->constraints;
>>> Shouldn't we merge the constraints instead of just overwriting those?
>>
>> Actually, I checked that coldef->constraints is always NIL in this case.
>> coldef (ColumnDef) is constructed from a member in the parent's TupleDesc
>> earlier in that function and any constraints that may be present in the
>> parent's definition of the column are saved in a separate variable that's
>> returned to the caller as one list containing "old"/inherited constraints.
>> So, no constraints are getting lost here.
>
> In case of multiple inheritance coldef->constraints is "union" of
> constraints from all the parents as described in the prologue.

AFAICS, the prologue doesn't mention *just* coldef->constraints. It talks
about both the constraints that are to be specified using various
ColumnDef fields (is_not_null, raw_default, cooked_default, etc.) and
constraints that are to be returned in the *supconstr output list. Both
types of constraints are obtained by union'ing corresponding values from
all parents and child's own definition.

> But in
> case of partitioning there is only one parent and hence
> coldef->constraints is NULL and hence just overwriting it works. I
> think we need comments mentioning this special case.

That's what I wrote in this comment:

/*
* Parent's constraints, if any, have been saved in
* 'constraints', which are passed back to the caller,
* so it's okay to overwrite the variable like this.
*/

> Also, I am not sure whether we really need all conditions related to
> raw_default and cooked_default. Do you have any testcase showing the
> need for those changes?

Without the patch (example below is tested on PG 10, but same is true with
PG 11 and HEAD too):

create table parent (a int, b int default -1) partition by list (a);
create table child partition of parent (b not null) for values in (0);

\d parent
Table "public.parent"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼───────────────
a │ integer │ │ │
b │ integer │ │ │ '-1'::integer
Partition key: LIST (a)
Number of partitions: 1 (Use \d+ to list them.)

\d child
Table "public.child"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ │
b │ integer │ │ not null │
Partition of: parent FOR VALUES IN (0)

Note that child didn't inherit -1 as default for b. That happens, because
the partition-specific ColumnDef for b, created to contain the NOT NULL
constraint, has its raw_default and cooked_default fields set to NULL.
The current code blindly assigns that ColumnDef's raw_default and
cooked_default to the inherited ColumnDef's corresponding fields.
Overriding raw_default like that is fine, because partition-specified
default get priority, but not cooked_default, which may contain the
inherited default.

That's not an issue if child's definition doesn't specify any of its own
constraints:

create table parent (a int, b int default -1) partition by list (a);
create table child partition of parent for values in (0);

\d child
Table "public.child"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼───────────────
a │ integer │ │ │
b │ integer │ │ │ '-1'::integer
Partition of: parent FOR VALUES IN (0)

That's because there is no partition-specific ColumnDef to override
anything in this case.

Anyway, I updated that code to look like this:

+ /*
+ * If the partition's definition specifies a default,
+ * it's in restdef->raw_default, which if non-NULL,
+ * overrides the parent's default that's in
+ * coldef->cooked_default.
+ */
+ if (restdef->raw_default)
+ {
+ coldef->raw_default = restdef->raw_default;
+ coldef->cooked_default = NULL;
+ }
+
+ /*
+ * coldef->cooked_default would contain the inherited
+ * default, unless overridden above. Don't try to
+ * override it with NULL.
+ */
+ Assert(restdef->cooked_default == NULL);

Also, the patch already adds a test case that demonstrates what this code
does, but I modified it a bit in the updated version to also show the fix
for $subject. Now its expected output looks like this:

+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null
default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of
parted_notnull_inh_test (a not null, b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | not null | 1
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;

Just to clarify what's different in that \d output, here is the output
with unmodified PG 10:

\d parted_notnull_inh_test1
Table "public.parted_notnull_inh_test1"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ not null │
b │ integer │ │ │ 1
Partition of: parted_notnull_inh_test FOR VALUES IN (1)

As can be seen, PG 10 loses inherited values of a's default and b's NOT NULL.

Please find attached updated patches for PG 10, PG 11 / HEAD, with changes
I mentioned above.

Thanks,
Amit