Date: Fri, 17 Jun 2022 11:45:35 +0900 (JST)
Message-Id: <20220617.114535.657035567967998560.horikyota.ntt@gmail.com>
To: aleksander@timescale.com
Cc: boekewurm+postgres@gmail.com, pgsql-hackers@postgresql.org,
 teodor@sigaev.ru, wjzeng2012@gmail.com
Subject: Re: CREATE TABLE ( .. STORAGE ..)
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: 
 <CAJ7c6TNntQrB2NsD7zWeFosNeJkqMtjX1th6peKGP0A+YR2bsQ@mail.gmail.com>
References: 
 <CAJ7c6TO_QBjMfLkDRgi2ceBZkvMYG_9GRvzboitBfN3PoJoqxQ@mail.gmail.com>
	<CAEze2WiqY3b_FKSUGV8mDMMu+Neb37aCsvMUvKoYJmfNCs+pqg@mail.gmail.com>
	<CAJ7c6TNntQrB2NsD7zWeFosNeJkqMtjX1th6peKGP0A+YR2bsQ@mail.gmail.com>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20220617.114535.657035567967998560.horikyota.ntt%40gmail.com>
Precedence: bulk

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