Re: [HACKERS] Proposal: Local indexes for partitioned table

On 12 January 2018 at 07:52, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> The delta patch turned out to have at least one stupid bug, and a few
> non-stupid bugs. Here's an updated version that should behave
> correctly, and addresses all reported problems.

Thanks for updating the patch.

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

1. Expression varattnos don't seem to get translated correctly.
drop table p;
create table p (a int, b int) partition by list (a);
create table p1 (b int, a int);
alter table p attach partition p1 for values in(1);
create index on p (abs(a));
\d p1
Table "public.p1"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
b | integer | | |
a | integer | | |
Partition of: p FOR VALUES IN (1)
Indexes:
"p1_abs_idx" btree (abs(b))

CompareIndexInfo() seems to validate existing indexes correctly.

Can you also write a test for this?

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;

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

drop table p;
create table p (a int not null) partition by list (a);
create table p1 partition of p for values in(1);
insert into p1 select 1 from generate_Series(1,10000000);

-- Session 1:
create index concurrently on p1 (a);

-- Session 2:
create index on p (a);

-- Session 1:
ERROR: deadlock detected
DETAIL: Process 10860 waits for ShareLock on virtual transaction
3/97; blocked by process 7968.
Process 7968 waits for ShareLock on relation 90492 of database 12342;
blocked by process 10860.
HINT: See server log for query details.

4. nparts initialized twice in DefineIndex()

int nparts = partdesc->nparts;
Oid *part_oids;
TupleDesc parentDesc;
bool invalidate_parent = false;

nparts = partdesc->nparts;

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);

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.

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?

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"?

DROP TABLE p;
CREATE TABLE p (a INT NOT NULL, b INT NOT NULL) PARTITION BY LIST (a);
CREATE TABLE p1 PARTITION OF p FOR VALUES IN(1) PARTITION BY LIST (b);
CREATE TABLE p2 PARTITION OF p1 FOR VALUES IN(1);

CREATE INDEX p_a_idx ON ONLY p (a);

CREATE INDEX p2_a_idx ON p2 (a);
ALTER INDEX p_a_idx ATTACH PARTITION p2_a_idx;
ERROR: cannot attach index "p2_a_idx" as a partition of index "p_a_idx"
DETAIL: Index "p2_a_idx" is not on a partition of table "p".

10. You've added code to get_relation_info() to handle partitioned
indexes, but that code is skipped due to:

/*
* Make list of indexes. Ignore indexes on system catalogs if told to.
* Don't bother with indexes for an inheritance parent, either.
*/
if (inhparent ||

The new code should either be removed, or you should load the index
list for a partitioned table.

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

/* Also, lock any descendant tables if recursive */
if (stmt->relation->inh)
inheritors = find_all_inheritors(relid, lockmode, NULL);

Otherwise we will (perhaps harmlessly) lock children in the following case:

DROP TABLE p;
CREATE TABLE p (a INT NOT NULL, b INT NOT NULL);
CREATE TABLE p1 (a INT NOT NULL, b INT NOT NULL) INHERITS (p);
CREATE INDEX ON p (a);

> I'll set this as Ready for Committer now.

Will set back to waiting on author until the above things are addressed.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services