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

classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
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
Reply | Threaded
Open this post in threaded view
|

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

Amit Langote-2
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

PG10-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patch (7K) Download Attachment
PG11-HEAD-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Thu, Jul 12, 2018 at 2:29 PM, Amit Langote
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Amit Langote-2
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

PG10-v3-0001-Fix-bug-regarding-partition-column-option-inherit.patch (6K) Download Attachment
PG11-HEAD-v3-0001-Fix-bug-regarding-partition-column-option-inherit.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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

PG10-v4-0001-Fix-bug-regarding-partition-column-option-inherit.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Amit Langote-2
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


Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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

Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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

0001-Revise-attribute-handling-code-on-partition-creation.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Amit Langote-2
In reply to this post by Alvaro Herrera-9
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

0002-Disallow-creating-partitions-with-mismatching-collat.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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

Reply | Threaded
Open this post in threaded view
|

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

Amit Langote
On Fri, Nov 9, 2018 at 1:03 AM Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
On 2018-Nov-09, Amit Langote wrote:

> On Fri, Nov 9, 2018 at 1:03 AM Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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

Reply | Threaded
Open this post in threaded view
|

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

Amit Langote-2
In reply to this post by Alvaro Herrera-9
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 <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Amit Langote-2
In reply to this post by Alvaro Herrera-9
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



Reply | Threaded
Open this post in threaded view
|

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

Amit Langote-2
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


Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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

Reply | Threaded
Open this post in threaded view
|

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

juergen+postgresql
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

Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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

Reply | Threaded
Open this post in threaded view
|

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

juergen+postgresql
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

12