On 2018-Aug-07, Amit Langote wrote:
> > 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.
>
> That's what I wrote in this comment:
>
> /*
> * Parent's constraints, if any, have been saved in
> * 'constraints', which are passed back to the caller,
> * so it's okay to overwrite the variable like this.
> */
What is this for? I tried commenting out that line to see what
test breaks, and found none.
I tried to figure it out, so while thinking what exactly is "restdef" in
that block, it struck me that we seem to be doing things in quite a
strange way there by concatenating both schema lists. I changed it so
that that block scans only the "saved_schema" list (ie. the
partition-local column list definition), searching the other list for
each matching item. This seems a much more natural implementation -- at
least, it results in less list mangling and shorter code, so.
One thing that broke was that duplicate columns in the partition-local
definition would not be caught. However, we already have that check a
few hundred lines above in the same function ... which was skipped for
partitions because of list-mangling that was done before that. I moved
the NIL-setting of schema one block below, and then all tests pass.
One thing that results is that is_from_parent becomes totally useless
and can be removed. It could in theory be removed from ColumnDef, if it
wasn't for the ABI incompatibility that would cause.
BTW this line:
coldef->is_not_null == (coldef->is_not_null || restdef->is_not_null)
can be written more easily like:
coldef->is_not_null |= restdef->is_not_null;
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services