Thread: pgsql: Refactor code in tablecmds.c to check and process tablespace mov

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

Two code paths of tablecmds.c (for relations with storage and without
storage) use the same logic to check if the move of a relation to a
new tablespace is allowed or not and to update pg_class.reltablespace
and pg_class.relfilenode.

A potential TABLESPACE clause for REINDEX, CLUSTER and VACUUM FULL needs
similar checks to make sure that nothing is moved around in illegal ways
(no mapped relations, shared relations only in pg_global, no move of
temp tables owned by other backends).

This reorganizes the existing code of ALTER TABLE so as all this logic
is controlled by two new routines that can be reused for the other
commands able to move relations across tablespaces, limiting the number
of code paths in need of the same protections.  This also removes some
code that was duplicated for tables with and without storage for ALTER
TABLE.

Author: Alexey Kondratov, Michael Paquier
Discussion: https://postgr.es/m/YA+9mAMWYLXJMVPL@paquier.xyz

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/4c9c359d38ff1e2de388eedd860785be6a49201c

Modified Files
--------------
src/backend/commands/tablecmds.c | 211 ++++++++++++++++++++++-----------------
src/include/commands/tablecmds.h |   4 +
2 files changed, 124 insertions(+), 91 deletions(-)

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

On Wed, Jan 27, 2021 at 11:07:41AM -0300, Alvaro Herrera wrote:
> Thanks, looks good.  Small comment: CheckRelationTableSpaceMove is
> documented as

  * newTableSpaceId is the new tablespace for the relation, and
- * newRelFileNode its new filenode.  If newrelfilenode is InvalidOid,
+ * newRelFileNode its new filenode.  If newRelFileNode is InvalidOid,
  * this field is not updated.
Looks like I have put an incorrect variable name in one of the new
comments as well.

> 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."

Good idea.

> 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.

Yeah, it is true that ShareUpdateExclusiveLock may not be completely
safe as it depends on the context where the operation is done.  For
REINDEX CONCURRENTLY, it would actually be logically fine even if an
AEL is not hold on the relation thanks to the intermediate waits.  You
are right that switching to an AEL in this part makes sense as of
HEAD.  However, for REINDEX CONCURRENTLY we will also make use of
these routines, but maybe we could just add an extra note the comment
block of this stuff once we begin to use it?

So, for now, I would just do the attached to address your
suggestions.  Is that fine for you?
--
Michael

On 2021-Jan-28, Michael Paquier wrote:

> So, for now, I would just do the attached to address your
> suggestions.  Is that fine for you?

Yep, seems OK to me, assuming that, as you say, you'll update the
comments again when they are used to handle the concurrent cases.

Thanks!

-- 
Álvaro Herrera       Valdivia, Chile
"There was no reply" (Kernel Traffic)

On Thu, Jan 28, 2021 at 10:56:29AM -0300, Alvaro Herrera wrote:
> Yep, seems OK to me, assuming that, as you say, you'll update the
> comments again when they are used to handle the concurrent cases.

That's the idea.  I need first to look in depth at what's proposed on
the other thread for REINDEX, but I see no reason why the case of
REINDEX CONCURRENTLY would not be able to use this stuff.  Applied
this part for now.  Thanks.
--
Michael