Date: Thu, 21 Jan 2021 10:41:46 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Alexey Kondratov <a.kondratov@postgrespro.ru>,
	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: <YAjb2kYBKH07H5Xv@paquier.xyz>
References: <03f88f70618ce73e75837b6125a143f7@postgrespro.ru>
 <20210120183439.GA21339@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="n39a3p29B5FL+J79"
Content-Disposition: inline
In-Reply-To: <20210120183439.GA21339@alvherre.pgsql>
Precedence: bulk


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

On Wed, Jan 20, 2021 at 03:34:39PM -0300, Alvaro Herrera wrote:
> On 2021-Jan-20, Alexey Kondratov wrote:
>> Ugh, forgot to attach the patches. Here they are.
>=20
> Yeah, looks reasonable.

Patch 0002 still has a whole set of issues as I pointed out a couple
of hours ago, but if we agree on 0001 as being useful if done
independently, I'd rather get that done first.  This way or just
merging both things in a single commit is not a big deal seeing the
amount of code, so I am fine with any approach.  It may be possible
that 0001 requires more changes depending on the work to-be-done for
0002 though?

>> +	/* No work if no change in tablespace. */
>> +	oldTablespaceOid =3D rd_rel->reltablespace;
>> +	if (tablespaceOid !=3D oldTablespaceOid ||
>> +		(tablespaceOid =3D=3D MyDatabaseTableSpace && OidIsValid(oldTablespac=
eOid)))
>> +	{
>> +		/* Update the pg_class row. */
>> +		rd_rel->reltablespace =3D (tablespaceOid =3D=3D MyDatabaseTableSpace)=
 ?
>> +			InvalidOid : tablespaceOid;
>> +		CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
>> +
>> +		changed =3D true;
>> +	}
>> +
>> +	if (changed)
>> +		/* Record dependency on tablespace */
>> +		changeDependencyOnTablespace(RelationRelationId,
>> +									 reloid, rd_rel->reltablespace);
>=20
> Why have a separate "if (changed)" block here instead of merging with
> the above?

Yep.

+       if (SetRelTablespace(reloid, newTableSpace))
+               /* Make sure the reltablespace change is visible */
+               CommandCounterIncrement();
At quick glance, I am wondering why you just don't do a CCI within
SetRelTablespace().
--
Michael

--n39a3p29B5FL+J79
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmAI29oACgkQnvQgOdby
QH3IexAAoFyFMTHSV7ZTvmla/2MDg+xPZ/yElqrYYTslNAQNB7bBujHB9uV3sl9d
Rwd4yIej8ff5yNGjhVtvDUxLjcYCIBKmwRSTEgekvBxth1ter6+VNndkDukiDYsl
l3Q7s82TJlvuWz0CMqkxsh2dt0v1Vr65YWqXZu4Vr3Dqn/471bQdulkD62SWTnJK
SiJp7L7H5i7S825GTdfLz+Rqeg8hdnSVMSY80EYAZR5bv/LSvVuRFi5eL6pt/Vot
JKl7mRRben5on0HCEMWSesLbL2OnYDfdJ0AClc9cNCInhSbc+Cz4mol1dAvbphM7
Y1dBL8FbjidPnl4X6gE3BSsmtZ5BujFjsE4khR31/K/aeVQZize2KEsMlZD1ejB1
JCKTDPvT+NN7CfxyX3LW5C8Opc/eJYsXRjNuggnShcqmrWW3KjeQI8ZqZwKsJfxm
XjBPYawflid1FRtrA0JY1hcdDM69wabPm1aN+f+F3ecHK2GGqRtGv3BoyX2aJxKY
Y4RQNtrp5AvPf6kY/iqe2dX60lUU3/PgG/V331XmH7ei8OTwknhyAifz3SC61EsG
Yifh4oRA+v/fdNTJygUs8KbX4MyCXLLfyCKBFm/T9CdNvLQslJ6YSIZEktGfCv3T
wck6afvR0pSntiIdVXGp9q3O/LfmV4331C7HBVRVEtldQrkpD0E=
=5jDX
-----END PGP SIGNATURE-----

--n39a3p29B5FL+J79--