Re: CREATE INDEX CONCURRENTLY on partitioned index

Ilya Gladyshev писал(а) 2024-05-24 00:14:
> Hi,

Hi.

> 
> I think it's well worth the effort to revive the patch, so I rebased it 
> on master, updated it and will return it back to the commitfest. 
> Alexander, Justin feel free to add yourselves as authors
> 
> On 29.01.2024 12:43, Alexander Pyhalov wrote:
>> Hi.
>> 
>> I've rebased patch on master and it'seems to me there's one more issue 
>> -
>> 
>> when we call DefineIndexConcurrentInternal() in partitioned case, it 
>> waits for transactions, locking tableId, not tabrelid - heaprelid 
>> LockRelId is constructed for parent index relation, not for child 
>> index relation.
>> 
>> Attaching fixed version.
>> 
>> Also I'm not sure what to do with locking of child relations. If we 
>> don't do anything, you can drop one of the partitioned table childs 
>> while CIC is in progress, and get error
>> 
>> ERROR:  cache lookup failed for index 16399
> I agree that we need to do something about it, in particular, I think 
> we should lock all the partitions inside the transaction that builds 
> the catalog entries. Fixed this in the new version.
>> If you try to lock all child tables in CIC session, you'll get 
>> deadlocks.
> 
> Do you mean the deadlock between the transaction that drops a partition 
> and the transaction doing CIC? I think this is unavoidable and can be 
> reproduced even without partitioning.

Yes, it seems we trade this error for possible deadlock between 
transaction, dropping a partition, and CIC.

> 
> Also not sure why a list of children relation was obtained with 
> ShareLock that CIC is supposed to avoid not to block writes, changed 
> that to ShareUpdateExclusive.
> 

I expect that it wasn't an issue due to the fact that it's held for a 
brief period until DefineIndexConcurrentInternal() commits for the first 
time. But it seems, it's more correct to use ShareUpdateExclusive lock 
here.


Also I'd like to note that in new patch version there's a strange 
wording in documentation:

"This can be very convenient as not only will all existing partitions be
  indexed, but any future partitions will be as well.
  <command>CREATE INDEX ... CONCURRENTLY</command> can incur long lock 
times
  on huge partitioned tables, to avoid that you can
  use <command>CREATE INDEX ON ONLY</command> the partitioned table, 
which
  creates the new index marked as invalid, preventing automatic 
application
  to existing partitions."

All the point of CIC is to avoid long lock times. So it seems this 
paragraph should be rewritten in the following way:

"To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
CREATE INDEX ON ONLY</command> the partitioned table..."


-- 
Best regards,
Alexander Pyhalov,
Postgres Professional