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(at)strobel(dot)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(at)strobel(dot)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

> On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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(at)strobel(dot)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(at)gmail(dot)com> wrote:
>> On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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(at)strobel(dot)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(at)gmail(dot)com> wrote:
>>> On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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(at)strobel(dot)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(at)gmail(dot)com> wrote:
>>>> On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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(at)strobel(dot)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:
>
> https://commitfest.postgresql.org/18/1671/

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

On Thu, Jul 12, 2018 at 2:29 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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

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

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

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

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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

Pushed.

I included the test case for collations to the three branches, but no
code changes. We can patch master for the handling of collations per
your patch, and if somebody has it, a change to how defaults are applied
when routing tuples.

Thanks to Jürgen for reporting the bug.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018/11/09 1:38, Alvaro Herrera wrote:
> On 2018-Nov-09, Amit Langote wrote:
>
>> On Fri, Nov 9, 2018 at 1:03 AM Alvaro Herrera <alvherre(at)2ndquadrant(dot)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.

That might work too. Would you like me to create and post patches like
that for 10 and 11 branches?

Thanks,
Amit

On 2018/11/09 4:39, Alvaro Herrera wrote:
> Pushed.

Thank you for committing. I've closed the CF entry.

> I included the test case for collations to the three branches, but no
> code changes. We can patch master for the handling of collations per
> your patch,

Okay, but should we back-patch it by adding WARNING to back-branches as
you suggest?

> and if somebody has it, a change to how defaults are applied
> when routing tuples.

I haven't written such a patch yet. Do we want such a feature?

Thanks,
Amit

On 2018/11/09 14:04, Amit Langote wrote:
> On 2018/11/09 4:39, Alvaro Herrera wrote:
>> and if somebody has it, a change to how defaults are applied
>> when routing tuples.
>
> I haven't written such a patch yet. Do we want such a feature?

Or is it a *bug* of tuple-routing that it doesn't substitute default
values that may be defined for partitions? It kind of looks like one if
you see an example like this.

create table p (a int, b int) partition by list (a);
create table p1 partition of p (b not null default 1) for values in (1);
insert into p1 values (1);
table p;
a │ b
───┼───
1 │ 1
(1 row)

insert into p values (1);
ERROR: null value in column "b" violates not-null constraint
DETAIL: Failing row contains (1, null).

Thanks,
Amit

On 2018-Nov-09, Amit Langote wrote:

> Or is it a *bug* of tuple-routing that it doesn't substitute default
> values that may be defined for partitions? It kind of looks like one if
> you see an example like this.
>
> create table p (a int, b int) partition by list (a);
> create table p1 partition of p (b not null default 1) for values in (1);
> insert into p1 values (1);
> table p;
> a │ b
> ───┼───
> 1 │ 1
> (1 row)
>
> insert into p values (1);
> ERROR: null value in column "b" violates not-null constraint
> DETAIL: Failing row contains (1, null).

I don't know. I wonder if the bug isn't that we allow the default to be
specified for the partition at all -- why shouldn't we just reject that
with an error?

See this example, where the inserts give values that could be said to be
inconsistent:

create table p (a int , b int default 3) partition by list (a);
create table p1 partition of p (b default 42) for values in (1);
insert into p (a) values (1);
insert into p1 (a) values (1);
select * from p;

a │ b
───┼────
1 │ 3
1 │ 42
(2 filas)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-11-09 16:59, Alvaro Herrera wrote:
> On 2018-Nov-09, Amit Langote wrote:
>
>> Or is it a *bug* of tuple-routing that it doesn't substitute default
>> values that may be defined for partitions? It kind of looks like one if
>> you see an example like this.
>>
>> create table p (a int, b int) partition by list (a);
>> create table p1 partition of p (b not null default 1) for values in (1);
>> insert into p1 values (1);
>> table p;
>> a │ b
>> ───┼───
>> 1 │ 1
>> (1 row)
>>
>> insert into p values (1);
>> ERROR: null value in column "b" violates not-null constraint
>> DETAIL: Failing row contains (1, null).
>
> I don't know. I wonder if the bug isn't that we allow the default to be
> specified for the partition at all -- why shouldn't we just reject that
> with an error?
>
> See this example, where the inserts give values that could be said to be
> inconsistent:
>
> create table p (a int , b int default 3) partition by list (a);
> create table p1 partition of p (b default 42) for values in (1);
> insert into p (a) values (1);
> insert into p1 (a) values (1);
> select * from p;
>
> a │ b
> ───┼────
> 1 │ 3
> 1 │ 42
> (2 filas)

I found this problem while attempting to create sub-ids with partition
defaults using distinct per-partition sequences.

I can think of other useful scenarios too, but if you don't want to
support it because of unexpected complexities all my cases can still be
implemented using custom triggers albeit slower and more inconvenient.

Regarding your example, what I expected is that *both* inserts would
consistently result in a tuple of (1, 42) since p should route the
insert to p1 and use p1's defaults. The current inconsistent behavior is
the heart of the bug.

Best regards,
Jürgen

On 2018-Nov-09, Jürgen Strobel wrote:

> Regarding your example, what I expected is that *both* inserts would
> consistently result in a tuple of (1, 42) since p should route the
> insert to p1 and use p1's defaults. The current inconsistent behavior is
> the heart of the bug.

I think that would be sensible behavior, as long as the partition
doesn't override values for the partitioning column -- i.e. if the
default values don't re-route the tuple to another partition, I think we
should use the partition's default rather than the parent. This says we
should expand defaults after routing. However, can we really route if
we haven't expanded defaults? In general, we may be lacking values for
some columns of the partition key. Another point: if we've already
expanded defaults when processing at the parent level, when we reach the
partition we don't know which values are still missing default
expansion, or which defaults coming from the parent ought to be
re-expanded.

So this looks to be a bit of a landmine.

In any case it seems really hard to see this is as a bug that we would
fix in back-branches.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-11-09 18:07, Alvaro Herrera wrote:
> On 2018-Nov-09, Jürgen Strobel wrote:
>
>> Regarding your example, what I expected is that *both* inserts would
>> consistently result in a tuple of (1, 42) since p should route the
>> insert to p1 and use p1's defaults. The current inconsistent behavior is
>> the heart of the bug.
>
> I think that would be sensible behavior, as long as the partition
> doesn't override values for the partitioning column -- i.e. if the
> default values don't re-route the tuple to another partition, I think we
> should use the partition's default rather than the parent. This says we
> should expand defaults after routing. However, can we really route if
> we haven't expanded defaults? In general, we may be lacking values for
> some columns of the partition key. Another point: if we've already
> expanded defaults when processing at the parent level, when we reach the
> partition we don't know which values are still missing default
> expansion, or which defaults coming from the parent ought to be
> re-expanded.
>
> So this looks to be a bit of a landmine.
>
> In any case it seems really hard to see this is as a bug that we would
> fix in back-branches.
>

I am slightly confused now, I thought this landmine was already fixed in
master and what remains is only whether to backport it or not? Which I
guess depends on whether this is classified as a bug or not.

Since the fix has the potential to break current applications and the
documentation is vague about wanted behavior I think it would be
sensible to add a warning only, even if we agree to call it a bug.

Best regards,
Jürgen

On 2018-Nov-09, Jürgen Strobel wrote:

> I am slightly confused now, I thought this landmine was already fixed in
> master and what remains is only whether to backport it or not? Which I
> guess depends on whether this is classified as a bug or not.

Hmm, I changed (and back-patched) what happens to the NOT NULL clauses,
but as I understand the behavior of the DEFAULT clauses has not changed.

> Since the fix has the potential to break current applications and the
> documentation is vague about wanted behavior I think it would be
> sensible to add a warning only, even if we agree to call it a bug.

Hmm, the part I was first proposing to backpatch as an ERROR was a
mismatch in the COLLATE clause; then I talked about using a WARNING. I
ended up backpatching neither -- only immortalized the current behavior
in a test case. I think it should be an ERROR, but in master only.

The DEFAULT clauses are a different problem (landmine) :-)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-11-09 22:11, Alvaro Herrera wrote:
> On 2018-Nov-09, Jürgen Strobel wrote:
>
>> I am slightly confused now, I thought this landmine was already fixed in
>> master and what remains is only whether to backport it or not? Which I
>> guess depends on whether this is classified as a bug or not.
>
> Hmm, I changed (and back-patched) what happens to the NOT NULL clauses,
> but as I understand the behavior of the DEFAULT clauses has not changed.
>
>> Since the fix has the potential to break current applications and the
>> documentation is vague about wanted behavior I think it would be
>> sensible to add a warning only, even if we agree to call it a bug.
>
> Hmm, the part I was first proposing to backpatch as an ERROR was a
> mismatch in the COLLATE clause; then I talked about using a WARNING. I
> ended up backpatching neither -- only immortalized the current behavior
> in a test case. I think it should be an ERROR, but in master only.
>
>
> The DEFAULT clauses are a different problem (landmine) :-)
>

OK got it. I agree about differing COLLATE clauses making no sense and
with the ERROR+WARNING solution, but I didn't report that.

The NULL violation is obvious too.

I still hope for a bug fix for the DEFAULT clause with sensible
restrictions.

-Jürgen

On 2018-Nov-09, Jürgen Strobel wrote:

> OK got it. I agree about differing COLLATE clauses making no sense and
> with the ERROR+WARNING solution, but I didn't report that.

Yeah, I happened to notice it on code inspection.

> The NULL violation is obvious too.

Yeah, IMO that was an obvious bugfix.

> I still hope for a bug fix for the DEFAULT clause with sensible
> restrictions.

Yeah, I understand that this is the one that you really care about.
However, I think a fix is unlikely to be back-patchable, because it may
break existing applications that are written to expect the current
behavior.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> On 2018-Nov-09, Jürgen Strobel wrote:
>> Regarding your example, what I expected is that *both* inserts would
>> consistently result in a tuple of (1, 42) since p should route the
>> insert to p1 and use p1's defaults. The current inconsistent behavior is
>> the heart of the bug.

> I think that would be sensible behavior, as long as the partition
> doesn't override values for the partitioning column -- i.e. if the
> default values don't re-route the tuple to another partition, I think we
> should use the partition's default rather than the parent. This says we
> should expand defaults after routing.

I'd argue not, actually. I think there is plausible precedent in
updatable views, where what we use is the defaults associated with the
view, not the underlying table. Correspondingly, what ought to govern
in a partitioned insert is the defaults associated with the table actually
named in the insert command, never mind what its partitions might say.
That is useful for many situations, and it avoids all the logical
inconsistencies you get into if you find that the defaults associated
with some partition would force re-routing elsewhere.

In any case, you can't make this happen without basically blowing up
default processing altogether. Defaults are currently expanded by the
planner, and pretty early too. To make it work like the OP wishes,
we'd have to insert them sometime late in the executor.

> In any case it seems really hard to see this is as a bug that we would
> fix in back-branches.

Backwards compatibility considerations would prevent that in any case.

regards, tom lane

On 2018/11/10 7:33, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> On 2018-Nov-09, Jürgen Strobel wrote:
>>> Regarding your example, what I expected is that *both* inserts would
>>> consistently result in a tuple of (1, 42) since p should route the
>>> insert to p1 and use p1's defaults. The current inconsistent behavior is
>>> the heart of the bug.
>
>> I think that would be sensible behavior, as long as the partition
>> doesn't override values for the partitioning column -- i.e. if the
>> default values don't re-route the tuple to another partition, I think we
>> should use the partition's default rather than the parent. This says we
>> should expand defaults after routing.
>
> I'd argue not, actually. I think there is plausible precedent in
> updatable views, where what we use is the defaults associated with the
> view, not the underlying table. Correspondingly, what ought to govern
> in a partitioned insert is the defaults associated with the table actually
> named in the insert command, never mind what its partitions might say.
> That is useful for many situations, and it avoids all the logical
> inconsistencies you get into if you find that the defaults associated
> with some partition would force re-routing elsewhere.
>
> In any case, you can't make this happen without basically blowing up
> default processing altogether. Defaults are currently expanded by the
> planner, and pretty early too. To make it work like the OP wishes,
> we'd have to insert them sometime late in the executor.

Considering multi-level partitioning, that'd mean the tuple being routed
would need to be filled with the defaults of every table on the way to a
leaf partition, including that of the leaf partition where the tuple
finally ends up. If we re-route upon the final leaf partition's defaults
leading to partition constraint violation, it's possible that someone
might end up setting up an infinite re-routing loop with that behavior.

create table p (a int) partition by list (a);
create table p1 partition of p (a default 2) for values in (1);
create table p2 partition of p (a default 1) for values in (2);

It won't be fun for the server to try to prevent that kind of thing.

IOW, it might be a good idea to call the ability to set partition-level
defaults a deprecated feature?

Thanks,
Amit

Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> writes:
> On 2018/11/10 7:33, Tom Lane wrote:
>> I'd argue not, actually. I think there is plausible precedent in
>> updatable views, where what we use is the defaults associated with the
>> view, not the underlying table. Correspondingly, what ought to govern
>> in a partitioned insert is the defaults associated with the table actually
>> named in the insert command, never mind what its partitions might say.
>> That is useful for many situations, and it avoids all the logical
>> inconsistencies you get into if you find that the defaults associated
>> with some partition would force re-routing elsewhere.

> ...
> IOW, it might be a good idea to call the ability to set partition-level
> defaults a deprecated feature?

Not necessarily. They'd apply when you insert directly into a particular
partition by name.

regards, tom lane

On 2018/11/12 12:59, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> writes:
>> On 2018/11/10 7:33, Tom Lane wrote:
>>> I'd argue not, actually. I think there is plausible precedent in
>>> updatable views, where what we use is the defaults associated with the
>>> view, not the underlying table. Correspondingly, what ought to govern
>>> in a partitioned insert is the defaults associated with the table actually
>>> named in the insert command, never mind what its partitions might say.
>>> That is useful for many situations, and it avoids all the logical
>>> inconsistencies you get into if you find that the defaults associated
>>> with some partition would force re-routing elsewhere.
>
>> ...
>> IOW, it might be a good idea to call the ability to set partition-level
>> defaults a deprecated feature?
>
> Not necessarily. They'd apply when you insert directly into a particular
> partition by name.

Yes. Maybe, we should document that the partition default are not honored
when inserting through the parent.

Thanks,
Amit

On 2018-11-09 23:33, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> On 2018-Nov-09, Jürgen Strobel wrote:
>>> Regarding your example, what I expected is that *both* inserts would
>>> consistently result in a tuple of (1, 42) since p should route the
>>> insert to p1 and use p1's defaults. The current inconsistent behavior is
>>> the heart of the bug.
>
>> I think that would be sensible behavior, as long as the partition
>> doesn't override values for the partitioning column -- i.e. if the
>> default values don't re-route the tuple to another partition, I think we
>> should use the partition's default rather than the parent. This says we
>> should expand defaults after routing.
>
> I'd argue not, actually. I think there is plausible precedent in
> updatable views, where what we use is the defaults associated with the
> view, not the underlying table. Correspondingly, what ought to govern
> in a partitioned insert is the defaults associated with the table actually
> named in the insert command, never mind what its partitions might say.
> That is useful for many situations, and it avoids all the logical
> inconsistencies you get into if you find that the defaults associated
> with some partition would force re-routing elsewhere.
>
> In any case, you can't make this happen without basically blowing up
> default processing altogether. Defaults are currently expanded by the
> planner, and pretty early too. To make it work like the OP wishes,
> we'd have to insert them sometime late in the executor.
>
>> In any case it seems really hard to see this is as a bug that we would
>> fix in back-branches.
>
> Backwards compatibility considerations would prevent that in any case.
I don't really think the view comparison holds water. Views are layered
above tables without affecting them by design, but partitions are
intrinsic parts of sharded tables and only make sense as a whole.

With all due respect I suspect your view of implementation issues biases
your judgement here. For me the partition's default handling was
completely unexpected, especially with the documentation silent about
default-applying behavior, and inconsistent with how check constraints
work with partitions.

Maybe there's another way to implement this. Like mark early and apply
defaults later, and handle meta data like constraints. Sorry if this
sounds stupid.

Btw I fully agree about not back-patching changes to this, but a
documentation update would be nice.

Best regards,
Jürgen

On 2018-Nov-12, Jürgen Strobel wrote:

> With all due respect I suspect your view of implementation issues biases
> your judgement here. For me the partition's default handling was
> completely unexpected, especially with the documentation silent about
> default-applying behavior, and inconsistent with how check constraints
> work with partitions.

This is valuable input. It would be good to have more opinions from
users, particularly those accustomed to how things work in other RDBMS
systems, so that we can inform our own opinions on how we should make it
work here in PG going forward.

One of the guiding principles that I think we should hold for
partitioning is that operating directly into the partition should be
seen as only an optimization compared to inserting into the parent table
-- thus it should not behave differently. Applying different default
values depending on where you're inserting into goes counter to that
principle.

> Maybe there's another way to implement this. Like mark early and apply
> defaults later, and handle meta data like constraints. Sorry if this
> sounds stupid.

It does not sound stupid -- it seems exactly what I was thinking.
Admittedly, it's a bit hand-wavy and we'd need to understand more how it
work in more detail, as well as how it works currently (and why it
does).

However, I'm not ATM in a position to design or write a patch for this.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> One of the guiding principles that I think we should hold for
> partitioning is that operating directly into the partition should be
> seen as only an optimization compared to inserting into the parent table
> -- thus it should not behave differently. Applying different default
> values depending on where you're inserting into goes counter to that
> principle.

I'm not entirely convinced that I buy that argument, especially not in
a case like this where it introduces logical inconsistencies where there
otherwise wouldn't be any.

regards, tom lane

On 2018-11-12 17:33, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> One of the guiding principles that I think we should hold for
>> partitioning is that operating directly into the partition should be
>> seen as only an optimization compared to inserting into the parent table
>> -- thus it should not behave differently. Applying different default
>> values depending on where you're inserting into goes counter to that
>> principle.
>
> I'm not entirely convinced that I buy that argument, especially not in
> a case like this where it introduces logical inconsistencies where there
> otherwise wouldn't be any.
>

I think I missed something, what are the *introduced* logical problems?
Apart from implementation issues the only logical problems I see are if
you allow to change defaults of the partition key columns, and these are
problematic (nonsensical really) in either case.

Regards,
Jürgen

=?UTF-8?Q?J=c3=bcrgen_Strobel?= <juergen+postgresql(at)strobel(dot)info> writes:
> On 2018-11-12 17:33, Tom Lane wrote:
>> I'm not entirely convinced that I buy that argument, especially not in
>> a case like this where it introduces logical inconsistencies where there
>> otherwise wouldn't be any.

> I think I missed something, what are the *introduced* logical problems?

What to do if a partition introduces a default value that would force
re-routing to some other partition.

> Apart from implementation issues the only logical problems I see are if
> you allow to change defaults of the partition key columns, and these are
> problematic (nonsensical really) in either case.

Just claiming that it's nonsensical doesn't fix the problem. Also, it
is neither problematic nor nonsensical for the root to provide defaults
for partition key columns. So if we go this route, we are giving up
useful behavior (key-column defaults on the root) for useless behavior
(key-column defaults on the partitions).

regards, tom lane

On 2018-11-13 00:57, Tom Lane wrote:
> =?UTF-8?Q?J=c3=bcrgen_Strobel?= <juergen+postgresql(at)strobel(dot)info> writes:
>> On 2018-11-12 17:33, Tom Lane wrote:
>>> I'm not entirely convinced that I buy that argument, especially not in
>>> a case like this where it introduces logical inconsistencies where there
>>> otherwise wouldn't be any.
>
>> I think I missed something, what are the *introduced* logical problems?
>
> What to do if a partition introduces a default value that would force
> re-routing to some other partition.

I would claim this is a problem which already exists with the current
behavior:

a) If a tuple is inserted through the parent it's ignored and useless.
b) If a tuple is inserted through the partition it can lead to
inconsistency and a constraint violation (possibly rectifiable by a
re-route which I think we all agree is out of the question).

>> Apart from implementation issues the only logical problems I see are if
>> you allow to change defaults of the partition key columns, and these are
>> problematic (nonsensical really) in either case.
>
> Just claiming that it's nonsensical doesn't fix the problem. Also, it
> is neither problematic nor nonsensical for the root to provide defaults
> for partition key columns. So if we go this route, we are giving up
> useful behavior (key-column defaults on the root) for useless behavior
> (key-column defaults on the partitions).

Yes, I strongly believe defaults on the key columns in partitions should
be disallowed, that's why I called them nonsensical in both cases, see
above a) and b) for why this applies even to current behavior.

Of course disallowing all DEFAULT clauses on partitions is the easiest
way out, and I'd rather see this than inserts through partitions being
more than a pure optimization.

Best regards,
Jürgen

On Tue, 2018-11-13 at 13:44 +0100, Jürgen Strobel wrote:
> On 2018-11-13 00:57, Tom Lane wrote:
> > =?UTF-8?Q?J=c3=bcrgen_Strobel?= <juergen+postgresql(at)strobel(dot)info>
> > writes:
> > > On 2018-11-12 17:33, Tom Lane wrote:
> > > > I'm not entirely convinced that I buy that argument, especially
> > > > not in
> > > > a case like this where it introduces logical inconsistencies
> > > > where there
> > > > otherwise wouldn't be any.
> > > I think I missed something, what are the *introduced* logical
> > > problems?
> >
> > What to do if a partition introduces a default value that would
> > force
> > re-routing to some other partition.
>
> I would claim this is a problem which already exists with the current
> behavior:
>

I think the question is what part of the current behavior is
intentional (albeit with corner-cases not handled or rejected
properly), and what part is just not thought through. Not sure.

> a) If a tuple is inserted through the parent it's ignored and
> useless.
> b) If a tuple is inserted through the partition it can lead to
> inconsistency and a constraint violation (possibly rectifiable by a
> re-route which I think we all agree is out of the question).
>

Yeah, allowing defaults mismatching the partition seems like a can of
worms. We certainly should not allow such rows to be inserted in the
partition. Routing them somewhere else seems tricky, so perhaps it'd be
better to just error-out in this case.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, 2018-11-12 at 11:33 -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > One of the guiding principles that I think we should hold for
> > partitioning is that operating directly into the partition should
> > be
> > seen as only an optimization compared to inserting into the parent
> > table
> > -- thus it should not behave differently. Applying different
> > default
> > values depending on where you're inserting into goes counter to
> > that
> > principle.
>
> I'm not entirely convinced that I buy that argument, especially not
> in a case like this where it introduces logical inconsistencies where
> there otherwise wouldn't be any.
>

Not sure what new logical inconsistencies you have in mind, but the
principle proposed by Alvaro seems sensible to me. I too see insertions
into the partition primarily as an optimization, and would not expect
it to use other defaults than when inserting through the parent.

Particularly not defaults contradicting the partition definition and
forcing rerouting the tuple somewhere else automatically. (IME when
inserting directly into partitions people expect that data to end in
that partition, and if not it's an error - bogus data, ...).

I'm not claiming there are no use cases for defaults on partitions. For
example I can imagine multi-level partitioning, where each layer can
specify defaults for the lower levels. But it seems rather surprising
to only apply the partition defaults for direct insertions.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018/11/09 14:04, Amit Langote wrote:
> On 2018/11/09 4:39, Alvaro Herrera wrote:
>> I included the test case for collations to the three branches, but no
>> code changes. We can patch master for the handling of collations per
>> your patch,
>
> Okay, but should we back-patch it by adding WARNING to back-branches as
> you suggest?

I was looking at the pending patches that I'd sent and noticed this one to
throw an error when a partition specifies a collation for a column that
doesn't match the parent's. Do we want to apply the attached rebased
patch to HEAD and leave the back-branches (10 and 11) alone?

Thanks,
Amit