From: | Julien Tachoires <julmon(at)gmail(dot)com> |
---|---|
To: | Andreas Karlsson <andreas(at)proxel(dot)se>, Alex Shulgin <ash(at)commandprompt(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: | 2015-03-03 15:14:38 |
Message-ID: | 54F5CFDE.20301@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
set_toast_tablespace_v0.12.patch | text/x-patch | 56.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Greg Stark | 2015-03-03 15:21:24 | Re: Idea: closing the loop for "pg_ctl reload" |
Previous Message | Greg Stark | 2015-03-03 15:08:33 | Re: Providing catalog view to pg_hba.conf file - Patch submission |