Hi Michael,
Thank you for your comments.
On 19.09.2019 7:43, Michael Paquier wrote:
On Wed, Sep 18, 2019 at 03:46:20PM +0300, Alexey Kondratov wrote:
Currently in Postgres SET TABLESPACE always comes with [ NOWAIT ] option, so
I hope it worth adding this option here for convenience. Added in the new
version.
It seems to me that it would be good to keep the patch as simple as
possible for its first version, and split it into two if you would
like to add this new option instead of bundling both together. This
makes the review of one and the other more simple.
OK, it makes sense. I would also prefer first patch as simple as
possible, but adding this NOWAIT option required only a few dozens of
lines, so I just bundled everything together. Anyway, I will split
patches if we decide to keep [ SET TABLESPACE ... [NOWAIT] ] grammar.
Anyway, regarding
the grammar, is SET TABLESPACE really our best choice here? What
about:
- TABLESPACE = foo, in parenthesis only?
- Only using TABLESPACE, without SET at the end of the query?
SET is used in ALTER TABLE per the set of subqueries available there,
but that's not the case of REINDEX.
I like SET TABLESPACE grammar, because it already exists and used both
in ALTER TABLE and ALTER INDEX. Thus, if we once add 'ALTER INDEX
index_name REINDEX SET TABLESPACE' (as was proposed earlier in the
thread), then it will be consistent with 'REINDEX index_name SET
TABLESPACE'. If we use just plain TABLESPACE, then it may be misleading
in the following cases:
- REINDEX TABLE table_name TABLESPACE tablespace_name
- REINDEX (TABLESPACE = tablespace_name) TABLE table_name
since it may mean 'Reindex all indexes of table_name, that stored in the
tablespace_name', doesn't it?
However, I have rather limited experience with Postgres, so I doesn't
insist.
+-- check that all relations moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE
spcname='regress_tblspace')
+AND relname IN ('regress_tblspace_test_tbl_idx');
+ relname
+-------------------------------
+ regress_tblspace_test_tbl_idx
+(1 row)
Just to check one relation you could use \d with the relation (index
or table) name.
Yes, \d outputs tablespace name if it differs from pg_default, but it
shows other information in addition, which is not necessary here. Also
its output has more chances to be changed later, which may lead to the
failed tests. This query output is more or less stable and new relations
may be easily added to tests if we once add tablespace change to
CLUSTER/VACUUM FULL. I can change test to use \d, but not sure that it
would reduce test output length or will be helpful for a future tests
support.
- if (RELATION_IS_OTHER_TEMP(iRel))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex temporary tables of other
- sessions")))
I would keep the order of this operation in order with
CheckTableNotInUse().
Sure, I haven't noticed that reordered these operations, thanks.
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
