From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> |
Cc: | Andrey Klychkov <aaklychkov(at)mail(dot)ru>, Robert Haas <robertmhaas(at)gmail(dot)com>, Victor Yegorov <vyegorov(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Alter index rename concurrently to |
Date: | 2018-10-13 02:01:31 |
Message-ID: | 20181013020131.waeojm6anbs4mavy@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 2018-10-05 12:03:54 +0200, Peter Eisentraut wrote:
> From 37591a06901e2d694e3928b7e1cddfcfd84f6267 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e(at)gmx(dot)net>
> Date: Mon, 13 Aug 2018 22:38:36 +0200
> Subject: [PATCH v2] Lower lock level for renaming indexes
>
> Change lock level for renaming index (either ALTER INDEX or implicitly
> via some other commands) from AccessExclusiveLock to
> ShareUpdateExclusiveLock.
>
> The reason we need a strong lock at all for relation renaming is that
> the name change causes a rebuild of the relcache entry. Concurrent
> sessions that have the relation open might not be able to handle the
> relcache entry changing underneath them. Therefore, we need to lock the
> relation in a way that no one can have the relation open concurrently.
> But for indexes, the relcache handles reloads specially in
> RelationReloadIndexInfo() in a way that keeps changes in the relcache
> entry to a minimum. As long as no one keeps pointers to rd_amcache and
> rd_options around across possible relcache flushes, which is the case,
> this ought to be safe.
>
> One could perhaps argue that this could all be done with an
> AccessShareLock then. But we standardize here for consistency on
> ShareUpdateExclusiveLock, which is already used by other DDL commands
> that want to operate in a concurrent manner. For example, renaming an
> index concurrently with CREATE INDEX CONCURRENTLY might be trouble.
I don't see how this could be argued. It has to be a self-conflicting
lockmode, otherwise you'd end up doing renames of tables where you
cannot see the previous state. And you'd get weird errors about updating
invisible rows etc.
> @@ -3155,11 +3157,13 @@ RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal)
> Oid namespaceId;
>
> /*
> - * Grab an exclusive lock on the target table, index, sequence, view,
> - * materialized view, or foreign table, which we will NOT release until
> - * end of transaction.
> + * Grab a lock on the target relation, which we will NOT release until end
> + * of transaction. The lock here guards mainly against triggering
> + * relcache reloads in concurrent sessions, which might not handle this
> + * information changing under them. For indexes, we can use a reduced
> + * lock level because RelationReloadIndexInfo() handles indexes specially.
> */
I don't buy this description. Imo it's a fundamental correctness
thing. Without it concurrent DDL would potentially overwrite the rename,
because they could start updating while still seeing the old version.
Greetings,
Andres Freund
From | Date | Subject | |
---|---|---|---|
Next Message | Bossart, Nathan | 2018-10-13 04:30:05 | Re: Maximum password length |
Previous Message | Andres Freund | 2018-10-13 00:12:33 | Re: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel |