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

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.

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

Attached is an updated patch.  I've updated some nearby comments as the
code around here seems pretty confusing, which I thought after having
returned to it after a while.

I have attached both a patch for PG10 and PG11/HEAD branches, which are
actually not all that different from each other.

Thanks,
Amit