Few cosmetic suggestions for commit 16828d5c (Fast Alter Table Add Column...)

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>
Subject: Few cosmetic suggestions for commit 16828d5c (Fast Alter Table Add Column...)
Date: 2018-06-02 23:38:23
Message-ID: CAA4eK1K2znsFpC+NQ9A4vxT4uDxADN4RmvHX0L6Y=aHVo9gB4Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
cosmetic_changes_fast_addcol_v1.patch application/octet-stream 1.3 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Justin Pryzby 2018-06-03 00:42:51 \d t: ERROR: XX000: cache lookup failed for relation
Previous Message Tom Lane 2018-06-02 22:06:10 Re: why partition pruning doesn't work?