Re: ALTER TABLE ADD COLUMN fast default

Hi,

On 2018-02-27 14:29:44 +1030, Andrew Dunstan wrote:
> This was debated quite some time ago. See for example
> <https://www.postgresql.org/message-id/22999.1475770835%40sss.pgh.pa.us>
> The consensus then seemed to be to store them in pg_attribute. If not,
> I think we'd need a new catalog - pg_attrdef doesn't seem right. They
> would have to go on separate rows, and we'd be storing values rather
> than expressions, so it would get somewhat ugly.

I know that I'm concerned with storing a number of large values in
pg_attributes, and I haven't seen that argument addressed much in a
quick scan of the discussion.

> >> @@ -293,10 +408,18 @@ heap_fill_tuple(TupleDesc tupleDesc,
> >> * ----------------
> >> */
> >> bool
> >> -heap_attisnull(HeapTuple tup, int attnum)
> >> +heap_attisnull(HeapTuple tup, int attnum, TupleDesc tupleDesc)
> >> {
> >> + /*
> >> + * We allow a NULL tupledesc for relations not expected to have
> >> + * missing values, such as catalog relations and indexes.
> >> + */
> >> + Assert(!tupleDesc || attnum <= tupleDesc->natts);
> >
> > This seems fairly likely to hide bugs.

> How? There are plenty of cases where we simply expect quite reasonably
> that there won't be any missing attributes, and we don't need a
> tupleDesc at all in such cases.

Missing to pass a tupledesc for tables that can have defaults with not
have directly visible issues, you'll just erroneously get back a null.

> >> @@ -508,6 +508,8 @@ tlist_matches_tupdesc(PlanState *ps, List *tlist, Index varno, TupleDesc tupdesc
> >> return false; /* out of order */
> >> if (att_tup->attisdropped)
> >> return false; /* table contains dropped columns */
> >> + if (att_tup->atthasmissing)
> >> + return false; /* table contains cols with missing values */
> >
> > Hm, so incrementally building a table definitions with defaults, even if
> > there aren't ever any rows, or if there's a subsequent table rewrite,
> > will cause slowdowns. If so, shouldn't a rewrite remove all
> > atthasmissing values?

> Yes, I think so. Working on it. OTOH, I'm somewhat mystified by having
> had to make this change. Still, it looks like dropping a column has
> the same effect.

What exactly is mystifying you? That the new default stuff doesn't work
when the physical tlist optimization is in use?

> >> @@ -561,10 +568,73 @@ 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)
> >> + {
> >
> > As I've written somewhere above, I don't think it's a good idea to do
> > this preemptively. Tupledescs are very commonly copied, and the
> > missing attributes are quite likely never used. IOW, we should just
> > remember the attmissingattr value and fill in the corresponding
> > attmissingval on-demand.

> Defaults are frequently not used either, yet this function fills those
> in regardless. I don't see why we need to treat missing values
> differently.

It's already hurting us quite a bit in workloads with a lot of trivial
queries. Adding more makes it worse.

> Attached is the current state of things, with the fixes indicated
> above, as well as some things pointed out by David Rowley.
>
> None of this seems to have had much effect on performance.
>
> Here's what oprofile reports from a run of pgbench with
> create-alter.sql and the query "select sum(c1000) from t":
>
> Profiling through timer interrupt
> samples % image name symbol name
> 22584 28.5982 postgres ExecInterpExpr
> 11950 15.1323 postgres slot_getmissingattrs
> 5471 6.9279 postgres AllocSetAlloc
> 3018 3.8217 postgres TupleDescInitEntry
> 2042 2.5858 postgres MemoryContextAllocZeroAligned
> 1807 2.2882 postgres SearchCatCache1
> 1638 2.0742 postgres expression_tree_walker
>
>
> That's very different from what we see on the master branch. The time
> spent in slot_getmissingattrs is perhaps not unexpected, but I don't
> (at least yet) understand why we're getting so much time spent in
> ExecInterpExpr, which doesn't show up at all when the same benchmark
> is run on master.

I'd guess that's because the physical tlist optimization is disabled. I
assume you'd see something similar on master if you dropped a column.

- Andres