Few cosmetic suggestions for commit 16828d5c (Fast Alter Table Add Column...)
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?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
cosmetic_changes_fast_addcol_v1.patchapplication/octet-stream; name=cosmetic_changes_fast_addcol_v1.patchDownload
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1cce00eaf92..1024f596fdc 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1187,7 +1187,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<para>
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
+ evaluated at the time of the statement and the result is 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.
diff --git a/src/include/access/tupdesc_details.h b/src/include/access/tupdesc_details.h
index 741e996b3cc..49b300996f3 100644
--- a/src/include/access/tupdesc_details.h
+++ b/src/include/access/tupdesc_details.h
@@ -19,7 +19,6 @@
* 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 */
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
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
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
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
Attachments:
cosmetic_changes_fast_addcol_v2.patchapplication/octet-stream; name=cosmetic_changes_fast_addcol_v2.patchDownload
From 2761fe07cf50c4ed529c4f47966643e864a0399c Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 15 Jun 2018 09:04:20 +0530
Subject: [PATCH] Cosmetic improvements for faster column addition.
Changed the name of few structure members for the sake of clarity and
removed spurious whitespace.
Discussion: https://postgr.es/m/CAA4eK1K2znsFpC+NQ9A4vxT4uDxADN4RmvHX0L6Y=aHVo9gB4Q@mail.gmail.com
---
src/backend/access/common/heaptuple.c | 21 ++++++++++-----------
src/backend/access/common/tupdesc.c | 18 +++++++++---------
src/backend/utils/cache/relcache.c | 10 +++++-----
src/include/access/tupdesc_details.h | 5 ++---
4 files changed, 26 insertions(+), 28 deletions(-)
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index 1041721..2ec7e6a 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -100,10 +100,10 @@ getmissingattr(TupleDesc tupleDesc,
attrmiss = tupleDesc->constr->missing + (attnum - 1);
- if (attrmiss->ammissingPresent)
+ if (attrmiss->am_present)
{
*isnull = false;
- return attrmiss->ammissing;
+ return attrmiss->am_value;
}
}
@@ -142,9 +142,8 @@ slot_getmissingattrs(TupleTableSlot *slot, int startAttNum, int lastAttNum)
missattnum < lastAttNum;
missattnum++)
{
- slot->tts_values[missattnum] = attrmiss[missattnum].ammissing;
- slot->tts_isnull[missattnum] =
- !attrmiss[missattnum].ammissingPresent;
+ slot->tts_values[missattnum] = attrmiss[missattnum].am_value;
+ slot->tts_isnull[missattnum] = !attrmiss[missattnum].am_present;
}
}
}
@@ -822,7 +821,7 @@ expand_tuple(HeapTuple *targetHeapTuple,
firstmissingnum < natts;
firstmissingnum++)
{
- if (attrmiss[firstmissingnum].ammissingPresent)
+ if (attrmiss[firstmissingnum].am_present)
break;
}
@@ -844,18 +843,18 @@ expand_tuple(HeapTuple *targetHeapTuple,
attnum < natts;
attnum++)
{
- if (attrmiss[attnum].ammissingPresent)
+ if (attrmiss[attnum].am_present)
{
Form_pg_attribute att = TupleDescAttr(tupleDesc, attnum);
targetDataLen = att_align_datum(targetDataLen,
att->attalign,
att->attlen,
- attrmiss[attnum].ammissing);
+ attrmiss[attnum].am_value);
targetDataLen = att_addlength_pointer(targetDataLen,
att->attlen,
- attrmiss[attnum].ammissing);
+ attrmiss[attnum].am_value);
}
else
{
@@ -981,14 +980,14 @@ expand_tuple(HeapTuple *targetHeapTuple,
Form_pg_attribute attr = TupleDescAttr(tupleDesc, attnum);
- if (attrmiss && attrmiss[attnum].ammissingPresent)
+ if (attrmiss && attrmiss[attnum].am_present)
{
fill_val(attr,
nullBits ? &nullBits : NULL,
&bitMask,
&targetData,
infoMask,
- attrmiss[attnum].ammissing,
+ attrmiss[attnum].am_value,
false);
}
else
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 2658399..b0434b4 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -185,13 +185,13 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
memcpy(cpy->missing, constr->missing, tupdesc->natts * sizeof(AttrMissing));
for (i = tupdesc->natts - 1; i >= 0; i--)
{
- if (constr->missing[i].ammissingPresent)
+ if (constr->missing[i].am_present)
{
Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
- cpy->missing[i].ammissing = datumCopy(constr->missing[i].ammissing,
- attr->attbyval,
- attr->attlen);
+ cpy->missing[i].am_value = datumCopy(constr->missing[i].am_value,
+ attr->attbyval,
+ attr->attlen);
}
}
}
@@ -337,9 +337,9 @@ FreeTupleDesc(TupleDesc tupdesc)
for (i = tupdesc->natts - 1; i >= 0; i--)
{
- if (attrmiss[i].ammissingPresent
+ if (attrmiss[i].am_present
&& !TupleDescAttr(tupdesc, i)->attbyval)
- pfree(DatumGetPointer(attrmiss[i].ammissing));
+ pfree(DatumGetPointer(attrmiss[i].am_value));
}
pfree(attrmiss);
}
@@ -512,13 +512,13 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
AttrMissing *missval1 = constr1->missing + i;
AttrMissing *missval2 = constr2->missing + i;
- if (missval1->ammissingPresent != missval2->ammissingPresent)
+ if (missval1->am_present != missval2->am_present)
return false;
- if (missval1->ammissingPresent)
+ if (missval1->am_present)
{
Form_pg_attribute missatt1 = TupleDescAttr(tupdesc1, i);
- if (!datumIsEqual(missval1->ammissing, missval2->ammissing,
+ if (!datumIsEqual(missval1->am_value, missval2->am_value,
missatt1->attbyval, missatt1->attlen))
return false;
}
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index d85dc92..6125421 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -614,18 +614,18 @@ RelationBuildTupleDesc(Relation relation)
if (attp->attbyval)
{
/* for copy by val just copy the datum direct */
- attrmiss[attnum - 1].ammissing = missval;
+ attrmiss[attnum - 1].am_value = missval;
}
else
{
/* otherwise copy in the correct context */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
- attrmiss[attnum - 1].ammissing = datumCopy(missval,
- attp->attbyval,
- attp->attlen);
+ attrmiss[attnum - 1].am_value = datumCopy(missval,
+ attp->attbyval,
+ attp->attlen);
MemoryContextSwitchTo(oldcxt);
}
- attrmiss[attnum - 1].ammissingPresent = true;
+ attrmiss[attnum - 1].am_present = true;
}
}
need--;
diff --git a/src/include/access/tupdesc_details.h b/src/include/access/tupdesc_details.h
index 741e996..f7bb1de 100644
--- a/src/include/access/tupdesc_details.h
+++ b/src/include/access/tupdesc_details.h
@@ -19,11 +19,10 @@
* 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 */
+ bool am_present; /* true if non-NULL missing value exists */
+ Datum am_value; /* value when attribute is missing */
} AttrMissing;
#endif /* TUPDESC_DETAILS_H */
--
1.8.3.1
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
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
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