Re: BUG #15212: Default values in partition tables don't work asexpected and allow NOT NULL violation - Mailing list pgsql-hackers

On Thu, Jul 12, 2018 at 2:29 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Thanks Ashutosh.
>
> On 2018/07/10 22:50, Ashutosh Bapat wrote:
>> I didn't see any hackers thread linked to this CF entry. Hence sending this mail through CF app.
>
> Hmm, yes.  I hadn't posted the patch to -hackers.
>
>> The patch looks good to me. It applies cleanly, compiles cleanly and make check passes.
>>
>> I think you could avoid condition
>> +                       /* Do not override parent's NOT NULL constraint. */
>> +                       if (restdef->is_not_null)
>> +                           coldef->is_not_null = restdef->is_not_null;
>>
>> by rewriting this line as
>> coldef->is_not_null = coldef->is_not_null || restdef->is_not_null;
>
> Agreed, done like that.
>
>> The comment looks a bit odd either way since we are changing coldef->is_not_null based on restdef->is_not_null.
That'sbecause of the nature of boolean variables. May be a bit of explanation is needed.
 
>
> 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.

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

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?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company