Date: Wed, 27 Jan 2021 11:07:41 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Refactor code in tablecmds.c to check and process
 tablespace mov
Message-ID: <20210127140741.GA14174@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <E1l4az0-0000uQ-Mz@gemulon.postgresql.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2021-Jan-27, Michael Paquier wrote:

> Refactor code in tablecmds.c to check and process tablespace moves

Thanks, looks good.  Small comment: CheckRelationTableSpaceMove is
documented as

+ * Returns true if the relation can be moved to the new tablespace;
+ * false otherwise.

but in reality it returns false if the relation does not *need* to be
moved because the tablespace is the same as before.  In the cases where
it "cannot" be moved, what it does is raise an error.

Maybe this needs is just be documented more clearly:

"Returns true if the relation can be moved to the new tablespace; raises
an error if it is not possible to do the move; returns false if the move
would have no effect."


For cases of relation with storage, I find it suspicious that the
functions are documented to be fine with just ShareUpdateExclusiveLock.
I think it'd be safer for the comment to explicitly indicate that for
relations with storage, lock should be AEL.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W