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.