Re: [HACKERS] Proposal: Local indexes for partitioned table - Mailing list pgsql-hackers

David Rowley wrote:

> I've just made another pass over the patch and have a few more comments.

Thanks!  I'm a bit tied up in a few other things ATM, so an updated
version will have to wait a couple of days.

> 1. Expression varattnos don't seem to get translated correctly.

Yesterday while creating a test case for something suggested by Jaime, I
found that the same problem of bogus attno mapping also appears in
predicates of partial indexes.  My overall conclusion is that I need to
examine varattno mapping carefully, because it seems broken in several
places.

> 2. No stats are gathered for the partitioned expression index.
> 
> drop table p;
> create table p (a int, b int) partition by range (a);
> create table p1 (b int, a int);
> alter table p attach partition p1 for values from (1) to (1001);
> create index on p1 (abs(a));
> create index on p (abs(a));
> insert into p select x,x from generate_series(1,1000) x;
> analyze p, p1;
> 
> select * from pg_statistic where starelid = 'p_abs_idx'::regclass;
> select * from pg_statistic where starelid = 'p1_abs_idx'::regclass;

Fun :-(  I think this is down to expand_vacuum_rel() not considering a
table worth vacuuming.  While I agree that this is a problem, I'm
disclaiming responsibility for fixing it for the time being.  Seems well
outside the scope of this patch.

> 3. deadlocking: I've not yet debugged this to see if this can be avoided.

Yeah, we discussed this before.  I think there's not a lot of room for
improvement in this particular case, though it's pretty unfortunate.

> 4. nparts initialized twice in DefineIndex()
> 
> int nparts = partdesc->nparts;
> Oid    *part_oids;
> TupleDesc parentDesc;
> bool invalidate_parent = false;
> 
> nparts = partdesc->nparts;

Hah.  Having second, third and fourth thoughts on restructuring the
block can do that ... seems I just need a fifth thought.

> 5. In DefineIndex, should the following have a heap_freetuple(newtup); ?
> 
> newtup = heap_copytuple(tup);
> ((Form_pg_index) GETSTRUCT(newtup))->indisvalid = false;
> CatalogTupleUpdate(pg_index, &tup->t_self, newtup);
> ReleaseSysCache(tup);
> heap_close(pg_index, RowExclusiveLock);

I can add it, but *shrug*.  We're not too stressed about never leaking
memory in heavy-handed DDL operations.  Consider indexInfo in the same
routine, for example: we just leave it for end-of-transaction to clean
up.

> 6. Header comment in ReindexPartitionedIndex() claims it "obtains the
> list of children and releases the lock on parent before applying
> reindex on each child.", but it does not do that. It just returns an
> error.

Yeah, it's a stub for now.  We can implement it later.  Fixed the
comment.

> 7. I see you changed StoreSingleInheritance to have the seqnumber as
> int32 instead of int16, but StoreCatalogInheritance1 still has an
> int16 seqNumber parameter. Maybe that should be fixed independently
> and backpatched?

Yes.

> 8. ATExecCmd: You've added an Assert() in the DETACH PARTITION case to
> ensure we're working with a table, but what makes you certain that the
> "else" case on the ATTACH PARTITION is always going to get a
> partitioned index?
> 
> Maybe it would be better to just move the Assert()s into the function
> being called?

> 9. Error details claim that p2_a_idx is not a partition of p.
> Shouldn't it say table "p2" is not a partition of "p"?

You missed the "on" in the DETAIL:
  DETAIL:  Index "p2_a_idx" is not on a partition of table "p".
You could argue that this is obscurely worded, but if you look at the
command:
   ALTER INDEX p_a_idx ATTACH PARTITION p2_a_idx;
nowhere is table p2 mentioned, so I'm not sure it's a great idea to
mention the table in the error message.
> 
> 10. You've added code to get_relation_info() to handle partitioned
> indexes, but that code is skipped [...]
> The new code should either be removed, or you should load the index
> list for a partitioned table.

That's a leftover from previous versions too.  YAGNI principle says I
should remove it rather than activate it, I think, since the optimizer
is not going to use the data for anything.

> 11. In ProcessUtilitySlow(), the following if needs to check if we're
> dealing with a partitioned table:

Sure thing; fixed.

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