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

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Amit Langote <amitlangote09(at)gmail(dot)com>
Subject: Re: BUG #15212: Default values in partition tables don't work as expected and allow NOT NULL violation
Date: 2018-11-05 22:25:09
Message-ID: 20181105222509.k26zipgeixc6fkjf@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs pgsql-hackers

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

Attachment Content-Type Size
PG10-v4-0001-Fix-bug-regarding-partition-column-option-inherit.patch text/x-diff 11.5 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Michael Paquier 2018-11-06 00:14:07 Re: BUG #15437: Segfault during insert into declarative partitioned table with a trigger creating partition
Previous Message Tom Lane 2018-11-05 20:16:17 Re: Ris: BUG #15482: Foreign keys to a partition (NOT A PARTITIONED) break the server

Browse pgsql-hackers by date

  From Date Subject
Next Message Tatsuo Ishii 2018-11-05 23:43:03 Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan
Previous Message Andres Freund 2018-11-05 22:11:42 Re: Why do pg_upgrade's test use the serial schedule?