From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Steve Singer <steve@ssinger.info>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: Mon, 2 Dec 2019 12:41:03 +0300
Message-ID: <e625699b-bb82-a28b-370b-2da4bbdb72ed@postgrespro.ru> (raw)
In-Reply-To: <20191202082134.GI1696@paquier.xyz>
References: <8a8f5f73-00d3-55f8-7583-1375ca8f6a91@postgrespro.ru>
<fe8c0444-ebfc-ce5d-8843-7f93c0f53d7f@postgrespro.ru>
<157395200750.29912.1178609357962324139.pgcf@coridan.postgresql.org>
<827a9139-e02b-2dbf-5c6c-a6fbcaa1739a@postgrespro.ru>
<CA+fd4k4uO5HS8Rb0PgH5oWQYT9xWYONZmLGUyAGvANFke-8c3Q@mail.gmail.com>
<20191127035416.GG5435@paquier.xyz>
<3ae48673-283c-3e99-3dd8-36ebb81614b5@postgrespro.ru>
<20191202082134.GI1696@paquier.xyz>
On 02.12.2019 11:21, Michael Paquier wrote:
> On Wed, Nov 27, 2019 at 08:47:06PM +0300, Alexey Kondratov wrote:
>> The only difference is that point 3) and tablespace part of 5) were missing
>> in RelationSetNewRelfilenode, so I added them, and I do 4) after 6) in
>> REINDEX. Thus, it seems that in my implementation of tablespace change in
>> REINDEX I am more sure that "the relation tablespace is correctly updated
>> before reindexing", since I do reindex after CCI (point 6), doesn't it?
>>
>> So why it is fine for ATExecSetTableSpace to do pretty much the same, but
>> not for REINDEX? Or the key point is in doing actual work before CCI, but
>> for me it seems a bit against what you have wrote?
> Nope, the order is not the same on what you do here, causing a
> duplication in the tablespace selection within
> RelationSetNewRelfilenode() and when flushing the relation on the new
> tablespace for the first time after the CCI happens, please see
> below. And we should avoid that.
>
>> Thus, I cannot get your point correctly here. Can you, please, elaborate a
>> little bit more your concerns?
> The case of REINDEX CONCURRENTLY is pretty simple, because a new
> relation which is a copy of the old relation is created before doing
> the reindex, so you simply need to set the tablespace OID correctly
> in index_concurrently_create_copy(). And actually, I think that the
> computation is incorrect because we need to check after
> MyDatabaseTableSpace as well, no?
No, the same logic already exists in heap_create:
if (reltablespace == MyDatabaseTableSpace)
reltablespace = InvalidOid;
Which is called by index_concurrently_create_copy -> index_create ->
heap_create.
> The case of REINDEX is more tricky, because you are working on a
> relation that already exists, hence I think that what you need to do a
> different thing before the actual REINDEX:
> 1) Update the existing relation's pg_class tuple to point to the new
> tablespace.
> 2) Do a CommandCounterIncrement.
> So I think that the order of the operations you are doing is incorrect,
> and that you have a risk of breaking the existing tablespace assignment
> logic done when first flushing a new relfilenode.
>
> This actually brings an extra thing: when doing a plain REINDEX you
> need to make sure that the past relfilenode of the relation gets away
> properly. The attached POC patch does that before doing the CCI which
> is a bit ugly, but that's enough to show my point, and there is no
> need to touch RelationSetNewRelfilenode() this way.
Thank you for the detailed answer and PoC patch. I will recheck
everything and dig deeper into this problem, and come up with something
closer to the next 01.2020 commitfest.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.kondratov@postgrespro.ru, michael@paquier.xyz, masahiko.sawada@2ndquadrant.com, steve@ssinger.info, pgsql-hackers@lists.postgresql.org, alvherre@2ndquadrant.com, robertmhaas@gmail.com, a.korotkov@postgrespro.ru, sawada.mshk@gmail.com, jltallon@adv-solutions.net
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
In-Reply-To: <e625699b-bb82-a28b-370b-2da4bbdb72ed@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox