Re: ALTER TABLE ADD COLUMN fast default

Hi,

On 2018-03-26 09:02:10 +1030, Andrew Dunstan wrote:
> Thanks for this, all looks good. Here is the consolidate patch
> rebased. If there are no further comments I propose to commit this in
> a few days time.

Here's a bit of post commit review:

@@ -1310,13 +1679,11 @@ slot_getsomeattrs(TupleTableSlot *slot, int attnum)

/*
* If tuple doesn't have all the atts indicated by tupleDesc, read the
- * rest as null
+ * rest as NULLs or missing values
*/
- for (; attno < attnum; attno++)
- {
- slot->tts_values[attno] = (Datum) 0;
- slot->tts_isnull[attno] = true;
- }
+ if (attno < attnum)
+ slot_getmissingattrs(slot, attno, attnum);
+
slot->tts_nvalid = attnum;
}

It's worthwhile to note that this'll re-process *all* missing values,
even if they've already been deformed. I.e. if
slot_getmissingattrs(.., first-missing)
slot_getmissingattrs(.., first-missing + 1)
slot_getmissingattrs(.., first-missing + 2)
is called, all three missing values will be copied every time. That's
because tts_nvalid isn't taken into account. I wonder if slot_getmissingattrs
could take tts_nvalid into account?

I also wonder if this doesn't deserve an unlikely(), to avoid the cost
of spilling registers in the hot branch of not missing any values.

+ else
+ {
+ /* if there is a missing values array we must process them one by one */
+ for (missattnum = lastAttNum - 1;
+ missattnum >= startAttNum;
+ missattnum--)
+ {
+ slot->tts_values[missattnum] = attrmiss[missattnum].ammissing;
+ slot->tts_isnull[missattnum] =
+ !attrmiss[missattnum].ammissingPresent;
+ }
+ }
+}

Why is this done backwards? It's noticeably slower to walk arrays
backwards on some CPU microarchitectures.

@@ -176,6 +179,23 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
}
}

@@ -469,6 +503,29 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
if (strcmp(defval1->adbin, defval2->adbin) != 0)
return false;
}
+ if (constr1->missing)
+ {
+ if (!constr2->missing)
+ return false;
+ for (i = 0; i < tupdesc1->natts; i++)
+ {
+ AttrMissing *missval1 = constr1->missing + i;
+ AttrMissing *missval2 = constr2->missing + i;

It's a bit odd to not use array indexing here?

@@ -625,7 +625,15 @@ ExecCopySlotMinimalTuple(TupleTableSlot *slot)
if (slot->tts_mintuple)
return heap_copy_minimal_tuple(slot->tts_mintuple);
if (slot->tts_tuple)
- return minimal_tuple_from_heap_tuple(slot->tts_tuple);
+ {
+ if (TTS_HAS_PHYSICAL_TUPLE(slot) &&
+ HeapTupleHeaderGetNatts(slot->tts_tuple->t_data)
+ < slot->tts_tupleDescriptor->natts)
+ return minimal_expand_tuple(slot->tts_tuple,
+ slot->tts_tupleDescriptor);
+ else
+ return minimal_tuple_from_heap_tuple(slot->tts_tuple);
+ }

What's the TTS_HAS_PHYSICAL_TUPLE doing here? How can this be relevant
given the previous tts_mintuple check? Am I missing something?

@@ -563,10 +569,63 @@ RelationBuildTupleDesc(Relation relation)
MemoryContextAllocZero(CacheMemoryContext,
relation->rd_rel->relnatts *
sizeof(AttrDefault));
- attrdef[ndef].adnum = attp->attnum;
+ attrdef[ndef].adnum = attnum;
attrdef[ndef].adbin = NULL;
+
ndef++;
}
+
+ /* Likewise for a missing value */
+ if (attp->atthasmissing)
+ {
+ Datum missingval;
+ bool missingNull;
+
+ /* Do we have a missing value? */
+ missingval = heap_getattr(pg_attribute_tuple,
+ Anum_pg_attribute_attmissingval,
+ pg_attribute_desc->rd_att,
+ &missingNull);
+ if (!missingNull)
+ {
+ /* Yes, fetch from the array */
+ MemoryContext oldcxt;
+ bool is_null;
+ int one = 1;
+ Datum missval;
+
+ if (attrmiss == NULL)
+ attrmiss = (AttrMissing *)
+ MemoryContextAllocZero(CacheMemoryContext,
+ relation->rd_rel->relnatts *
+ sizeof(AttrMissing));
+
+ missval = array_get_element(missingval,
+ 1,
+ &one,
+ -1,
+ attp->attlen,
+ attp->attbyval,
+ attp->attalign,
+ &is_null);
+ Assert(!is_null);
+ if (attp->attbyval)
+ {
+ /* for copy by val just copy the datum direct */
+ attrmiss[attnum - 1].ammissing = missval;
+ }
+ else
+ {
+ /* otherwise copy in the correct context */
+ oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
+ attrmiss[attnum - 1].ammissing = datumCopy(missval,
+ attp->attbyval,
+ attp->attlen);
+ MemoryContextSwitchTo(oldcxt);
+ }
+ attrmiss[attnum - 1].ammissingPresent = true;
+ }
+ }
need--;
if (need == 0)
break;

I'm still strongly object to do doing this unconditionally for queries
that never need any of this. We're can end up with a significant number
of large tuples in memory here, and then copy that through dozens of
tupledesc copies in queries.

@@ -167,6 +170,12 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK

/* Column-level FDW options */
text attfdwoptions[1] BKI_DEFAULT(_null_);
+
+ /*
+ * Missing value for added columns. This is a one element array which lets
+ * us store a value of the attribute type here.
+ */
+ anyarray attmissingval BKI_DEFAULT(_null_);
#endif
} FormData_pg_attribute;

Still think this is a bad location, and it'll reduce cache hit ratio for
catalog lookups.

Greetings,

Andres Freund