Re: [BUG] Partition creation fails after dropping a column and adding a partial index
On Fri, Nov 01, 2019 at 09:58:26AM +0900, Amit Langote wrote: > On Thu, Oct 31, 2019 at 1:45 PM Michael Paquier wrote: >> The patch is rather simple as per the attached, with extended >> regression tests included. I have not checked on back-branches yet, >> but that's visibly wrong since 8b08f7d down to v11 (will do that when >> back-patching). > > The patch looks correct and applies to both v12 and v11. Thanks for the review, committed down to v11. The version for v11 had a couple of conflicts actually. >> There could be a point in changing convert_tuples_by_name_map & co so >> as they return the length of the map on top of the map to avoid such >> mistakes in the future. That's a more invasive patch not really >> adapted for a back-patch, but we could do that on HEAD once this bug >> is fixed. I have also checked other calls of this API and the >> handling is done correctly. > > I've been bitten by this logical error when deciding what length to > use for the map, so seems like a good idea. Okay, let's see about that then. -- Michael signature.asc Description: PGP signature
Re: [BUG] Partition creation fails after dropping a column and adding a partial index
On Thu, Oct 31, 2019 at 1:45 PM Michael Paquier wrote: > On Tue, Oct 29, 2019 at 01:16:58PM +0900, Michael Paquier wrote: > > Yes, something looks wrong with that. I have not looked at it in > > details yet though. I'll see about that tomorrow. > > So.. When building the attribute map for a cloned index (with either > LIKE during the transformation or for partition indexes), we store > each attribute number with 0 used for dropped columns. Unfortunately, > if you look at the way the attribute map is built in this case the > code correctly generates the mapping with convert_tuples_by_name_map. > But, the length of the mapping used is incorrect as this makes use of > the number of attributes for the newly-created child relation, and not > the parent which includes the dropped column in its count. So the > answer is simply to use the parent as reference for the mapping > length. > > The patch is rather simple as per the attached, with extended > regression tests included. I have not checked on back-branches yet, > but that's visibly wrong since 8b08f7d down to v11 (will do that when > back-patching). The patch looks correct and applies to both v12 and v11. > There could be a point in changing convert_tuples_by_name_map & co so > as they return the length of the map on top of the map to avoid such > mistakes in the future. That's a more invasive patch not really > adapted for a back-patch, but we could do that on HEAD once this bug > is fixed. I have also checked other calls of this API and the > handling is done correctly. I've been bitten by this logical error when deciding what length to use for the map, so seems like a good idea. Thanks, Amit
Re: [BUG] Partition creation fails after dropping a column and adding a partial index
On Thu, Oct 31, 2019 at 9:45 AM Michael Paquier wrote:
> On Tue, Oct 29, 2019 at 01:16:58PM +0900, Michael Paquier wrote:
> > Yes, something looks wrong with that. I have not looked at it in
> > details yet though. I'll see about that tomorrow.
>
> So.. When building the attribute map for a cloned index (with either
> LIKE during the transformation or for partition indexes), we store
> each attribute number with 0 used for dropped columns. Unfortunately,
> if you look at the way the attribute map is built in this case the
> code correctly generates the mapping with convert_tuples_by_name_map.
> But, the length of the mapping used is incorrect as this makes use of
> the number of attributes for the newly-created child relation, and not
> the parent which includes the dropped column in its count. So the
> answer is simply to use the parent as reference for the mapping
> length.
>
> The patch is rather simple as per the attached, with extended
> regression tests included. I have not checked on back-branches yet,
> but that's visibly wrong since 8b08f7d down to v11 (will do that when
> back-patching).
>
> There could be a point in changing convert_tuples_by_name_map & co so
> as they return the length of the map on top of the map to avoid such
> mistakes in the future. That's a more invasive patch not really
> adapted for a back-patch, but we could do that on HEAD once this bug
> is fixed. I have also checked other calls of this API and the
> handling is done correctly.
>
> The patch works for me on master and on 12. I have rebased the patch for
Version 11.
> Wyatt, what do you think?
> --
> Michael
>
--
Ibrar Ahmed
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b53f6ed3ac..9a1ef78a1f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -961,7 +961,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
gettext_noop("could not convert row type"));
idxstmt =
generateClonedIndexStmt(NULL, RelationGetRelid(rel), idxRel,
- attmap, RelationGetDescr(rel)->natts,
+ attmap, RelationGetDescr(parent)->natts,
);
DefineIndex(RelationGetRelid(rel),
idxstmt,
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index c25117e47c..d20ba3dbdf 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -1006,3 +1006,52 @@ CONTEXT: SQL statement "create table tab_part_create_1 partition of tab_part_cr
PL/pgSQL function func_part_create() line 3 at EXECUTE
drop table tab_part_create;
drop function func_part_create();
+-- tests of column drop with partition tables and indexes using
+-- predicates and expressions.
+create table part_column_drop (
+ useless_1 int,
+ id int,
+ useless_2 int,
+ d int,
+ b int,
+ useless_3 int
+) partition by range (id);
+alter table part_column_drop drop column useless_1;
+alter table part_column_drop drop column useless_2;
+alter table part_column_drop drop column useless_3;
+create index part_column_drop_b_pred on part_column_drop(b) where b = 1;
+create index part_column_drop_b_expr on part_column_drop((b = 1));
+create index part_column_drop_d_pred on part_column_drop(d) where d = 2;
+create index part_column_drop_d_expr on part_column_drop((d = 2));
+create table part_column_drop_1_10 partition of
+ part_column_drop for values from (1) to (10);
+\d part_column_drop
+ Table "public.part_column_drop"
+ Column | Type | Collation | Nullable | Default
++-+---+--+-
+ id | integer | | |
+ d | integer | | |
+ b | integer | | |
+Partition key: RANGE (id)
+Indexes:
+"part_column_drop_b_expr" btree ((b = 1))
+"part_column_drop_b_pred" btree (b) WHERE b = 1
+"part_column_drop_d_expr" btree ((d = 2))
+"part_column_drop_d_pred" btree (d) WHERE d = 2
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d part_column_drop_1_10
+ Table "public.part_column_drop_1_10"
+ Column | Type | Collation | Nullable | Default
++-+---+--+-
+ id | integer | | |
+ d | integer | | |
+ b | integer | | |
+Partition of: part_column_drop FOR VALUES FROM (1) TO (10)
+Indexes:
+"part_column_drop_1_10_b_idx" btree (b) WHERE b = 1
+"part_column_drop_1_10_d_idx" btree (d) WHERE d = 2
+"part_column_drop_1_10_expr_idx" btree ((b = 1))
+"part_column_drop_1_10_expr_idx1" btree ((d = 2))
+
+drop table part_column_drop;
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 27e6f59e87..601a4b9dc3 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -814,3 +814,26 @@ create trigger trig_part_create before insert on
Re: [BUG] Partition creation fails after dropping a column and adding a partial index
On Tue, Oct 29, 2019 at 01:16:58PM +0900, Michael Paquier wrote: > Yes, something looks wrong with that. I have not looked at it in > details yet though. I'll see about that tomorrow. So.. When building the attribute map for a cloned index (with either LIKE during the transformation or for partition indexes), we store each attribute number with 0 used for dropped columns. Unfortunately, if you look at the way the attribute map is built in this case the code correctly generates the mapping with convert_tuples_by_name_map. But, the length of the mapping used is incorrect as this makes use of the number of attributes for the newly-created child relation, and not the parent which includes the dropped column in its count. So the answer is simply to use the parent as reference for the mapping length. The patch is rather simple as per the attached, with extended regression tests included. I have not checked on back-branches yet, but that's visibly wrong since 8b08f7d down to v11 (will do that when back-patching). There could be a point in changing convert_tuples_by_name_map & co so as they return the length of the map on top of the map to avoid such mistakes in the future. That's a more invasive patch not really adapted for a back-patch, but we could do that on HEAD once this bug is fixed. I have also checked other calls of this API and the handling is done correctly. Wyatt, what do you think? -- Michael diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 8d25d14772..5597be6e3d 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -1091,7 +1091,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, RelationGetDescr(parent)); idxstmt = generateClonedIndexStmt(NULL, idxRel, - attmap, RelationGetDescr(rel)->natts, + attmap, RelationGetDescr(parent)->natts, ); DefineIndex(RelationGetRelid(rel), idxstmt, diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out index ce2484fd4e..f63016871c 100644 --- a/src/test/regress/expected/create_table.out +++ b/src/test/regress/expected/create_table.out @@ -1168,3 +1168,52 @@ insert into defcheck_def values (0, 0); create table defcheck_0 partition of defcheck for values in (0); ERROR: updated partition constraint for default partition "defcheck_def" would be violated by some row drop table defcheck; +-- tests of column drop with partition tables and indexes using +-- predicates and expressions. +create table part_column_drop ( + useless_1 int, + id int, + useless_2 int, + d int, + b int, + useless_3 int +) partition by range (id); +alter table part_column_drop drop column useless_1; +alter table part_column_drop drop column useless_2; +alter table part_column_drop drop column useless_3; +create index part_column_drop_b_pred on part_column_drop(b) where b = 1; +create index part_column_drop_b_expr on part_column_drop((b = 1)); +create index part_column_drop_d_pred on part_column_drop(d) where d = 2; +create index part_column_drop_d_expr on part_column_drop((d = 2)); +create table part_column_drop_1_10 partition of + part_column_drop for values from (1) to (10); +\d part_column_drop +Partitioned table "public.part_column_drop" + Column | Type | Collation | Nullable | Default ++-+---+--+- + id | integer | | | + d | integer | | | + b | integer | | | +Partition key: RANGE (id) +Indexes: +"part_column_drop_b_expr" btree ((b = 1)) +"part_column_drop_b_pred" btree (b) WHERE b = 1 +"part_column_drop_d_expr" btree ((d = 2)) +"part_column_drop_d_pred" btree (d) WHERE d = 2 +Number of partitions: 1 (Use \d+ to list them.) + +\d part_column_drop_1_10 + Table "public.part_column_drop_1_10" + Column | Type | Collation | Nullable | Default ++-+---+--+- + id | integer | | | + d | integer | | | + b | integer | | | +Partition of: part_column_drop FOR VALUES FROM (1) TO (10) +Indexes: +"part_column_drop_1_10_b_idx" btree (b) WHERE b = 1 +"part_column_drop_1_10_d_idx" btree (d) WHERE d = 2 +"part_column_drop_1_10_expr_idx" btree ((b = 1)) +"part_column_drop_1_10_expr_idx1" btree ((d = 2)) + +drop table part_column_drop; diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index 144eeb480d..e835b65ac4 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -903,3 +903,26 @@ create table defcheck_1 partition of defcheck for values in (1, null); insert into defcheck_def values (0, 0); create table defcheck_0 partition of defcheck for values in (0); drop table defcheck; + +-- tests of column drop with partition tables and indexes using +--
Re: [BUG] Partition creation fails after dropping a column and adding a partial index
Here's a slightly smaller repro: https://gist.github.com/wkalt/36720f39c97567fa6cb18cf5c05ac60f
Re: [BUG] Partition creation fails after dropping a column and adding a partial index
On Mon, Oct 28, 2019 at 09:00:24PM -0700, Wyatt Alt wrote: > I think this demonstrates a bug, tested in 11.5: > https://gist.github.com/wkalt/a298fe82c564668c803b3537561e67a0 If this source goes away, then we would lose it. It is always better to copy directly the example in the message sent to the mailing lists, and here it is: create table demo ( id int, useless int, d timestamp, b boolean ) partition by range (id, d); alter table demo drop column useless; -- only seems to cause failure when it's a partial index on b. create index on demo(b) where b = 't'; create table demo_1_20191031 partition of demo for values from (1, '2019-10-31') to (1, '2019-11-01'); > The same script succeeds if the index on line 11 is either dropped, made to > be non-partial on b, or shifted to a different column (the others are used > in the partitioning; maybe significant). > > This seems somewhat related to > https://www.postgresql.org/message-id/flat/CAFjFpRc0hqO5hc-%3DFNePygo9j8WTtOvvmysesnN8bfkp3vxHPQ%40mail.gmail.com#00ca695e6c71834622a6e42323f5558a Yes, something looks wrong with that. I have not looked at it in details yet though. I'll see about that tomorrow. -- Michael signature.asc Description: PGP signature
[BUG] Partition creation fails after dropping a column and adding a partial index
Hello, I think this demonstrates a bug, tested in 11.5: https://gist.github.com/wkalt/a298fe82c564668c803b3537561e67a0 The same script succeeds if the index on line 11 is either dropped, made to be non-partial on b, or shifted to a different column (the others are used in the partitioning; maybe significant). This seems somewhat related to https://www.postgresql.org/message-id/flat/CAFjFpRc0hqO5hc-%3DFNePygo9j8WTtOvvmysesnN8bfkp3vxHPQ%40mail.gmail.com#00ca695e6c71834622a6e42323f5558a . Regards, Wyatt
