public inbox for [email protected]  

help / color / mirror / Atom feed

From: Alexander Pyhalov <[email protected]>
To: Ilya Gladyshev <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: [email protected]
Cc: Masahiko Sawada <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: 李杰(慎追) <[email protected]>
Cc: 曾文旌(义从) <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Subject: Re: CREATE INDEX CONCURRENTLY on partitioned index
Date: Fri, 24 May 2024 12:04:33 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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

view thread (8+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: CREATE INDEX CONCURRENTLY on partitioned index
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox