On Thu, Jul 12, 2018 at 2:29 PM, Amit Langote <langote_amit...@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'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
