Message-ID: <54FE2C23.1080604@proxel.se>
Date: Tue, 10 Mar 2015 00:26:27 +0100
From: Andreas Karlsson <andreas@proxel.se>
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
 rv:31.0) Gecko/20100101 Icedove/31.4.0
MIME-Version: 1.0
To: Julien Tachoires <julmon@gmail.com>, Alex Shulgin <ash@commandprompt.com>
CC: Alvaro Herrera <alvherre@commandprompt.com>,
 Jaime Casanova <jaime@2ndquadrant.com>,
 Robert Haas <robertmhaas@gmail.com>,
 Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: patch : Allow toast tables to be moved to a different tablespace
References: <4E8F1666.2060409@gmail.com>
	<CAJKUy5ix3xb8E_+hLfPvz_n+oYTU36-aMfOVNpC5=4PAf-o_jg@mail.gmail.com>
	<CAFEQCbGPrRpV1+bQSQ-v0i-md18rvx6jpyR=TEYt+9sPCfTMYw@mail.gmail.com>
	<CAJKUy5iMp-NE=24WSNQt5Npp0SLkPTDkqVz9C8D9y=pdK=eFoQ@mail.gmail.com>
	<CAFEQCbH756DyyAPQ1ykh3+b+kE1-EhWRww1WO_x5v38C-uLnUg@mail.gmail.com>
	<CAJKUy5itRAtv7A3y7nVKmKe8SZdTU5Q-OsPr4d3ySYqoHkzK1g@mail.gmail.com>
	<CAFEQCbGmVoeSVyeh8ZCJKpN+NKH2rgvZq1rMrKjZ+YE7RpM8zw@mail.gmail.com>
	<CAJKUy5gYJPR0ETVQQvSf2-mDfdsJU-7XSUVDFN_o4+ZaZtyATg@mail.gmail.com>
	<CAFEQCbGEjp6j6-XD4C-vDaAyRHh6aMbH__BWB3kBuT505LXQyw@mail.gmail.com>
	<CA+TgmoaJOK6yKMZw8sc5HzRD8hb0bzCV0U4JuNc3XdACmfQCYw@mail.gmail.com>
	<CAFEQCbESRGXLE4+tRAnazr8QmBJzfYPwc6GP1Daq0quyuyEpZQ@mail.gmail.com>
	<1323956011-sup-9589@alvh.no-ip.org>
	<CAFEQCbEq07OopgE5xFYv2Q3eMq45hRSJkjCBO+kvpJq9NEVhow@mail.gmail.com>
 <87r3wn5lhq.fsf@commandprompt.com> <54A2127D.5020907@proxel.se>
 <54F5CFDE.20301@gmail.com>
In-Reply-To: <54F5CFDE.20301@gmail.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

On 03/03/2015 04:14 PM, Julien Tachoires wrote:
> 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.
>>
>>[...]
>
> I can't reproduce this issue.

Neither can I anymore.

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

I still get a failing pg_dump test though when running make check-world. 
And it does not work when run manually either.

+ pg_dumpall -f 
/home/andreas/dev/postgresql/contrib/pg_upgrade/tmp_check/dump1.sql
pg_dump: column number -1 is out of range 0..28
cannot duplicate null pointer (internal error)
pg_dumpall: pg_dump failed on database "postgres", exiting
+ pg_dumpall1_status=1
+ [ /home/andreas/dev/postgresql != /home/andreas/dev/postgresql ]
+ 
/home/andreas/dev/postgresql/contrib/pg_upgrade/tmp_check/install//home/andreas/dev/postgresql-inst/bin/pg_ctl 
-m fast stop
waiting for server to shut down.... done
server stopped
+ [ -n  ]
+ [ -n  ]
+ [ -n 1 ]
+ echo pg_dumpall of pre-upgrade database cluster failed
pg_dumpall of pre-upgrade database cluster failed
+ exit 1
+ rm -rf /tmp/pg_upgrade_check-5A3wsI

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

I think we will have to agree to disagree here. I think it should be 
obvious when the toast table is in the default tablespace for tables 
outside it.

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

I think we should allow moving the indexes for consistency. With this 
patch we can move everything except for TOAST indexes.

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

Excellent, also add a test case for this.

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


I mean that "TOAST table can be moved to a different tablespace with 
<command>ALTER TABLE SET TOAST TABLESPACE</>" should be changed to 
"TOAST table can be moved to a different tablespace with <command>ALTER 
TABLE SET TOAST TABLESPACE</>." since a sentece should always end in ".".

= New comments

- The patch does not apply cleanly anymore, I had to solve to minor 
conflicts.

- Rewrap the documentation for SET TABLESPACE.

- You have added a tab in alter_table.sgml.

- Merge "case AT_SetTableTableSpace:" and "case AT_SetToastTableSpace:" 
where they both do the same thing, i.e. nothing.

- Should it not be foo_mv in the query from the test suite below?

SELECT spcname FROM pg_class c JOIN pg_class d ON 
(c.reltoastrelid=d.oid) JOIN pg_tablespace ON (d.reltablespace = 
pg_tablespace.oid) WHERE c.relname = 'foo2';

-- 
Andreas Karlsson


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers