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

On Thu, Jul 12, 2018 at 2:29 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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's because 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