Re: patch : Allow toast tables to be moved to a different tablespace - Mailing list pgsql-hackers

Hi,

Here is my review of the feature.

- I have not worked enough with tablespaces to see if this patch would 
be useful. There was some uncertainty about this upstream.

- Would it not be enough to simply allow running ALTER TABLE SET 
TABLESPACE on the TOAST tables? Or is manually modifying those too ugly?

- I like that it adds tab completion for the new commands.

- The feature seems to work as described, other than the ALTER INDEX 
issue noted below and that it broke pg_dump. The broken pg_dump means I 
have not tested how this changes database dumps.

- A test fails in create_view.out. I looked some into it and did not see 
how this could happen.
    *** 
/home/andreas/dev/postgresql/src/test/regress/expected/create_view.out 
2014-08-09 01:35:50.008886776 +0200    --- 
/home/andreas/dev/postgresql/src/test/regress/results/create_view.out 
2014-12-30 00:41:17.966525339 +0100    ***************    *** 283,289 ***        relname   | relkind |
reloptions     ------------+---------+--------------------------       mysecview1 | v       |    !  mysecview2 | v
|       mysecview3 | v       | {security_barrier=true}       mysecview4 | v       | {security_barrier=false}      (4
rows)   --- 283,289 ----        relname   | relkind |        reloptions
------------+---------+--------------------------      mysecview1 | v       |    !  mysecview2 | v       |
{security_barrier=true}      mysecview3 | v       | {security_barrier=true}       mysecview4 | v       |
{security_barrier=false}     (4 rows)
 

- pg_dump is broken
    pg_dump: [archiver (db)] query failed: ERROR:  syntax error at or 
near "("    LINE 1: ...nest(tc.reloptions) x), ', ') AS toast_reloptions 
(SELECT sp...

- "ALTER INDEX foo_idx SET TABLE TABLESPACE ..." should not be allowed, 
currently it changes the tablespace of the index. I do not think "ALTER 
INDEX foo_idx SET (TABLE|TOAST) TABLESPACE ..." should even be allowed 
in the grammar.

- You should add a link to 
http://www.postgresql.org/docs/current/interactive/storage-toast.html to 
the manual page of ALTER TABLE.

- I do not like how \d handles the toast tablespace. Having TOAST in 
pg_default and the table in another space looks the same as if there was 
no TOAST table at all. I think we should always print both tablespaces 
if either of them are not pg_default.

- Would it be interesting to add syntax for moving the toast index to a 
separate tablespace?

- There is no warning if you set the toast table space of a table 
lacking toast. I think there should be one.

- No support for materialized views as pointed out by Alex.

- I think the code would be cleaner if ATPrepSetTableSpace and 
ATPrepSetToastTableSpace were merged into one function or split into 
two, one setting the toast and one setting the tablespace of the table 
and one setting it on the TOAST table.

- I think a couple of tests for the error cases would be nice.

= Minor style issue

- Missing periods on the ALTER TABLE manual page after "See also CREATE 
TABLESPACE" (in two places).

- Missing period last in the new paragraph added to the storage manual page.

- Double spaces in src/backend/catalog/toasting.c after "if (new_toast".

- The comment "and if this is not a TOAST re-creation case (through 
CLUSTER)." incorrectly implies that CLUSTER is the only case where the 
TOAST table is recreated.

- The local variables ToastTableSpace and ToastRel do not follow the 
naming conventions.

- Remove the parentheses around "(is_system_catalog)".

- Why was the "Save info for Phase 3 to do the real work" comment 
changed to "XXX: Save info for Phase 3 to do the real work"?

- Incorrect indentation for new code added last in ATExecSetTableSpace.

- The patch adds commented out code in src/include/commands/tablecmds.h.

-- 
Andreas Karlsson