Re: ALTER TYPE 0: Introduction; test cases

On Sat, Jan 15, 2011 at 08:57:30AM -0500, Robert Haas wrote:
> On Sat, Jan 15, 2011 at 1:30 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > Here's v2 based on your feedback.
> >
> > I pruned test coverage down to just the highlights. ?By the end of patch series,
> > the net change becomes +67 to alter_table.sql and +111 to alter_table.out. ?The
> > alter_table.out delta is larger in this patch (+150), because the optimizations
> > don't yet apply and more objects are reported as rebuilt.
> >
> > If this looks sane, I'll rebase the rest of the patches accordingly.
>
> + * Table, NOT NULL and DEFAULT constraints and the "oid" system column do
> + * not (currently) follow the row type, so they require no attention here.
> + * The non-propagation of DEFAULT and NOT NULL make ADD COLUMN safe, too.
>
> This comment seems somewhat unrelated to the rest of the patch, and I
> really hope that the first word ("Table") actually means "CHECK",
> because we certainly shouldn't be treating table check constraints and
> column check constraints differently, at least AIUI.

"Table" should be "CHECK"; thanks. I added the comment in this patch because
it's clarifying existing behavior that was not obvious to me, rather than
documenting a new fact arising due to the patch series. A few of the new test
cases address this behavior.

> > That was a good idea, but the implementation is awkward. ?index_build has the
> > TOAST table and index relations, and there's no good way to find the parent
> > table from either. ?No index covers pg_class.reltoastrelid, so scanning by that
> > would be a bad idea. ?Autovacuum solves this by building its own hash table with
> > the mapping; that wouldn't fit well here. ?We could parse the parent OID out of
> > the TOAST name (heh, heh). ?I suppose we could pass the parent relation from
> > create_toast_table down through index_create to index_build. ?Currently,
> > index_create knows nothing of the fact that it's making a TOAST index, and
> > changing that to improve this messages seems a bit excessive. ?Thoughts?
> >
> > For this version, I've kept the DEBUG1/DEBUG2 split by TOAST.
>
> Well, I pretty much think that split is going to be not what anyone
> wants for any purpose OTHER than the regression tests. So if there's
> no other way around it I'd be much more inclined to remove the
> regression tests than to keep that split.

Do you value test coverage so little?