Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

On 27.11.2019 6:54, Michael Paquier wrote:
> On Tue, Nov 26, 2019 at 11:09:55PM +0100, Masahiko Sawada wrote:
>> I looked at v4 patch. Here are some comments:
>>
>> + /* Skip all mapped relations if TABLESPACE is specified */
>> + if (OidIsValid(tableSpaceOid) &&
>> + classtuple->relfilenode == 0)
>>
>> I think we can use OidIsValid(classtuple->relfilenode) instead.
> Yes, definitely.

Yes, switched to !OidIsValid(classtuple->relfilenode). Also I added a
comment that it is meant to be equivalent to RelationIsMapped() and
extended tests.

>
>> This change says that temporary relation is not supported but it
>> actually seems to work. Which is correct?
> Yeah, I don't really see a reason why it would not work.

My bad, I was keeping in mind RELATION_IS_OTHER_TEMP validation, but it
is for temp tables of other backends only, so it definitely should not
be in the doc. Removed.

> Your patch has forgotten to update copyfuncs.c and equalfuncs.c with
> the new tablespace string field.

Fixed, thanks.

> It would be nice to add tab completion for this new clause in psql.

Added.

> There is no need for opt_tablespace_name as new node for the parsing
> grammar of gram.y as OptTableSpace is able to do the exact same job.

Sure, it was an artifact from the times, where I used optional SET
TABLESPACE clause. Removed.

>
> @@ -3455,6 +3461,8 @@ RelationSetNewRelfilenode(Relation relation,
> char persistence)
> */
> newrnode = relation->rd_node;
> newrnode.relNode = newrelfilenode;
> + if (OidIsValid(tablespaceOid))
> + newrnode.spcNode = newTablespaceOid;
> The core of the patch is actually here. It seems to me that this is a
> very bad idea because you actually hijack a logic which happens at a
> much lower level which is based on the state of the tablespace stored
> in the relation cache entry of the relation being reindexed, then the
> tablespace choice actually happens in RelationInitPhysicalAddr() which
> for the new relfilenode once the follow-up CCI is done. So this very
> likely needs more thoughts, and bringing to the point: shouldn't you
> actually be careful that the relation tablespace is correctly updated
> before reindexing it and before creating its new relfilenode? This
> way, RelationSetNewRelfilenode() does not need any additional work,
> and I think that this saves from potential bugs in the choice of the
> tablespace used with the new relfilenode.

When I did the first version of the patch I was looking on
ATExecSetTableSpace, which implements ALTER ... SET TABLESPACE. And
there is very similar pipeline there:

1) Find pg_class entry with SearchSysCacheCopy1

2) Create new relfilenode with GetNewRelFileNode

3) Set new tablespace for this relfilenode

4) Do some work with new relfilenode

5) Update pg_class entry with new tablespace

6) Do CommandCounterIncrement

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?

Thus, I cannot get your point correctly here. Can you, please, elaborate
a little bit more your concerns?

>> ISTM the kind of above errors are the same: the given tablespace
>> exists but moving tablespace to it is not allowed since it's not
>> supported in PostgreSQL. So I think we can use
>> ERRCODE_FEATURE_NOT_SUPPORTED instead of
>> ERRCODE_INVALID_PARAMETER_VALUE (which is used at 3 places) .
> Yes, it is also not project style to use full sentences in error
> messages, so I would suggest instead (note the missing quotes in the
> original patch):
> cannot move non-shared relation to tablespace \"%s\"

Same here. I have taken this validation directly from tablecmds.c part
for ALTER ... SET TABLESPACE. And there is exactly the same message
"only shared relations can be placed in pg_global tablespace" with
ERRCODE_INVALID_PARAMETER_VALUE there.

However, I understand your point, but still, would it be better if I
stick to the same ERRCODE/message? Or should I introduce new
ERRCODE/message for the same case?

> And I have somewhat missed to notice the timing of the review replies
> as you did not have room to reply, so fixed the CF entry to "waiting
> on author", and bumped it to next CF instead.

Thank you! Attached is a patch, that addresses all the issues above,
excepting the last two points (core part and error messages for
pg_global), which are not clear for me right now.

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company