On Tue, 11 Dec 2018 at 15:43, Michael Paquier <michael@paquier.xyz> wrote:
> + parentrel = heap_openrv(parent, AccessExclusiveLock);
> So, in order to determine which tablespace should be used here, an
> exclusive lock is taken on the parent because its partition descriptor
> gets updated by the addition of the new partition. This process is
> actually done already in MergeAttributes() as well, but it won't get
> triggered if a tablespace is defined directly in the CREATE TABLE
> statement. I think that we should add a comment to explain the
> dependency between both, as well as why an exclusive lock is needed, so
> something among those lines perhaps? Here is an idea:
> + /*
> + * Grab an exclusive lock on the parent because its partition
> + * descriptor will be changed by the addition of the new partition.
> + * The same lock level is taken as when merging attributes below
> + * in MergeAttributes() to protect from lock upgrade deadlocks.
> + */
>
> The position where the tablespace is chosen is definitely the good one.
>
> What do you think?
I think a comment in that location is a good idea. There's work being
done to reduce the lock level required for attaching a partition so a
comment here will help show that it's okay to reduce the lock level
for fetching the tablespace too.
I've attached an updated patch that includes the new comment. I didn't
use your proposed words though.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services