Re: 回复：how to create index concurrently on partitioned table

On Wed, Aug 12, 2020 at 12:28:20AM -0500, Justin Pryzby wrote:
> On Wed, Aug 12, 2020 at 01:54:38PM +0900, Michael Paquier wrote:
>> +++ b/src/backend/catalog/index.c
>> @@ -3661,20 +3662,12 @@ reindex_relation(Oid relid, int flags, int options)
>> + elog(ERROR, "unsupported relation kind for relation \"%s\"",
>> + RelationGetRelationName(rel));
>
> I guess it should show the relkind(%c) in the message, like these:

Sure, but I don't see much the point in adding the relkind here
knowing that we know which one it is.

> ISTM reindex_index is missing that, too:
>
> 8b08f7d4820fd7a8ef6152a9dd8c6e3cb01e5f99
> + if (iRel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
> + elog(ERROR, "unsupported relation kind for index \"%s\"",
> + RelationGetRelationName(iRel));

The error string does not follow the usual project style either, so I
have updated both.

>> <para>
>> - Reindexing partitioned tables or partitioned indexes is not supported.
>> - Each individual partition can be reindexed separately instead.
>> + Reindexing partitioned indexes or partitioned tables is supported
>> + with respectively <command>REINDEX INDEX</command> or
>> + <command>REINDEX TABLE</command>.
>
> Should say "..with REINDEX INDEX or REINDEX TABLE, respectively".
>
>> + Each partition of the partitioned
>> + relation defined is rebuilt in its own transaction.
>
> => Each partition of the specified partitioned relation is reindexed in a
> separate transaction.

Thanks, good idea.

I have been able to work more on this patch today, and finally added
an error context for the transaction block check, as that's cleaner.
In my manual tests, I have also bumped into a case that failed with
the original patch (where there were no session locks), and created
an isolation test based on it: drop of a partition leaf concurrently
to REINDEX done on the parent. One last thing I have spotted is that
we failed to discard properly foreign tables defined as leaves of a
partition tree, causing a reindex to fail, so reindex_partitions()
ought to just use RELKIND_HAS_STORAGE() to do its filtering work. I
am leaving this patch alone for a couple of days now, and I'll try to
come back to it after and potentially commit it. The attached has
been indented by the way.
--
Michael