Thread: CREATE TABLE ( .. STORAGE ..)

CREATE TABLE ( .. STORAGE ..)

From
Teodor Sigaev
Date:
Hi!

Working on pluggable toaster (mostly, for JSONB improvements, see links 
below) I had found that STORAGE attribute on column is impossible to set 
  in CREATE TABLE command but COMPRESS option is possible. It looks 
unreasonable. Suggested patch implements this possibility.

[1] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgconfnyc-2021.pdf
[2] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgvision-2021.pdf
[3] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgconfonline-2021.pdf
[4] http://www.sai.msu.su/~megera/postgres/talks/bytea-pgconfonline-2021.pdf

PS I will propose pluggable toaster patch a bit later
-- 
Teodor Sigaev                      E-mail: teodor@sigaev.ru
                                       WWW: http://www.sigaev.ru/
Attachment

Re: CREATE TABLE ( .. STORAGE ..)

From
wenjing zeng
Date:
HI

For patch create_table_storage-v1

1
+ALTER opt_column ColId SET STORAGE name

+opt_column_storage:
+            STORAGE    ColId                            { $$ = $2; }

Are they both set to name or ColId? Although they are the same.

2 For ColumnDef new member storage_name, did you miss the function _copyColumnDef()  _equalColumnDef()?


Regards
Wenjing


> 2021年12月27日 15:51,Teodor Sigaev <teodor@sigaev.ru> 写道:
>
> Hi!
>
> Working on pluggable toaster (mostly, for JSONB improvements, see links below) I had found that STORAGE attribute on
columnis impossible to set  in CREATE TABLE command but COMPRESS option is possible. It looks unreasonable. Suggested
patchimplements this possibility. 
>
> [1] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgconfnyc-2021.pdf
> [2] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgvision-2021.pdf
> [3] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgconfonline-2021.pdf
> [4] http://www.sai.msu.su/~megera/postgres/talks/bytea-pgconfonline-2021.pdf
>
> PS I will propose pluggable toaster patch a bit later
> --
> Teodor Sigaev                      E-mail: teodor@sigaev.ru
>                                      WWW: http://www.sigaev.ru/<create_table_storage-v1.patch.gz>




Re: CREATE TABLE ( .. STORAGE ..)

From
Teodor Sigaev
Date:
Hi!

> Are they both set to name or ColId? Although they are the same.
> 

Thank you, fixed, that was just an oversight.

> 2 For ColumnDef new member storage_name, did you miss the function _copyColumnDef()  _equalColumnDef()?

Thank you, fixed

> 
> 
> Regards
> Wenjing
> 
> 
>> 2021年12月27日 15:51,Teodor Sigaev <teodor@sigaev.ru> 写道:
>>
>> Hi!
>>
>> Working on pluggable toaster (mostly, for JSONB improvements, see links below) I had found that STORAGE attribute on
columnis impossible to set  in CREATE TABLE command but COMPRESS option is possible. It looks unreasonable. Suggested
patchimplements this possibility.
 
>>
>> [1] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgconfnyc-2021.pdf
>> [2] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgvision-2021.pdf
>> [3] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgconfonline-2021.pdf
>> [4] http://www.sai.msu.su/~megera/postgres/talks/bytea-pgconfonline-2021.pdf
>>
>> PS I will propose pluggable toaster patch a bit later
>> -- 
>> Teodor Sigaev                      E-mail: teodor@sigaev.ru
>>                                       WWW: http://www.sigaev.ru/<create_table_storage-v1.patch.gz>
> 

-- 
Teodor Sigaev                      E-mail: teodor@sigaev.ru
                                       WWW: http://www.sigaev.ru/
Attachment

Re: CREATE TABLE ( .. STORAGE ..)

From
Matthias van de Meent
Date:
On Wed, 2 Feb 2022 at 11:13, Teodor Sigaev <teodor@sigaev.ru> wrote:
>
> Hi!
>
> > Are they both set to name or ColId? Although they are the same.
> >
>
> Thank you, fixed, that was just an oversight.
>
> > 2 For ColumnDef new member storage_name, did you miss the function _copyColumnDef()  _equalColumnDef()?
>
> Thank you, fixed

I noticed this and tried it out after needing it in a different
thread, so this is quite the useful addition.

I see that COMPRESSION and STORAGE now are handled slightly
differently in the grammar. Maybe we could standardize that a bit
more; so that we have only one `STORAGE [kind]` definition in the
grammar?

As I'm new to the grammar files; would you know the difference between
`name` and `ColId`, and why you would change from one to the other in
ALTER COLUMN STORAGE?

Thanks!

-Matthias



Re: CREATE TABLE ( .. STORAGE ..)

From
Aleksander Alekseev
Date:
Hi hackers,

I noticed that cfbot is not entirely happy with the patch, so I rebased it.

> I see that COMPRESSION and STORAGE now are handled slightly
> differently in the grammar. Maybe we could standardize that a bit
> more; so that we have only one `STORAGE [kind]` definition in the
> grammar?
>
> As I'm new to the grammar files; would you know the difference between
> `name` and `ColId`, and why you would change from one to the other in
> ALTER COLUMN STORAGE?

Good point, Matthias. I addressed this in 0002. Does it look better now?

-- 
Best regards,
Aleksander Alekseev

Attachment

Re: CREATE TABLE ( .. STORAGE ..)

From
Matthias van de Meent
Date:
On Wed, 15 Jun 2022 at 16:51, Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi hackers,
>
> I noticed that cfbot is not entirely happy with the patch, so I rebased it.
>
> > I see that COMPRESSION and STORAGE now are handled slightly
> > differently in the grammar. Maybe we could standardize that a bit
> > more; so that we have only one `STORAGE [kind]` definition in the
> > grammar?
> >
> > As I'm new to the grammar files; would you know the difference between
> > `name` and `ColId`, and why you would change from one to the other in
> > ALTER COLUMN STORAGE?
>
> Good point, Matthias. I addressed this in 0002. Does it look better now?

When updating a patchset generally we try to keep the patches
self-contained, and update patches as opposed to adding incremental
patches to the set.

Apart from this comment on the format of the patch, the result seems solid.

- Matthias



Re: CREATE TABLE ( .. STORAGE ..)

From
Aleksander Alekseev
Date:
Hi Matthias,

> Apart from this comment on the format of the patch, the result seems solid.

Many thanks.

> When updating a patchset generally we try to keep the patches
> self-contained, and update patches as opposed to adding incremental
> patches to the set.

My reasoning was to separate my changes from the ones originally
proposed by Teodor. After doing `git am` locally a reviewer can see
them separately, or together with `git diff origin/master`, whatever
he or she prefers. The committer can choose between committing two
patches ony by one, or rebasing them to a single commit.

I will avoid the "patch for the patch" practice from now on. Sorry for
the inconvenience.

-- 
Best regards,
Aleksander Alekseev



Re: CREATE TABLE ( .. STORAGE ..)

From
Kyotaro Horiguchi
Date:
Thanks! I have been annoyed sometimes by the lack of this feature.

At Thu, 16 Jun 2022 16:40:55 +0300, Aleksander Alekseev <aleksander@timescale.com> wrote in 
> Hi Matthias,
> 
> > Apart from this comment on the format of the patch, the result seems solid.
> 
> Many thanks.
> 
> > When updating a patchset generally we try to keep the patches
> > self-contained, and update patches as opposed to adding incremental
> > patches to the set.
> 
> My reasoning was to separate my changes from the ones originally
> proposed by Teodor. After doing `git am` locally a reviewer can see
> them separately, or together with `git diff origin/master`, whatever
> he or she prefers. The committer can choose between committing two
> patches ony by one, or rebasing them to a single commit.
> 
> I will avoid the "patch for the patch" practice from now on. Sorry for
> the inconvenience.

0001 contains one tranling whitespace error. (which "git diff --check"
can detect)

The modified doc line gets too long to me. Maybe we should wrap it as
done in other lines of the same page.

I think we should avoid descriptions dead-copied between pages. In
this case, I think we should remove the duplicate part of the
description of ALTER TABLE then replace with something like "See
CREATE TABLE for details".

As the result of copying-in the description, SET-STORAGE and
COMPRESSION in the page of CREATE-TABLE use different articles in the
same context.

> SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
>   This form sets the storage mode for *a* column.

> COMPRESSION compression_method
>   The COMPRESSION clause sets the compression method for *the* column.

FWIW I feel "the" is better here, but anyway we should unify them.

  
 static char GetAttributeCompression(Oid atttypid, char *compression);
+static char    GetAttributeStorage(const char *storagemode);

The whitespace after "char" is TAB which differs from SPC used in
neigbouring lines.

In the grammar, COMPRESSION uses ColId, but STORAGE uses name. It
seems to me the STORAGE is correct here, though.. (So, do we need to
fix COMPRESSION syntax?)


This adds support for "ADD COLUMN SET STORAGE" but it is not described
in the doc.  COMPRESSION is not described, too.  Shouldn't we add the
both this time?  Or the fix for COMPRESSION can be a different patch.

Now that we have three column options COMPRESSION, COLLATE and STORGE
which has the strict order in syntax.  I wonder it can be relaxed but
it might be too much..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: CREATE TABLE ( .. STORAGE ..)

From
Peter Eisentraut
Date:
On 15.06.22 16:51, Aleksander Alekseev wrote:
> I noticed that cfbot is not entirely happy with the patch, so I rebased it.
> 
>> I see that COMPRESSION and STORAGE now are handled slightly
>> differently in the grammar. Maybe we could standardize that a bit
>> more; so that we have only one `STORAGE [kind]` definition in the
>> grammar?
>>
>> As I'm new to the grammar files; would you know the difference between
>> `name` and `ColId`, and why you would change from one to the other in
>> ALTER COLUMN STORAGE?
> 
> Good point, Matthias. I addressed this in 0002. Does it look better now?

In your patch, the documentation for CREATE TABLE says "SET STORAGE", 
but the actual syntax does not contain "SET".




Re: CREATE TABLE ( .. STORAGE ..)

From
Peter Eisentraut
Date:
On 29.03.22 22:28, Matthias van de Meent wrote:
> As I'm new to the grammar files; would you know the difference between
> `name` and `ColId`, and why you would change from one to the other in
> ALTER COLUMN STORAGE?

The grammar says

name:       ColId                                   { $$ = $1; };

so it doesn't matter technically.

It seems we are using "name" mostly for names of objects, so I wouldn't 
use it here for storage or compression types.



Re: CREATE TABLE ( .. STORAGE ..)

From
Aleksander Alekseev
Date:
Hi hackers,

Many thanks for the review!

Here is a patch updated according to all the recent feedback, except
for two suggestions:

> This adds support for "ADD COLUMN SET STORAGE" but it is not described
> in the doc.  COMPRESSION is not described, too.  Shouldn't we add the
> both this time?  Or the fix for COMPRESSION can be a different patch.

The documentation for ADD COLUMN simply says:

```
 <para>
 This form adds a new column to the table, using the same syntax as
 <link linkend="sql-createtable"><command>CREATE
TABLE</command></link>. If <literal>IF NOT EXISTS</literal>
 is specified and a column already exists with this name,
 no error is thrown.
 </para>
```

I suggest keeping a reference to CREATE TABLE, similarly as it was
done for ALTER COLUMN.

> Now that we have three column options COMPRESSION, COLLATE and STORGE
> which has the strict order in syntax.  I wonder it can be relaxed but
> it might be too much..

Agree, this could be a bit too much for this particular discussion.
Although this shouldn't be a difficult change, and I agree that this
should be useful, personally I don't feel enthusiastic enough to
deliver it right now. I suggest we address this later.

-- 
Best regards,
Aleksander Alekseev

Attachment

Re: CREATE TABLE ( .. STORAGE ..)

From
Aleksander Alekseev
Date:
Hi hackers,

> Here is a patch updated according to all the recent feedback, except
> for two suggestions:

In v4 I forgot to list possible arguments for STORAGE in
alter_table.sgml, similarly as it is done for other subcommands. Here
is a corrected patch.

- <literal>SET STORAGE</literal>
+ <literal>SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal>

-- 
Best regards,
Aleksander Alekseev

Attachment

Re: CREATE TABLE ( .. STORAGE ..)

From
Aleksander Alekseev
Date:
Hi hackers,

> > Here is a patch updated according to all the recent feedback, except
> > for two suggestions:
>
> In v4 I forgot to list possible arguments for STORAGE in
> alter_table.sgml, similarly as it is done for other subcommands. Here
> is a corrected patch.

Here is the rebased patch.

-- 
Best regards,
Aleksander Alekseev

Attachment

Re: CREATE TABLE ( .. STORAGE ..)

From
Peter Eisentraut
Date:
On 11.07.22 11:27, Aleksander Alekseev wrote:
>>> Here is a patch updated according to all the recent feedback, except
>>> for two suggestions:
>>
>> In v4 I forgot to list possible arguments for STORAGE in
>> alter_table.sgml, similarly as it is done for other subcommands. Here
>> is a corrected patch.
> 
> Here is the rebased patch.

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

ATExecSetStorage() currently doesn't do any such check, and your patch 
isn't adding one.  Is there a reason for that?



Re: CREATE TABLE ( .. STORAGE ..)

From
Aleksander Alekseev
Date:
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

-- 
Best regards,
Aleksander Alekseev

Attachment

Re: CREATE TABLE ( .. STORAGE ..)

From
Peter Eisentraut
Date:
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.




Re: CREATE TABLE ( .. STORAGE ..)

From
Aleksander Alekseev
Date:
Hi,

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

Thanks, Peter!

-- 
Best regards,
Aleksander Alekseev