Thread: Few cosmetic suggestions for commit 16828d5c (Fast Alter Table Add Column...)
Few cosmetic suggestions for commit 16828d5c (Fast Alter Table Add Column...)
From
Amit Kapila
Date:
Some assorted comments:
1.
- When a column is added with <literal>ADD COLUMN</literal>, all existing
- rows in the table are initialized with the column's default value
- (NULL if no <literal>DEFAULT</literal> clause is specified).
- If there is no <literal>DEFAULT</literal> clause, this is merely a metadata
- change and does not require any immediate update of the table's data;
- the added NULL values are supplied on readout, instead.
+ When a column is added with <literal>ADD COLUMN</literal> and a
+ non-volatile <literal>DEFAULT</literal> is specified, the default is
+ evaluated at the time of the statement and the result stored in the
+ table's metadata. That value will be used for the column for all existing
+ rows. If no <literal>DEFAULT</literal> is specified, NULL is used. In
+ neither case is a rewrite of the table required.
/the result stored/the result is stored
2.
+/*
+ * Structure used to represent value to be used when the attribute is not
+ * present at all in a tuple, i.e. when the column was created after the tuple
+ */
+
+typedef struct attrMissing
+{
+ bool ammissingPresent; /* true if non-NULL missing value exists */
+ Datum ammissing; /* value when attribute is missing */
+} AttrMissing;
+
a. Extra space (empty line) between structure and comments seems unnecessary.
b. The names of structure members seem little difficult to understand if you and or others also think so, can we rename them to something like (missingPresent, missingVal) or (attmissingPresent, attmissingVal) or something similar.
Patch to address 1 and 2a is attached. What do you think about 2b?
Attachment
Re: Few cosmetic suggestions for commit 16828d5c (Fast Alter TableAdd Column...)
From
Amit Kapila
Date:
On Sun, Jun 3, 2018 at 5:08 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Some assorted comments:
>
> 2.
> +/*
> + * Structure used to represent value to be used when the attribute is not
> + * present at all in a tuple, i.e. when the column was created after the
> tuple
> + */
> +
> +typedef struct attrMissing
> +{
> + bool ammissingPresent; /* true if non-NULL missing value exists
> */
> + Datum ammissing; /* value when attribute is missing */
> +} AttrMissing;
> +
>
> a. Extra space (empty line) between structure and comments seems
> unnecessary.
> b. The names of structure members seem little difficult to understand if you
> and or others also think so, can we rename them to something like
> (missingPresent, missingVal) or (attmissingPresent, attmissingVal) or
> something similar.
>
> Patch to address 1 and 2a is attached. What do you think about 2b?
>
As nobody responded, it seems that the variable naming pointed above
is okay, but in any case, I think we should fix cosmetic changes
proposed. I will commit the patch unless you or someone thinks that
we should change the name of the variable.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: Few cosmetic suggestions for commit 16828d5c (Fast Alter TableAdd Column...)
From
Alvaro Herrera
Date:
On 2018-Jun-14, Amit Kapila wrote:
> On Sun, Jun 3, 2018 at 5:08 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > 2.
> > +/*
> > + * Structure used to represent value to be used when the attribute is not
> > + * present at all in a tuple, i.e. when the column was created after the
> > tuple
> > + */
> > +
> > +typedef struct attrMissing
> > +{
> > + bool ammissingPresent; /* true if non-NULL missing value exists
> > */
> > + Datum ammissing; /* value when attribute is missing */
> > +} AttrMissing;
> > +
> As nobody responded, it seems that the variable naming pointed above
> is okay, but in any case, I think we should fix cosmetic changes
> proposed. I will commit the patch unless you or someone thinks that
> we should change the name of the variable.
We used to use prefixes for common struct members names to help
disambiguate across members that would otherwise have identical names in
different structs. Our convention was to use _ as a separator. This
convention has been partially lost, but seems we can use it to good
effect here, by renaming ammissingPresent to am_present and ammissing to
am_missing (I would go as far as suggesting am_default or am_substitute
or something like that).
BTW I think "the result stored" is correct English.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Few cosmetic suggestions for commit 16828d5c (Fast Alter TableAdd Column...)
From
Andrew Dunstan
Date:
Somehow I missed this thread ...
On 06/14/2018 02:01 PM, Alvaro Herrera wrote:
> On 2018-Jun-14, Amit Kapila wrote:
>
>> On Sun, Jun 3, 2018 at 5:08 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>> 2.
>>> +/*
>>> + * Structure used to represent value to be used when the attribute is not
>>> + * present at all in a tuple, i.e. when the column was created after the
>>> tuple
>>> + */
>>> +
>>> +typedef struct attrMissing
>>> +{
>>> + bool ammissingPresent; /* true if non-NULL missing value exists
>>> */
>>> + Datum ammissing; /* value when attribute is missing */
>>> +} AttrMissing;
>>> +
>> As nobody responded, it seems that the variable naming pointed above
>> is okay, but in any case, I think we should fix cosmetic changes
>> proposed. I will commit the patch unless you or someone thinks that
>> we should change the name of the variable.
> We used to use prefixes for common struct members names to help
> disambiguate across members that would otherwise have identical names in
> different structs. Our convention was to use _ as a separator. This
> convention has been partially lost, but seems we can use it to good
> effect here, by renaming ammissingPresent to am_present and ammissing to
> am_missing (I would go as far as suggesting am_default or am_substitute
> or something like that).
am_present and am_value perhaps? I'm not dogmatic about it.
> BTW I think "the result stored" is correct English.
>
Yes, it certainly is.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Few cosmetic suggestions for commit 16828d5c (Fast Alter TableAdd Column...)
From
Amit Kapila
Date:
On Fri, Jun 15, 2018 at 12:54 AM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
>
> On 06/14/2018 02:01 PM, Alvaro Herrera wrote:
>>
>> On 2018-Jun-14, Amit Kapila wrote:
>>
>>> On Sun, Jun 3, 2018 at 5:08 AM, Amit Kapila <amit.kapila16@gmail.com>
>>> wrote:
>>>>
>>>> 2.
>>>> +/*
>>>> + * Structure used to represent value to be used when the attribute is
>>>> not
>>>> + * present at all in a tuple, i.e. when the column was created after
>>>> the
>>>> tuple
>>>> + */
>>>> +
>>>> +typedef struct attrMissing
>>>> +{
>>>> + bool ammissingPresent; /* true if non-NULL missing value
>>>> exists
>>>> */
>>>> + Datum ammissing; /* value when attribute is missing */
>>>> +} AttrMissing;
>>>> +
..
>>
>> We used to use prefixes for common struct members names to help
>> disambiguate across members that would otherwise have identical names in
>> different structs. Our convention was to use _ as a separator. This
>> convention has been partially lost, but seems we can use it to good
>> effect here, by renaming ammissingPresent to am_present and ammissing to
>> am_missing (I would go as far as suggesting am_default or am_substitute
>> or something like that).
>
> am_present and am_value perhaps? I'm not dogmatic about it.
>
+1. Attached patch changed the names as per suggestion.
>
>
>
>> BTW I think "the result stored" is correct English.
>>
>
> Yes, it certainly is.
>
Okay.
How about attached?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachment
Re: Few cosmetic suggestions for commit 16828d5c (Fast Alter TableAdd Column...)
From
Amit Kapila
Date:
On Fri, Jun 15, 2018 at 9:46 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Fri, Jun 15, 2018 at 12:54 AM, Andrew Dunstan > <andrew.dunstan@2ndquadrant.com> wrote: >> >> On 06/14/2018 02:01 PM, Alvaro Herrera wrote: >>> >>> We used to use prefixes for common struct members names to help >>> disambiguate across members that would otherwise have identical names in >>> different structs. Our convention was to use _ as a separator. This >>> convention has been partially lost, but seems we can use it to good >>> effect here, by renaming ammissingPresent to am_present and ammissing to >>> am_missing (I would go as far as suggesting am_default or am_substitute >>> or something like that). >> >> am_present and am_value perhaps? I'm not dogmatic about it. >> > > +1. Attached patch changed the names as per suggestion. > >> >> >> >>> BTW I think "the result stored" is correct English. >>> >> >> Yes, it certainly is. >> > > Okay. > > How about attached? > Andrew, Alvaro, do you think we can go ahead with above naming suggestions or do we want to brainstorm more on it? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Few cosmetic suggestions for commit 16828d5c (Fast Alter TableAdd Column...)
From
Alvaro Herrera
Date:
On 2018-Jun-26, Amit Kapila wrote: > Andrew, Alvaro, do you think we can go ahead with above naming > suggestions or do we want to brainstorm more on it? Looks good to me in a quick skim. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Few cosmetic suggestions for commit 16828d5c (Fast Alter TableAdd Column...)
From
Amit Kapila
Date:
On Tue, Jun 26, 2018 at 7:11 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2018-Jun-26, Amit Kapila wrote: > >> Andrew, Alvaro, do you think we can go ahead with above naming >> suggestions or do we want to brainstorm more on it? > > Looks good to me in a quick skim. > Thanks, pushed! -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com