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

Hi,

Thank you for looking at this.

On 2018/11/06 7:25, Alvaro Herrera wrote:
> 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.

The quoted comment is an explanation I wrote to describe why I think the
*existing* statement (shown below) is correct.

coldef->constraints = restdef->constraints;

As you've already figured out, 'coldef' here refers (referred) to the
inherited column definition and 'restdef' to the partition's local
definition. Ashutosh asked in his review why doing this is OK, because it
appeared that it's essentially leaking/overwriting inherited constraints.
The comment I added was to try to assure future readers that the inherited
constraints have already been linked into into another output variable and
so no constraints are being leaked.

As for why ignoring partition's local constraints (restdef->constraints)
didn't break anything, I see that's because transformCreateStmt would
already have added them to CreateStmt.constraints, so they're already
taken care of.

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

I had hit some error when I made the partition case reuse the code that
handles the OF type case, the details of which unfortunately I don't
remember. Looking at your take, I can't now think of some case that's
being broken with it. It's definitely nice that the same strange piece of
code is not repeated twice now.

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

:(. That thing is never meaningful/true outside MergeAttributes().

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

Yeah, although there seems to be a typo above: s/==/=/g

Thanks,
Amit