Thread: CREATE TABLE ( .. STORAGE ..)
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
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>
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
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
Attachment
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
Attachment
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
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
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]. 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
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