Re: Alter index rename concurrently to - Mailing list 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@gmx.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