BUG #15212: Default values in partition tables don't work as expected and allow NOT NULL violation
The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:
I found unexpected behavior when playing around with declarative
partitioning.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table. The easiest way to reproduce this is:
jue=> create table ptest (a int, b int) partition by list (a);
CREATE TABLE
jue=> create table ptest1 partition of ptest (b default 7) for values in
(1);
CREATE TABLE
jue=> insert into ptest (a) values (1);
INSERT 0 1
jue=> table ptest;
a | b
---+---
1 |
(1 row)
jue=> insert into ptest1 (a) values (1);
INSERT 0 1
jue=> table ptest;
a | b
---+---
1 |
1 | 7
(2 rows)
Second, this is a way to violate a NOT NULL constraint, presumably because a
default value should be applied later but isn't:
jue=> create table ptest (a int, b int not null) partition by list (a);
CREATE TABLE
jue=> create table ptest1 partition of ptest (b default 7) for values in
(1);
CREATE TABLE
jue=> insert into ptest (a) values (1);
INSERT 0 1
jue=> select * from ptest where b is null;
a | b
---+---
1 |
(1 row)
The same happens for defaults using nextval(sequence), either if specified
directly or as SERIAL columns with ALTER TABLE ... ATTACH PARTITION. My
current workaround is to use a before-row trigger to apply the default
value.
Hello.
On 2018/05/28 9:30, PG Bug reporting form wrote:
The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:I found unexpected behavior when playing around with declarative
partitioning.
Thank you for reporting this and sorry it took a while to reply here.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table. The easiest way to reproduce this is:jue=> create table ptest (a int, b int) partition by list (a);
CREATE TABLE
jue=> create table ptest1 partition of ptest (b default 7) for values in
(1);
CREATE TABLE
jue=> insert into ptest (a) values (1);
INSERT 0 1
jue=> table ptest;
a | b
---+---
1 |
(1 row)jue=> insert into ptest1 (a) values (1);
INSERT 0 1
jue=> table ptest;
a | b
---+---
1 |
1 | 7
(2 rows)The same happens for defaults using nextval(sequence), either if specified
directly or as SERIAL columns with ALTER TABLE ... ATTACH PARTITION.
Hmm, so we provide the ability to specify default values per partition,
but it is not applied when inserting through the parent. I'd like to hear
from others on whether we should fix things so that we fill the
partition's default value for a given column if it's null in the input
tuple, after that tuple is routed to that partition. It does seem like a
inconvenience to have to do it through workarounds like a BR trigger.
Actually, default value substitution happens much earlier in the query
rewrite phase, whereas the partition to actually insert the tuple into
(that is, tuple routing) is determined much later during the execution of
the query. So fixing this will require some work.
Second, this is a way to violate a NOT NULL constraint, presumably because a
default value should be applied later but isn't:jue=> create table ptest (a int, b int not null) partition by list (a);
CREATE TABLE
jue=> create table ptest1 partition of ptest (b default 7) for values in
(1);
CREATE TABLE
jue=> insert into ptest (a) values (1);
INSERT 0 1
jue=> select * from ptest where b is null;
a | b
---+---
1 |
(1 row)
This is clearly a bug of CREATE TABLE .. PARTITION OF. It seems that the
parent's NOT NULL constraint is not copied to the partition when a clause
to set other column options, such as default 7 above, is used in the
command to create a partition. It *is* successfully copied when such a
clause is not specified. For example, same example but without the
default value clause will lead to correct behavior wrt NOT NULL constraint.
create table p (a int, b int not null) partition by list (a);
-- note there is no (b default 7) clause being used here
create table p1 partition of p for values in (1);
-- NOT NULL constraint is correctly enforced
insert into p values (1);
ERROR: null value in column "b" violates not-null constraint
DETAIL: Failing row contains (1, null).
Attached patches fix that for PG 10 (patch filename starting with PG10-)
and HEAD branches, respectively.
Thanks,
Amit
Attachments:
0001-Fix-bug-that-partition-won-t-inherit-NOT-NULL-if-def.patchtext/plain; charset=UTF-8; name=0001-Fix-bug-that-partition-won-t-inherit-NOT-NULL-if-def.patchDownload+21-2
PG10-0001-Fix-bug-that-partition-won-t-inherit-NOT-NULL-if-def.patchtext/plain; charset=UTF-8; name=PG10-0001-Fix-bug-that-partition-won-t-inherit-NOT-NULL-if-def.patchDownload+21-2
On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Hello.On 2018/05/28 9:30, PG Bug reporting form wrote:
The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:I found unexpected behavior when playing around with declarative
partitioning.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table.Hmm, so we provide the ability to specify default values per partition,
but it is not applied when inserting through the parent. I'd like to hear
from others on whether we should fix things so that we fill the
partition's default value for a given column if it's null in the input
tuple, after that tuple is routed to that partition. It does seem like a
inconvenience to have to do it through workarounds like a BR trigger.Actually, default value substitution happens much earlier in the query
rewrite phase, whereas the partition to actually insert the tuple into
(that is, tuple routing) is determined much later during the execution of
the query. So fixing this will require some work.
Well, since documentation says that partitioning build on top of inheritance,
and for inheritance:
If the new table explicitly specifies a default value for the column, this
default overrides any defaults from inherited declarations of the column.
So one may think it should be the same for partitioning as well.
On 7 June 2018 at 15:53, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Hello.On 2018/05/28 9:30, PG Bug reporting form wrote:
The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:I found unexpected behavior when playing around with declarative
partitioning.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table.Hmm, so we provide the ability to specify default values per partition,
but it is not applied when inserting through the parent. I'd like to hear
from others on whether we should fix things so that we fill the
partition's default value for a given column if it's null in the input
tuple, after that tuple is routed to that partition. It does seem like a
inconvenience to have to do it through workarounds like a BR trigger.Actually, default value substitution happens much earlier in the query
rewrite phase, whereas the partition to actually insert the tuple into
(that is, tuple routing) is determined much later during the execution of
the query. So fixing this will require some work.Well, since documentation says that partitioning build on top of inheritance,
and for inheritance:If the new table explicitly specifies a default value for the column, this
default overrides any defaults from inherited declarations of the column.So one may think it should be the same for partitioning as well.
"The same for partitioning" - I mean the same approach when in all the
situations (whether it's an insert into a parent table or a partition) a
partition default value will take precedence.
Hi.
On 2018/06/07 23:08, Dmitry Dolgov wrote:
On 7 June 2018 at 15:53, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2018/05/28 9:30, PG Bug reporting form wrote:The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:I found unexpected behavior when playing around with declarative
partitioning.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table.Hmm, so we provide the ability to specify default values per partition,
but it is not applied when inserting through the parent. I'd like to hear
from others on whether we should fix things so that we fill the
partition's default value for a given column if it's null in the input
tuple, after that tuple is routed to that partition. It does seem like a
inconvenience to have to do it through workarounds like a BR trigger.Actually, default value substitution happens much earlier in the query
rewrite phase, whereas the partition to actually insert the tuple into
(that is, tuple routing) is determined much later during the execution of
the query. So fixing this will require some work.Well, since documentation says that partitioning build on top of inheritance,
and for inheritance:If the new table explicitly specifies a default value for the column, this
default overrides any defaults from inherited declarations of the column.So one may think it should be the same for partitioning as well.
"The same for partitioning" - I mean the same approach when in all the
situations (whether it's an insert into a parent table or a partition) a
partition default value will take precedence.
I think you have a point. Before partitioning, one would either insert
directly into the child table or use a trigger to redirect an insert on
parent into one of the child tables. In both cases, child table's default
value would be used, because the insert query would mention the child
table name.
With partitioning, inserts into parent are internally handled in a way
that bypasses the processing which would otherwise fill a partition's own
default values for columns whose value is missing in the input row.
That said, I'd like to make sure before writing a patch if the feature of
being able to set defaults on partition level is something that users will
want in the long run.
Thanks,
Amit
On 06/13/2018 11:42 AM, Amit Langote wrote:
Hi.
On 2018/06/07 23:08, Dmitry Dolgov wrote:
On 7 June 2018 at 15:53, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2018/05/28 9:30, PG Bug reporting form wrote:The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:I found unexpected behavior when playing around with declarative
partitioning.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table.
...
Well, since documentation says that partitioning build on top of inheritance,
and for inheritance:If the new table explicitly specifies a default value for the column, this
default overrides any defaults from inherited declarations of the column.So one may think it should be the same for partitioning as well.
"The same for partitioning" - I mean the same approach when in all the
situations (whether it's an insert into a parent table or a partition) a
partition default value will take precedence.I think you have a point. Before partitioning, one would either insert
directly into the child table or use a trigger to redirect an insert on
parent into one of the child tables. In both cases, child table's default
value would be used, because the insert query would mention the child
table name.With partitioning, inserts into parent are internally handled in a way
that bypasses the processing which would otherwise fill a partition's own
default values for columns whose value is missing in the input row.That said, I'd like to make sure before writing a patch if the feature of
being able to set defaults on partition level is something that users will
want in the long run.Thanks,
Amit
I agree, and I imagine especially being able to use per-partition
sequences would be a common use case. That was my motivation when I
discovered it, and it was very counter intuitive to me.
Thanks for the NULL violation patch btw.
Best regards,
Jürgen
On Wed, Jun 13, 2018 at 03:39:04PM +0200, Jürgen Strobel wrote:
I agree, and I imagine especially being able to use per-partition
sequences would be a common use case. That was my motivation when I
discovered it, and it was very counter intuitive to me.Thanks for the NULL violation patch btw.
Please note that I have added this thread to the Open item page, under
the older bug queue, and that a CF entry has been added so as we don't
lost track of it:
https://commitfest.postgresql.org/18/1671/
--
Michael
On 2018/06/14 10:57, Michael Paquier wrote:
On Wed, Jun 13, 2018 at 03:39:04PM +0200, J�rgen Strobel wrote:
I agree, and I imagine especially being able to use per-partition
sequences would be a common use case. That was my motivation when I
discovered it, and it was very counter intuitive to me.Thanks for the NULL violation patch btw.
Please note that I have added this thread to the Open item page, under
the older bug queue, and that a CF entry has been added so as we don't
lost track of it:
Thank you, Michael.
Just to be sure, you meant to add the open item for the NOT NULL violation
bug fix patch mainly or all of what appears to be wrong here? About
partition-specific defaults being ignored when inserting through the
parent, do you think we should consider it a bug? I could imagine fixing
the documentation to say that partition-specific defaults are only honored
if directly inserted into and ignored otherwise.
Thanks,
Amit
I didn't see any hackers thread linked to this CF entry. Hence sending this mail through CF app.
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;
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.
On a side note, I see
coldef->constraints = restdef->constraints;
Shouldn't we merge the constraints instead of just overwriting those?
--
Best Wishesh
Ashutosh
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
Attachments:
PG10-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patchtext/plain; charset=UTF-8; name=PG10-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patchDownload+64-6
PG11-HEAD-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patchtext/plain; charset=UTF-8; name=PG11-HEAD-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patchDownload+64-6
On Thu, Jul 12, 2018 at 2:29 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
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.
+ /*
+ * Each member in 'saved_schema' contains a ColumnDef containing
+ * partition-specific options for the column. Below, we merge the
+ * information from each into the ColumnDef of the same name found in
+ * the original 'schema' list before deleting it from the list. So,
+ * once we've finished processing all the columns from the original
+ * 'schema' list, there shouldn't be any ColumnDefs left that came
+ * from the 'saved_schema' list.
+ */This is conveyed by the prologue of the function.
+ /*
+ * Combine using OR so that the NOT NULL constraint
+ * in the parent's definition doesn't get lost.
+ */
This too is specified in prologue as
* Constraints (including NOT NULL constraints) for the child table
* are the union of all relevant constraints, from both the child schema
* and parent tables.So, I don't think we need any special mention of OR, "union" conveys
the intention.
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.
In case of multiple inheritance coldef->constraints is "union" of
constraints from all the parents as described in the prologue. 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.
Also, I am not sure whether we really need all conditions related to
raw_default and cooked_default. Do you have any testcase showing the
need for those changes?
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Thanks Ashutosh, and sorry that I somehow missed replying to this.
On 2018/07/13 22:50, Ashutosh Bapat wrote:
On Thu, Jul 12, 2018 at 2:29 PM, Amit Langote wrote:
I have modified the comments around this code in the updated patch.
+ /* + * Each member in 'saved_schema' contains a ColumnDef containing + * partition-specific options for the column. Below, we merge the + * information from each into the ColumnDef of the same name found in + * the original 'schema' list before deleting it from the list. So, + * once we've finished processing all the columns from the original + * 'schema' list, there shouldn't be any ColumnDefs left that came + * from the 'saved_schema' list. + */This is conveyed by the prologue of the function.
+ /* + * Combine using OR so that the NOT NULL constraint + * in the parent's definition doesn't get lost. + */ This too is specified in prologue as * Constraints (including NOT NULL constraints) for the child table * are the union of all relevant constraints, from both the child schema * and parent tables.So, I don't think we need any special mention of OR, "union" conveys
the intention.
OK, I have removed both comments.
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.In case of multiple inheritance coldef->constraints is "union" of
constraints from all the parents as described in the prologue.
AFAICS, the prologue doesn't mention *just* coldef->constraints. It talks
about both the constraints that are to be specified using various
ColumnDef fields (is_not_null, raw_default, cooked_default, etc.) and
constraints that are to be returned in the *supconstr output list. Both
types of constraints are obtained by union'ing corresponding values from
all parents and child's own definition.
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.
*/
Also, I am not sure whether we really need all conditions related to
raw_default and cooked_default. Do you have any testcase showing the
need for those changes?
Without the patch (example below is tested on PG 10, but same is true with
PG 11 and HEAD too):
create table parent (a int, b int default -1) partition by list (a);
create table child partition of parent (b not null) for values in (0);
\d parent
Table "public.parent"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼───────────────
a │ integer │ │ │
b │ integer │ │ │ '-1'::integer
Partition key: LIST (a)
Number of partitions: 1 (Use \d+ to list them.)
\d child
Table "public.child"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ │
b │ integer │ │ not null │
Partition of: parent FOR VALUES IN (0)
Note that child didn't inherit -1 as default for b. That happens, because
the partition-specific ColumnDef for b, created to contain the NOT NULL
constraint, has its raw_default and cooked_default fields set to NULL.
The current code blindly assigns that ColumnDef's raw_default and
cooked_default to the inherited ColumnDef's corresponding fields.
Overriding raw_default like that is fine, because partition-specified
default get priority, but not cooked_default, which may contain the
inherited default.
That's not an issue if child's definition doesn't specify any of its own
constraints:
create table parent (a int, b int default -1) partition by list (a);
create table child partition of parent for values in (0);
\d child
Table "public.child"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼───────────────
a │ integer │ │ │
b │ integer │ │ │ '-1'::integer
Partition of: parent FOR VALUES IN (0)
That's because there is no partition-specific ColumnDef to override
anything in this case.
Anyway, I updated that code to look like this:
+ /*
+ * If the partition's definition specifies a default,
+ * it's in restdef->raw_default, which if non-NULL,
+ * overrides the parent's default that's in
+ * coldef->cooked_default.
+ */
+ if (restdef->raw_default)
+ {
+ coldef->raw_default = restdef->raw_default;
+ coldef->cooked_default = NULL;
+ }
+
+ /*
+ * coldef->cooked_default would contain the inherited
+ * default, unless overridden above. Don't try to
+ * override it with NULL.
+ */
+ Assert(restdef->cooked_default == NULL);Also, the patch already adds a test case that demonstrates what this code
does, but I modified it a bit in the updated version to also show the fix
for $subject. Now its expected output looks like this:
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null
default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of
parted_notnull_inh_test (a not null, b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | not null | 1
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;Just to clarify what's different in that \d output, here is the output
with unmodified PG 10:
\d parted_notnull_inh_test1
Table "public.parted_notnull_inh_test1"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ not null │
b │ integer │ │ │ 1
Partition of: parted_notnull_inh_test FOR VALUES IN (1)
As can be seen, PG 10 loses inherited values of a's default and b's NOT NULL.
Please find attached updated patches for PG 10, PG 11 / HEAD, with changes
I mentioned above.
Thanks,
Amit
Attachments:
PG10-v3-0001-Fix-bug-regarding-partition-column-option-inherit.patchtext/plain; charset=UTF-8; name=PG10-v3-0001-Fix-bug-regarding-partition-column-option-inherit.patchDownload+49-6
PG11-HEAD-v3-0001-Fix-bug-regarding-partition-column-option-inherit.patchtext/plain; charset=UTF-8; name=PG11-HEAD-v3-0001-Fix-bug-regarding-partition-column-option-inherit.patchDownload+49-6
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
Attachments:
PG10-v4-0001-Fix-bug-regarding-partition-column-option-inherit.patchtext/x-diff; charset=us-asciiDownload+68-59
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
Looking over the stuff in gram.y (to make sure there's nothing that
could be lost), I noticed that we're losing the COLLATE clause when they
are added to columns in partitions. I would expect part1 to end up with
collation es_CL, or alternatively that the command throws an error:
55462 10.6 138851=# create table part (a text collate "en_US") partition by range (a);
CREATE TABLE
Duración: 23,511 ms
55462 10.6 138851=# create table part1 partition of part (a collate "es_CL") for values from ('ca') to ('cu');
CREATE TABLE
Duración: 111,551 ms
55462 10.6 138851=# \d part
Tabla «public.part»
Columna │ Tipo │ Collation │ Nullable │ Default
─────────┼──────┼───────────┼──────────┼─────────
a │ text │ en_US │ │
Partition key: RANGE (a)
Number of partitions: 1 (Use \d+ to list them.)
55462 10.6 138851=# \d part1
Tabla «public.part1»
Columna │ Tipo │ Collation │ Nullable │ Default
─────────┼──────┼───────────┼──────────┼─────────
a │ text │ en_US │ │
Partition of: part FOR VALUES FROM ('ca') TO ('cu')
(This case is particularly bothersome because the column is the
partition key, so the partition range bounds would differ depending on
which collation is used to compare. I assume we'd always want to use
the parent table's collation; so there's even a stronger case for
raising an error if it doesn't match the parent's. However, this case
could arise for other columns too, where it's not *so* bad, but still
not completely correct I think.)
This happens on unpatched code, and doesn't seem covered by any tests.
However, this seems an independent bug that isn't affected by this
patch.
The only other things there are deferrability markers, which seem to be
propagated in a relatively sane fashion (but no luck if you want to add
foreign keys with mismatching deferrability than the parent's -- you
just get a dupe, and there's no way in the grammar to change the flags
for the existing constraint). But you can add a UNIQUE DEFERRED
constraint in a partition that wasn't there in the parent, for example.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Here's the patch I intend to push to branches 10 and 11. The patch in
master is almost identical -- the only difference is that
ColumnDef->is_from_parent is removed.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Revise-attribute-handling-code-on-partition-creation.patchtext/x-diff; charset=iso-8859-1Download+71-64
Hi,
On 2018/11/07 0:10, Alvaro Herrera wrote:
Looking over the stuff in gram.y (to make sure there's nothing that
could be lost), I noticed that we're losing the COLLATE clause when they
are added to columns in partitions. I would expect part1 to end up with
collation es_CL, or alternatively that the command throws an error:55462 10.6 138851=# create table part (a text collate "en_US") partition by range (a);
CREATE TABLE
Duración: 23,511 ms
55462 10.6 138851=# create table part1 partition of part (a collate "es_CL") for values from ('ca') to ('cu');
CREATE TABLE
Duración: 111,551 ms
55462 10.6 138851=# \d part
Tabla «public.part»
Columna │ Tipo │ Collation │ Nullable │ Default
─────────┼──────┼───────────┼──────────┼─────────
a │ text │ en_US │ │
Partition key: RANGE (a)
Number of partitions: 1 (Use \d+ to list them.)55462 10.6 138851=# \d part1
Tabla «public.part1»
Columna │ Tipo │ Collation │ Nullable │ Default
─────────┼──────┼───────────┼──────────┼─────────
a │ text │ en_US │ │
Partition of: part FOR VALUES FROM ('ca') TO ('cu')(This case is particularly bothersome because the column is the
partition key, so the partition range bounds would differ depending on
which collation is used to compare. I assume we'd always want to use
the parent table's collation; so there's even a stronger case for
raising an error if it doesn't match the parent's. However, this case
could arise for other columns too, where it's not *so* bad, but still
not completely correct I think.)
Thank you for investigating.
I think the result in this case should be an error, just as it would in
the regular inheritance case.
create table parent (a text);
create table child (a text collate "en_US") inherits (parent);
NOTICE: merging column "a" with inherited definition
ERROR: column "a" has a collation conflict
DETAIL: "default" versus "en_US"
In fact, I see that ATTACH PARTITION rejects a partition if collations
don't match.
create table part (a text collate "en_US") partition by range (a);
create table part1 (a text collate "es_CL");
alter table part attach partition part1 for values from ('ca') to ('cu');
ERROR: child table "part1" has different collation for column "a"
This happens on unpatched code, and doesn't seem covered by any tests.
However, this seems an independent bug that isn't affected by this
patch.The only other things there are deferrability markers, which seem to be
propagated in a relatively sane fashion (but no luck if you want to add
foreign keys with mismatching deferrability than the parent's -- you
just get a dupe, and there's no way in the grammar to change the flags
for the existing constraint). But you can add a UNIQUE DEFERRED
constraint in a partition that wasn't there in the parent, for example.
Looking again at MergeAttributes, it seems that the fix for disallowing
mismatching collations is not that invasive. PFA a patch that applies on
top of your 0001-Revise-attribute-handling-code-on-partition-creation.patch.
I haven't looked closely at the cases involving deferrability markers though.
Thanks,
Amit
Attachments:
0002-Disallow-creating-partitions-with-mismatching-collat.patchtext/plain; charset=UTF-8; name=0002-Disallow-creating-partitions-with-mismatching-collat.patchDownload+32-1
On 2018-Nov-07, Amit Langote wrote:
I think the result in this case should be an error, just as it would in
the regular inheritance case.create table parent (a text);
create table child (a text collate "en_US") inherits (parent);
NOTICE: merging column "a" with inherited definition
ERROR: column "a" has a collation conflict
DETAIL: "default" versus "en_US"In fact, I see that ATTACH PARTITION rejects a partition if collations
don't match.
Hmm, I'm thinking perhaps we shouldn't backpatch this part. It's
obviously a bug, but we might break somebody's working apps. Therefore
I think I'd rather leave it out of the current bugfix and commit
separately.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Nov 9, 2018 at 1:03 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Nov-07, Amit Langote wrote:
I think the result in this case should be an error, just as it would in
the regular inheritance case.create table parent (a text);
create table child (a text collate "en_US") inherits (parent);
NOTICE: merging column "a" with inherited definition
ERROR: column "a" has a collation conflict
DETAIL: "default" versus "en_US"In fact, I see that ATTACH PARTITION rejects a partition if collations
don't match.Hmm, I'm thinking perhaps we shouldn't backpatch this part. It's
obviously a bug, but we might break somebody's working apps. Therefore
I think I'd rather leave it out of the current bugfix and commit
separately.
Okay, that may be fine because nothing wrong is happening by silently
ignoring the partition's specified collation.
Thanks,
Amit
On 2018-Nov-09, Amit Langote wrote:
On Fri, Nov 9, 2018 at 1:03 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Nov-07, Amit Langote wrote:
Hmm, I'm thinking perhaps we shouldn't backpatch this part. It's
obviously a bug, but we might break somebody's working apps. Therefore
I think I'd rather leave it out of the current bugfix and commit
separately.Okay, that may be fine because nothing wrong is happening by silently
ignoring the partition's specified collation.
Actually, how about we reduce the message to a WARNING in released
branches, and ERROR in master? Broken applications would continue to
work, but users might become aware that there might be a problem.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
