Horton

Thread

Commits

  1. Allow specifying STORAGE attribute for a new table

  1. CREATE TABLE ( .. STORAGE ..)

    Teodor Sigaev <teodor@sigaev.ru> — 2021-12-27T07:51:42Z

    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/

  2. Re: CREATE TABLE ( .. STORAGE ..)

    Wenjing Zeng <wjzeng2012@gmail.com> — 2022-01-21T07:40:57Z

    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 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/<create_table_storage-v1.patch.gz>

  3. Re: CREATE TABLE ( .. STORAGE ..)

    Teodor Sigaev <teodor@sigaev.ru> — 2022-02-02T10:13:29Z

    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 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/<create_table_storage-v1.patch.gz>
> 

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

  4. Re: CREATE TABLE ( .. STORAGE ..)

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2022-03-29T20:28:45Z

    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

  5. Re: CREATE TABLE ( .. STORAGE ..)

    Aleksander Alekseev <aleksander@timescale.com> — 2022-06-15T14:51:06Z

    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

  6. Re: CREATE TABLE ( .. STORAGE ..)

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2022-06-16T09:17:01Z

    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

  7. Re: CREATE TABLE ( .. STORAGE ..)

    Aleksander Alekseev <aleksander@timescale.com> — 2022-06-16T13:40:55Z

    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

  8. Re: CREATE TABLE ( .. STORAGE ..)

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-06-17T02:45:35Z

    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

  9. Re: CREATE TABLE ( .. STORAGE ..)

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-06-22T13:25:48Z

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

  10. Re: CREATE TABLE ( .. STORAGE ..)

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-06-22T13:29:24Z

    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.

  11. Re: CREATE TABLE ( .. STORAGE ..)

    Aleksander Alekseev <aleksander@timescale.com> — 2022-06-24T11:44:07Z

    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

  12. Re: CREATE TABLE ( .. STORAGE ..)

    Aleksander Alekseev <aleksander@timescale.com> — 2022-06-24T12:30:31Z

    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

  13. Re: CREATE TABLE ( .. STORAGE ..)

    Aleksander Alekseev <aleksander@timescale.com> — 2022-07-11T09:27:50Z

    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

  14. Re: CREATE TABLE ( .. STORAGE ..)

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-11T13:17:13Z

    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?

  15. Re: CREATE TABLE ( .. STORAGE ..)

    Aleksander Alekseev <aleksander@timescale.com> — 2022-07-12T10:10:39Z

    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

  16. Re: CREATE TABLE ( .. STORAGE ..)

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-13T10:28:30Z

    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.

  17. Re: CREATE TABLE ( .. STORAGE ..)

    Aleksander Alekseev <aleksander@timescale.com> — 2022-07-13T10:38:05Z

    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