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