Date: Sat, 30 Jan 2021 11:23:01 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Steve Singer <steve@ssinger.info>,
	pgsql-hackers@lists.postgresql.org,
	Robert Haas <robertmhaas@gmail.com>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on
 the fly
Message-ID: <YBTC0IYIGmyYfiIn@paquier.xyz>
References: <20210127213658.GA736@alvherre.pgsql>
 <55c6c1526b94d2caf54c33262fe00674@postgrespro.ru>
 <e15777decc533006c9dbe4988bd66d31@postgrespro.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="3ijwfRZFbUKs6mY+"
Content-Disposition: inline
In-Reply-To: <e15777decc533006c9dbe4988bd66d31@postgrespro.ru>
Precedence: bulk


--3ijwfRZFbUKs6mY+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 29, 2021 at 08:56:47PM +0300, Alexey Kondratov wrote:
> On 2021-01-28 14:42, Alexey Kondratov wrote:
>> No, you are right, we are doing REINDEX with AccessExclusiveLock as it
>> was before. This part is a more specific one. It only applies to
>> partitioned indexes, which do not hold any data, so we do not reindex
>> them directly, only their leafs. However, if we are doing a TABLESPACE
>> change, we have to record it in their pg_class entry, so all future
>> leaf partitions were created in the proper tablespace.
>>=20
>> That way, we open partitioned index relation only for a reference,
>> i.e. read-only, but modify pg_class entry under a proper lock
>> (RowExclusiveLock). That's why I thought that ShareLock will be
>> enough.
>>=20
>> IIUC, 'ALTER TABLE ... SET TABLESPACE' uses AccessExclusiveLock even
>> for relations with no storage, since AlterTableGetLockLevel() chooses
>> it if AT_SetTableSpace is met. This is very similar to our case, so
>> probably we should do the same?
>>=20
>> Actually it is not completely clear for me why
>> ShareUpdateExclusiveLock is sufficient for newly added
>> SetRelationTableSpace() as Michael wrote in the comment.

Nay, it was not fine.  That's something Alvaro has mentioned, leading
to 2484329.  This also means that the main patch of this thread should
refresh the comments at the top of CheckRelationTableSpaceMove() and
SetRelationTableSpace() to mention that this is used by REINDEX
CONCURRENTLY with a lower lock.

> Changed patch to use AccessExclusiveLock in this part for now. This is wh=
at
> 'ALTER TABLE/INDEX ... SET TABLESPACE' and 'REINDEX' usually do. Anyway, =
all
> real leaf partitions are processed in the independent transactions later.

+       if (partkind =3D=3D RELKIND_PARTITIONED_INDEX)
+       {
+           Relation iRel =3D index_open(partoid, AccessExclusiveLock);
+
+           if (CheckRelationTableSpaceMove(iRel, params->tablespaceOid))
+               SetRelationTableSpace(iRel,
+                                     params->tablespaceOid,
+                                     InvalidOid);
+           index_close(iRel, NoLock);
Are you sure that this does not represent a risk of deadlocks as EAL
is not taken consistently across all the partitions?  A second issue
here is that this breaks the assumption of REINDEX CONCURRENTLY kicked
on partitioned relations that should use ShareUpdateExclusiveLock for
all its steps.  This would make the first transaction invasive for the
user, but we don't want that.

This makes me really wonder if we would not be better to restrict this
operation for partitioned relation as part of REINDEX as a first step.
Another thing, mentioned upthread, is that we could do this part of
the switch at the last transaction, or we could silently *not* do the
switch for partitioned indexes in the flow of REINDEX, letting users
handle that with an extra ALTER TABLE SET TABLESPACE once REINDEX has
finished on all the partitions, cascading the command only on the
partitioned relation of a tree.  It may be interesting to look as well
at if we could lower the lock used for partitioned relations with
ALTER TABLE SET TABLESPACE from AEL to SUEL, choosing AEL only if at
least one partition with storage is involved in the command,
CheckRelationTableSpaceMove() discarding anything that has no need to
change.
--
Michael

--3ijwfRZFbUKs6mY+
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmAUwwUACgkQnvQgOdby
QH2q1g/+Pq5H5eb8ycIsMiqjOiYazQG3pZ33DFyoHQJTAOEbv+MJPcJjcA7EHgr0
Tf4oZSR6D4H36BILWZ5Hgv2aa67T+AsP5STE8O/crE2ob7jNfcHrxL+S80dN1zD7
V292eXUsFTRGAoIrPyovW2SbgtanMVHjgRnbweyf/hWORizH2JmxWjUgRygO+jBJ
9EsfBv8bosGymk++UyI56DreSoYQ3XtmBPcTNM3p/bD0yqfZrI+9pgP37+vaPEPY
q2bVphtr+40roxVQBPQp05oANL48CiLT2jeH/wUJ3BOC7X4ctxTsiKw6P7j6tONf
JiTT3cRaJWUDFsCkGhwEaJKvS8+OPoHWqLsZsNMTJ8AD6Bj+kvoIM9hJbJVnoWZR
MjEp4UVDIz7FMYxs2RJR1hxOo5H1jSiWpRAWwYYcGHJ2u5BAgsoLiGdoUfL73QyK
+CWFINgACG3k+/n/XT/bxSyrM8s4U2klMBqM1iR3j19Xg13qesENB3qMnMXEuNBL
8awGgpHBkj7KaP/DTDIW7BfJTX7kslYoSXRqULsfxbuXW5Blvd1I/eJE3kQ3Z0sG
o25bdcoQaanxBlDp/5czRpb+gUEoEq26d3cGdrxy6O1PNx78+YFzJsTpq+D4wGFe
am8XJF9ycMaz7gSzqNuu7hhQiIO3BzOxXG/CViLWts+jnA3ICnE=
=2Gee
-----END PGP SIGNATURE-----

--3ijwfRZFbUKs6mY+--