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 | Resend email |
Thread: | |
Lists: | pgsql-bugs pgsql-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 |
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 |
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 |