Feedback-ID: i131946ab:Fastmail
Message-ID: <46546823-5b6c-4aeb-43b8-d4db48d4a606@enterprisedb.com>
Date: Wed, 13 Jul 2022 12:28:30 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.11.0
Subject: Re: CREATE TABLE ( .. STORAGE ..)
Content-Language: en-US
To: Aleksander Alekseev <aleksander@timescale.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
 Teodor Sigaev <teodor@sigaev.ru>, wenjing zeng <wjzeng2012@gmail.com>
References: <de83407a-ae3d-a8e1-a788-920eb334f25b@sigaev.ru>
 <ECF2E7F0-D237-44EA-971B-CB163B4EB457@gmail.com>
 <ea723b92-d87a-aeef-3467-56453a36921d@sigaev.ru>
 <CAEze2WhWef-vemQzZJxQ3SuYANKT32Y1wyNB=NhjP_vfVgY2jQ@mail.gmail.com>
 <7166e46b-e718-3f3f-c165-ab321cc5dfaa@enterprisedb.com>
 <CAJ7c6TP5sdpRLbzEvu5yDAw2LRB8OXZFo=ENjhq=bouBzSxh9A@mail.gmail.com>
 <CAJ7c6TM-O3=duY8Zk3S4Joi6s=wuLfePFTFuBgNu65bVZY+PdQ@mail.gmail.com>
 <CAJ7c6TOn1BrFAxWxK7zMfsGyGNqg3YWsou9DpvMp1=JX80909A@mail.gmail.com>
 <d3cb9b64-0665-4e72-329f-b4c10f6faff3@enterprisedb.com>
 <CAJ7c6TPsg6iUGZGaEGKi5vdG6VpEcLyZMbXTUQFnCNdi+Kd00Q@mail.gmail.com>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: 
 <CAJ7c6TPsg6iUGZGaEGKi5vdG6VpEcLyZMbXTUQFnCNdi+Kd00Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/46546823-5b6c-4aeb-43b8-d4db48d4a606%40enterprisedb.com>
Precedence: bulk

On 12.07.22 12:10, Aleksander Alekseev wrote:
> Hi Peter,
> 
>> The "safety check: do not allow toasted storage modes unless column
>> datatype is TOAST-aware" could be moved into GetAttributeStorage(), so
>> it doesn't have to be repeated.  (Note that GetAttributeCompression()
>> does similar checking.)
> 
> Good point. Fixed.
> 
>> ATExecSetStorage() currently doesn't do any such check, and your patch
>> isn't adding one.  Is there a reason for that?
> 
> ATExecSetStorage() does this, but the check is a bit below [1]. In v7
> I moved the check to GetAttributeStorage() as well.
> 
> [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/tablecmds.c#l8312

Committed.

I thought the removal of the documentation details of SET COMPRESSION 
and SET STORAGE from the ALTER TABLE ref page was a bit excessive, since 
that material actually contained useful information about what happens 
when you change compression or storage on a table with existing data. 
So I left that in.  Maybe there is room to deduplicate that material a 
bit, but it would need to be more fine-grained than just removing one 
side of it.