Hi, For v13-0006-More-refactoring.patch : + /* It's not a shared catalog, so refuse to move it to shared tablespace */ + if (params->tablespaceOid == GLOBALTABLESPACE_OID && false) + ereport(ERROR,
Do you intend to remove the ineffective check ? + else + heapRelation = table_open(heapId, + ShareUpdateExclusiveLock); + table_close(heapRelation, NoLock); The table_open() seems to be unnecessary since there is no check after the open. + // heapRelationIds = list_make1_oid(heapId); If the code is not needed, you can remove the above. For v13-0005-Refactor-to-allow-reindexing-all-index-partition.patch : + /* Skip invalid indexes, if requested */ + if ((options & REINDEXOPT_SKIPVALID) != 0 && + get_index_isvalid(partoid)) The comment seems to diverge from the name of the flag (which says skip valid index). Cheers On Mon, Feb 15, 2021 at 11:34 AM Justin Pryzby <pry...@telsasoft.com> wrote: > On Mon, Feb 15, 2021 at 10:06:47PM +0300, Anastasia Lubennikova wrote: > > On 28.01.2021 17:30, Justin Pryzby wrote: > > > On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote: > > > > On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby <pry...@telsasoft.com> > wrote: > > > > > On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote: > > > > > > Forking this thread, since the existing CFs have been closed. > > > > > > > https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd > > > > > > > > > > > > The strategy is to create catalog entries for all tables with > indisvalid=false, > > > > > > and then process them like REINDEX CONCURRENTLY. If it's > interrupted, it > > > > > > leaves INVALID indexes, which can be cleaned up with DROP or > REINDEX, same as > > > > > > CIC on a plain table. > > > > > > > > > > > > On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote: > > > > > > > On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier > wrote: > > > > > > > Note that the mentioned problem wasn't serious: there was > missing index on > > > > > > > child table, therefor the parent index was invalid, as > intended. However I > > > > > > > agree that it's not nice that the command can fail so easily > and leave behind > > > > > > > some indexes created successfully and some failed some not > created at all. > > > > > > > > > > > > > > But I took your advice initially creating invalid inds. > > > > > > ... > > > > > > > That gave me the idea to layer CIC on top of Reindex, since I > think it does > > > > > > > exactly what's needed. > > > > > > On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote: > > > > > > > On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier > wrote: > > > > > > > > It would be good also to check if > > > > > > > > we have a partition index tree that maps partially with a > partition > > > > > > > > table tree (aka no all table partitions have a partition > index), where > > > > > > > > these don't get clustered because there is no index to work > on. > > > > > > > This should not happen, since a incomplete partitioned index > is "invalid". > > > > > > > > > I had been waiting to rebase since there hasn't been any > review comments and I > > > > > > > expected additional, future conflicts. > > > > > > > > > > > I attempted to review this feature, but the last patch conflicts with the > > recent refactoring, so I wasn't able to test it properly. > > Could you please send a new version? > > I rebased this yesterday, so here's my latest. > > > 2) Here we access relation field after closing the relation. Is it safe? > > /* save lockrelid and locktag for below */ > > heaprelid = rel->rd_lockInfo.lockRelId; > > Thanks, fixed this just now. > > > 3) leaf_partitions() function only handles indexes, so I suggest to name > it > > more specifically and add a comment about meaning of 'options' parameter. > > > > 4) I don't quite understand the idea of the regression test. Why do we > > expect to see invalid indexes there? > > + "idxpart_a_idx1" UNIQUE, btree (a) INVALID > > Because of the unique failure: > +create unique index concurrently on idxpart (a); -- partitioned, unique > failure > +ERROR: could not create unique index "idxpart2_a_idx2_ccnew" > +DETAIL: Key (a)=(10) is duplicated. > +\d idxpart > > This shows that CIC first creates catalog-only INVALID indexes, and then > reindexes them to "validate". > > -- > Justin >
