Re: CREATE INDEX CONCURRENTLY on partitioned index - Mailing list pgsql-hackers

On 24.05.2024 10:04, Alexander Pyhalov wrote:
> 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..."


True, the current wording doesn't look right. Right now CREATE INDEX ON 
ONLY is described as a workaround for the missing CIC. I think it rather 
makes sense to say that it gives more fine-grained control of partition 
locking than both CIC and ordinary CREATE INDEX. See the updated patch.