Re: patch : Allow toast tables to be moved to a different tablespace

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