Re: Alter index rename concurrently to

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