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