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
