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 "REINDEX (without )" 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