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