CREATE TABLE ( .. STORAGE ..)

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

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/&lt;create_table_storage-v1.patch.gz&gt;

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

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

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

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

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

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

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

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.

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

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

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

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?

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]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/tablecmds.c#l8312. 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

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.

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