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

From: Andreas Karlsson <andreas(at)proxel(dot)se>
To: Alex Shulgin <ash(at)commandprompt(dot)com>, Julien Tachoires <julmon(at)gmail(dot)com>
Cc: Alvaro Herrera <alvherre(at)commandprompt(dot)com>, Jaime Casanova <jaime(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: patch : Allow toast tables to be moved to a different tablespace
Date: 2014-12-30 02:48:29
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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.

2014-08-09 01:35:50.008886776 +0200
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 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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Kevin Grittner 2014-12-30 03:00:43 Re: BUG #12330: ACID is broken for unique constraints
Previous Message Jim Nasby 2014-12-30 02:20:41 Re: nls and server log