Re: ALTER TYPE 0: Introduction; test cases

On Tue, Jan 11, 2011 at 06:37:33AM -0500, Robert Haas wrote:
> On Sun, Jan 9, 2011 at 4:59 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > This begins the patch series for the design I recently proposed[1] for avoiding
> > some table rewrites in ALTER TABLE ... ALTER COLUMN ... TYPE. ?I'm posting these
> > patches today:
> >
> > 0 - new test cases
>
> This doesn't look right. You might be building it, but you sure
> aren't rebuilding it.
>
> +CREATE TABLE parent (keycol numeric PRIMARY KEY);
> +NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
> "parent_pkey" for table "parent"
> +DEBUG: Rebuilding index "parent_pkey"

Yes. I considered saying "Building" unconditionally. Differentiating the debug
message by passing down the fact that the index recently existed seemed like
overkill. What do you think?

> In general, I think this is six kinds of overkill. I don't think we
> really need 2000 lines of new regression tests for this feature. I'd
> like to see that chopped down by at least 10x.

I just included all the tests I found useful to check my own work. If 200 lines
of SQL and expected output is the correct amount, I'll make it so.

> I don't like this bit:
>
> + ereport(IsToastRelation(indexRelation) ? DEBUG2 : DEBUG1,
>
> I see no reason to set the verbosity differently depending on whether
> or not something's a toast relation; that seems more likely to be
> confusing than helpful. I guess my vote would be to make all of these
> messages DEBUG2, period. A quick test suggests that doesn't produce
> too much noise executing DDL commands.

The theoretical basis is that users can do little to directly change the need to
rebuild a TOAST index. We'll rebuild the TOAST index if and only if we rewrote
the table. The practical basis is that the TOAST relation names contain parent
relation OIDs, so we don't want them mentioned in regression test output.

Thanks for reviewing.