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

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Cc: 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-07-12 08:59:02
Message-ID: 62378a90-f5cc-3b83-3f97-bd3273e23fc8@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-bugspgsql-hackers

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.

> 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

Attachment Content-Type Size
PG10-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patch text/plain 5.5 KB
PG11-HEAD-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patch text/plain 5.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2018-07-12 09:06:16 Re: Negotiating the SCRAM channel binding type
Previous Message Marina Polyakova 2018-07-12 08:57:01 Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

Browse pgsql-bugs by date

  From Date Subject
Next Message Arthur Zakirov 2018-07-12 09:22:06 Re: BUG #15277: ts_headline strips things that look like HTML tags and it cannot be disabled
Previous Message PG Bug reporting form 2018-07-12 07:59:40 BUG #15277: ts_headline strips things that look like HTML tags and it cannot be disabled