Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly - Mailing list pgsql-hackers

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