Thread: [BUG] Partition creation fails after dropping a column and adding apartial 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

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

On Thu, Oct 31, 2019 at 1:45 PM Michael Paquier <michael@paquier.xyz> 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

On Fri, Nov 01, 2019 at 09:58:26AM +0900, Amit Langote wrote:
> On Thu, Oct 31, 2019 at 1:45 PM Michael Paquier <michael@paquier.xyz> 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