Re: [HACKERS] REINDEX CONCURRENTLY 2.0

Hello
Thank you! I review new patch version. It applied, builds and pass tests. Code looks good, but i notice new behavior notes:

> postgres=# reindex (verbose) table CONCURRENTLY measurement ;
> WARNING: REINDEX of partitioned tables is not yet implemented, skipping "measurement"
> NOTICE: table "measurement" has no indexes
> REINDEX
> postgres=# \d measurement
> Partitioned table "public.measurement"
> Column | Type | Collation | Nullable | Default
> -----------+---------+-----------+----------+---------
> city_id | integer | | not null |
> logdate | date | | not null |
> peaktemp | integer | | |
> unitsales | integer | | |
> Partition key: RANGE (logdate)
> Indexes:
> "measurement_logdate_idx" btree (logdate)
> Number of partitions: 0

NOTICE seems unnecessary here.

Unfortunally concurrenttly reindex loses comments, reproducer:

> create table testcomment (i int);
> create index testcomment_idx1 on testcomment (i);
> comment on index testcomment_idx1 IS 'test comment';
> \di+ testcomment_idx1
> reindex table testcomment ;
> \di+ testcomment_idx1 # ok
> reindex table CONCURRENTLY testcomment ;
> \di+ testcomment_idx1 # we lose comment

Also i think we need change REINDEX to "<command>REINDEX</command> (without <option>CONCURRENTLY</option>)" in ACCESS EXCLUSIVE section Table-level Lock Modes documentation (to be similar with REFRESH MATERIALIZED VIEW and CREATE INDEX description)

About reindex invalid indexes - i found one good question in archives [1]: how about toast indexes?
I check it now, i am able drop invalid toast index, but i can not drop reduntant valid index.
Reproduce:
session 1: begin; select from test_toast ... for update;
session 2: reindex table CONCURRENTLY test_toast ;
session 2: interrupt by ctrl+C
session 1: commit
session 2: reindex table test_toast ;
and now we have two toast indexes. DROP INDEX is able to remove only invalid ones. Valid index gives "ERROR: permission denied: "pg_toast_16426_index_ccnew" is a system catalog"

About syntax: i vote for current syntax "reindex table CONCURRENTLY tablename". This looks consistent with existed CREATE INDEX CONCURRENTLY and REFRESH MATERIALIZED VIEW CONCURRENTLY.

regards, Sergei

[1]: https://www.postgresql.org/message-id/CAB7nPqT%2B6igqbUb59y04NEgHoBeUGYteuUr89AKnLTFNdB8Hyw%40mail.gmail.com