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

Hi,

Sorry for the delay, I missed this thread.
Here is a new version of this patch considering Andreas' comments.

On 30/12/2014 03:48, Andreas Karlsson wrote:
> - 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)

I can't reproduce this issue.

> - 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...

Fixed.

> - "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.

Fixed.

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

Added.

> - 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.

If we do it that way, we should always show the tablespace even if it's
pg_default in any case to be consistent. I'm pretty sure that we don't
want that.

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

-1, we just want to be able to move TOAST heap, which is supposed to
contain a lot of data and we want to move on a different storage device
/ filesystem.

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

A notice is raised now in that case.

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

Support, documentation and regression tests added.

> - 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.

Done.

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

2 more tests added.

> - 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.

I don't understand those 2 last points ?

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

Fixed.

> - 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.

Fixed.

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

Fixed (I hope so).

> - Remove the parentheses around "(is_system_catalog)".

Fixed.

> - 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"?

Fixed.

> - Incorrect indentation for new code added last in ATExecSetTableSpace.

Fixed.

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

Fixed.

Thank you for your review.

--
Julien