Thread: ALTER TYPE 0: Introduction; test cases

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
1 - recheck UNIQUE constraints on ALTER TYPE
2 - skip cases where we can already prove there's no work
3 - add ability to identify more cases; demo with varchar and xml
4 - support temporal types
5 - support varbit
6 - support numeric

Patches 0-2 are each freestanding.  Patch 3 depends on patch 2.  Patches 4-6 all
depend on 3, but not on each other.  I haven't tested permutations of patch
application other than a linear progression, so YMMV -- those are the conceptual
dependencies, though perhaps not the lexical ones.


This first patch adds various test cases that will exercise the conditions
pertinent to this patch series.  Later patches generally do not change the test
cases, but they do update the expected output, and this illustrates the
improvements each patch brings.  To make that possible, this patch also adds
DEBUG-level messages for when we build/rebuild an index, rewrite a table, scan a
table for CHECK constraint verification, or validate a foreign key constraint.

[1] http://archives.postgresql.org/pgsql-hackers/2010-12/msg02360.php

On Sun, 2011-01-09 at 16:59 -0500, Noah Misch 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:

These sound very good.

I have a concern that by making the ALTER TABLE more complex that we
might not be able to easily tell if a rewrite happens, or not.

Perhaps we should add a WITHOUT REWRITE clause? That would allow a user
to specify that they do not wish a rewrite, so if the AT requires them
to have one it would then fail.

You might point out I didn't do anything like that for my ALTER TABLE
patch, and I would agree with you. WITHOUT ACCESS EXCLUSIVE LOCK might
be an option here also.

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services

On Sun, Jan 9, 2011 at 4:59 PM, Noah Misch <noah@leadboat.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"

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 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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 11, 2011 at 09:24:46AM +0000, Simon Riggs wrote:
> On Sun, 2011-01-09 at 16:59 -0500, Noah Misch 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:
> 
> These sound very good.

Thanks.

> I have a concern that by making the ALTER TABLE more complex that we
> might not be able to easily tell if a rewrite happens, or not.
> 
> Perhaps we should add a WITHOUT REWRITE clause? That would allow a user
> to specify that they do not wish a rewrite, so if the AT requires them
> to have one it would then fail.

These changes do make it harder to guess how much work the ALTER TABLE will do.
Indeed, about 1/4 of my own guesses prior to writing were wrong.  Something like
WITHOUT REWRITE might be the way to go, though there are more questions: if it
does not rewrite, does it scan the table?  Which indexes, if any, does it
rebuild?  Which foreign key constraints, if any, does it recheck?  With patch 0,
you can answer all these questions by enabling DEBUG1 messages and trying the
command on your test system.  For this reason, I did consider adding a VERBOSE
clause to show those messages at DETAIL, rather than unconditionally showing
them at DEBUG1.  In any case, if a WITHOUT REWRITE like you describe covers the
important question, it's certainly easy enough to implement.

> You might point out I didn't do anything like that for my ALTER TABLE
> patch, and I would agree with you. WITHOUT ACCESS EXCLUSIVE LOCK might
> be an option here also.

True.  At least we could completely document the lock choices on the ALTER TABLE
reference page.  The no-rewrite cases are defined at arms length from ALTER
TABLE, and users can define their own, so it's a tougher fit.

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@leadboat.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.

On Tue, Jan 11, 2011 at 7:14 AM, Noah Misch <noah@leadboat.com> wrote:
>> I have a concern that by making the ALTER TABLE more complex that we
>> might not be able to easily tell if a rewrite happens, or not.
>>
>> Perhaps we should add a WITHOUT REWRITE clause? That would allow a user
>> to specify that they do not wish a rewrite, so if the AT requires them
>> to have one it would then fail.
>
> These changes do make it harder to guess how much work the ALTER TABLE will do.
> Indeed, about 1/4 of my own guesses prior to writing were wrong.  Something like
> WITHOUT REWRITE might be the way to go, though there are more questions: if it
> does not rewrite, does it scan the table?  Which indexes, if any, does it
> rebuild?  Which foreign key constraints, if any, does it recheck?  With patch 0,
> you can answer all these questions by enabling DEBUG1 messages and trying the
> command on your test system.  For this reason, I did consider adding a VERBOSE
> clause to show those messages at DETAIL, rather than unconditionally showing
> them at DEBUG1.  In any case, if a WITHOUT REWRITE like you describe covers the
> important question, it's certainly easy enough to implement.

I'm doubtful that it's worth complicating the parser logic.  Our DDL
is transactional, so someone can always begin a transaction, increase
client_min_messages, and then look at the output from these added
debug messages to see what is happening.  It should be clear within a
second or two if it's not what is wanted, and they can just hit ^C.

> True.  At least we could completely document the lock choices on the ALTER TABLE
> reference page.  The no-rewrite cases are defined at arms length from ALTER
> TABLE, and users can define their own, so it's a tougher fit.

I don't think it's prohibitively, tough, though, and I think it would
be very valuable.  We may have to scratch our heads over exactly where
to put the information, but we can figure out something that works.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, 2011-01-11 at 07:14 -0500, Noah Misch wrote:

> These changes do make it harder to guess how much work the ALTER TABLE
> will do. Indeed, about 1/4 of my own guesses prior to writing were
> wrong.  Something like WITHOUT REWRITE might be the way to go, though
> there are more questions: if it does not rewrite, does it scan the
> table?  Which indexes, if any, does it rebuild?  Which foreign key
> constraints, if any, does it recheck?  With patch 0, you can answer
> all these questions by enabling DEBUG1 messages and trying the command
> on your test system.  For this reason, I did consider adding a VERBOSE
> clause to show those messages at DETAIL, rather than unconditionally
> showing them at DEBUG1.  In any case, if a WITHOUT REWRITE like you
> describe covers the important question, it's certainly easy enough to
> implement.

Trouble is, only superusers can set DEBUG1.

You're right, its more complex than I made out, though that strengthens
the feeling that we need a way to check what it does before it does it,
or a way to limit your expectations. Ideally that would be a
programmatic way, so that pgAdmin et al can issue a warning.

Given your thoughts above, my preference would be for 
EXPLAIN ALTER TABLE to describe the actions that will take place.

Or other ideas... 

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Jan 11, 2011 at 12:37:28PM +0000, Simon Riggs wrote:
> On Tue, 2011-01-11 at 07:14 -0500, Noah Misch wrote:
> 
> > These changes do make it harder to guess how much work the ALTER TABLE
> > will do. Indeed, about 1/4 of my own guesses prior to writing were
> > wrong.  Something like WITHOUT REWRITE might be the way to go, though
> > there are more questions: if it does not rewrite, does it scan the
> > table?  Which indexes, if any, does it rebuild?  Which foreign key
> > constraints, if any, does it recheck?  With patch 0, you can answer
> > all these questions by enabling DEBUG1 messages and trying the command
> > on your test system.  For this reason, I did consider adding a VERBOSE
> > clause to show those messages at DETAIL, rather than unconditionally
> > showing them at DEBUG1.  In any case, if a WITHOUT REWRITE like you
> > describe covers the important question, it's certainly easy enough to
> > implement.
> 
> Trouble is, only superusers can set DEBUG1.

Setting client_min_messages in one's session works, as does "ALTER ROLE myself
SET client_min_messages = debug1".  Still, indeed, DEBUG1 is not the usual place
to send a user for information.

> You're right, its more complex than I made out, though that strengthens
> the feeling that we need a way to check what it does before it does it,
> or a way to limit your expectations. Ideally that would be a
> programmatic way, so that pgAdmin et al can issue a warning.
> 
> Given your thoughts above, my preference would be for 
> EXPLAIN ALTER TABLE to describe the actions that will take place.

That does seem like the best UI.  Offhand, I would guess that's a project larger
than the patch series I have here.  We'd need to restructure ALTER TABLE into
clear planning and execution stages, if not use the actual planner and executor.

On Tue, Jan 11, 2011 at 07:27:46AM -0500, Robert Haas wrote:
> On Tue, Jan 11, 2011 at 7:14 AM, Noah Misch <noah@leadboat.com> wrote:
> > True. ?At least we could completely document the lock choices on the ALTER TABLE
> > reference page. ?The no-rewrite cases are defined at arms length from ALTER
> > TABLE, and users can define their own, so it's a tougher fit.
> 
> I don't think it's prohibitively, tough, though, and I think it would
> be very valuable.  We may have to scratch our heads over exactly where
> to put the information, but we can figure out something that works.

Agreed.  I don't mean to suggest giving up on documenting anything, just that
some behaviors are clear enough by documentation alone, while others benefit
from additional syntax/messages to constrain/see what actually happens.

On Tue, 2011-01-11 at 08:06 -0500, Noah Misch wrote:
> On Tue, Jan 11, 2011 at 12:37:28PM +0000, Simon Riggs wrote:
> > On Tue, 2011-01-11 at 07:14 -0500, Noah Misch wrote:
> > 
> > > These changes do make it harder to guess how much work the ALTER TABLE
> > > will do. Indeed, about 1/4 of my own guesses prior to writing were
> > > wrong.  Something like WITHOUT REWRITE might be the way to go, though
> > > there are more questions: if it does not rewrite, does it scan the
> > > table?  Which indexes, if any, does it rebuild?  Which foreign key
> > > constraints, if any, does it recheck?  With patch 0, you can answer
> > > all these questions by enabling DEBUG1 messages and trying the command
> > > on your test system.  For this reason, I did consider adding a VERBOSE
> > > clause to show those messages at DETAIL, rather than unconditionally
> > > showing them at DEBUG1.  In any case, if a WITHOUT REWRITE like you
> > > describe covers the important question, it's certainly easy enough to
> > > implement.
> > 
> > Trouble is, only superusers can set DEBUG1.
> 
> Setting client_min_messages in one's session works, as does "ALTER ROLE myself
> SET client_min_messages = debug1".  Still, indeed, DEBUG1 is not the usual place
> to send a user for information.
> 
> > You're right, its more complex than I made out, though that strengthens
> > the feeling that we need a way to check what it does before it does it,
> > or a way to limit your expectations. Ideally that would be a
> > programmatic way, so that pgAdmin et al can issue a warning.
> > 
> > Given your thoughts above, my preference would be for 
> > EXPLAIN ALTER TABLE to describe the actions that will take place.
> 
> That does seem like the best UI.  Offhand, I would guess that's a project larger
> than the patch series I have here.  We'd need to restructure ALTER TABLE into
> clear planning and execution stages, if not use the actual planner and executor.

Please do something that works in this release, whatever that is. I will
follow your lead in putting a similar mechanism in for judging lock
levels.

I don't want to be looking through the docs each time I run this,
sweating between it taking 5 secs and 5 hours on a big server.
We need to be able to run stuff overnight, with certainty that we know
what will happen.

And I want a clear answer when the "but how can you be certain?"
question gets asked.

Thank you for the rest of the patch.

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Jan 11, 2011 at 01:17:23PM +0000, Simon Riggs wrote:
> On Tue, 2011-01-11 at 08:06 -0500, Noah Misch wrote:
> > On Tue, Jan 11, 2011 at 12:37:28PM +0000, Simon Riggs wrote:
> > > Given your thoughts above, my preference would be for 
> > > EXPLAIN ALTER TABLE to describe the actions that will take place.
> > 
> > That does seem like the best UI.  Offhand, I would guess that's a project larger
> > than the patch series I have here.  We'd need to restructure ALTER TABLE into
> > clear planning and execution stages, if not use the actual planner and executor.
> 
> Please do something that works in this release, whatever that is. I will
> follow your lead in putting a similar mechanism in for judging lock
> levels.

Okay; I'll see what I can come up with.  The other part I was going to try to
finish before the last commitfest begins is avoiding unnecessary rebuilds of
indexes involving changed columns.  Is that more or less important than having
an EXPLAIN ALTER TABLE?

> I don't want to be looking through the docs each time I run this,
> sweating between it taking 5 secs and 5 hours on a big server.
> We need to be able to run stuff overnight, with certainty that we know
> what will happen.
> 
> And I want a clear answer when the "but how can you be certain?"
> question gets asked.

Just to clarify: does Robert's suggestion of starting the command in a
transaction block and cancelling it after messages appear (on any other DB with
the same schema, if need be) give too little certainty?  You could check
pg_locks to see what lock was taken, too.  It's surely not the ideal user
experience, but it seems dependable at least.

Thanks,
nm

On Jan 11, 2011, at 8:50 AM, Noah Misch <noah@leadboat.com> wrote:
> Okay; I'll see what I can come up with.  The other part I was going to try to
> finish before the last commitfest begins is avoiding unnecessary rebuilds of
> indexes involving changed columns.  Is that more or less important than having
> an EXPLAIN ALTER TABLE?

I think something like EXPLAIN ALTER TABLE would be #%^* awesome (and kudos to Simon for thinking of it), but your
chancesof getting into 9.1 are not very good.  So I'd focus on the index rebuild issue, which is more clear-cut. 

...Robert

On Tue, 2011-01-11 at 07:14 -0500, Noah Misch wrote:
> On Tue, Jan 11, 2011 at 09:24:46AM +0000, Simon Riggs wrote:
> > I have a concern that by making the ALTER TABLE more complex that we
> > might not be able to easily tell if a rewrite happens, or not.

What about add EXPLAIN support to it, then whoever wants to know what it
does should just run explain on it. I have no idea how hard that would
be to implement and if it makes sense at all, but I sure would like to
see a plan for DDLs too to estimate how long it would take.

Cheers,
Csaba.

On Tue, Jan 11, 2011 at 7:25 AM, Noah Misch <noah@leadboat.com> wrote:
> 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@leadboat.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?

I'm wondering if we should consider moving this call to index_build()
so that it appears everywhere that we build an index rather than just
in ALTER TABLE, and saying something like:

building index "%s" on table "%s"

> 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.

Perhaps in this case we could write:

building TOAST index for table "%s"

There's limited need for users to know the actual name of the TOAST
table, but I think it's better to log a line for all the actions we're
performing or none of them, rather than some but not others.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 11, 2011 at 09:41:35PM -0500, Robert Haas wrote:
> On Tue, Jan 11, 2011 at 7:25 AM, Noah Misch <noah@leadboat.com> wrote:
> > 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@leadboat.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?
> 
> I'm wondering if we should consider moving this call to index_build()
> so that it appears everywhere that we build an index rather than just
> in ALTER TABLE, and saying something like:
> 
> building index "%s" on table "%s"

The patch does have it in index_build.  That new wording seems better.

> > 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.
> 
> Perhaps in this case we could write:
> 
> building TOAST index for table "%s"

Good idea; thanks.

I'll incorporate those changes into the next version.

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.

On Tue, Jan 11, 2011 at 09:41:35PM -0500, Robert Haas wrote:
> On Tue, Jan 11, 2011 at 7:25 AM, Noah Misch <noah@leadboat.com> wrote:
> I'm wondering if we should consider moving this call to index_build()
> so that it appears everywhere that we build an index rather than just
> in ALTER TABLE, and saying something like:
>
> building index "%s" on table "%s"

Done.

I also fixed the leading capital letter on the other new messages.

> > 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.
>
> Perhaps in this case we could write:
>
> building TOAST index for table "%s"
>
> There's limited need for users to know the actual name of the TOAST
> table, but I think it's better to log a line for all the actions we're
> performing or none of them, rather than some but not others.

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.

On Sat, Jan 15, 2011 at 1:30 AM, Noah Misch <noah@leadboat.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.

> 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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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@leadboat.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?

On Sat, Jan 15, 2011 at 10:25 AM, Noah Misch <noah@leadboat.com> wrote:
> Do you value test coverage so little?

If you're asking whether I think real-world usability is more
important than test coverage, then yes.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Jan 15, 2011 at 02:31:21PM -0500, Robert Haas wrote:
> On Sat, Jan 15, 2011 at 10:25 AM, Noah Misch <noah@leadboat.com> wrote:
> > Do you value test coverage so little?
>
> If you're asking whether I think real-world usability is more
> important than test coverage, then yes.

No, I wasn't asking that.

The attached version implements your preferred real-world usability for the
index_build DEBUG message.

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Jan 15, 2011 at 10:25 AM, Noah Misch <noah@leadboat.com> wrote:
>> Do you value test coverage so little?

> If you're asking whether I think real-world usability is more
> important than test coverage, then yes.

Quite honestly, I'd be inclined to rip out most of the DEBUG messages I
see in that regression test altogether.  They are useless, and so is the
regression test itself.  An appropriate regression test would involve
something more like checking that the relfilenode changed and then
checking that the contained data is still sane.
        regards, tom lane

On Sun, Jan 16, 2011 at 12:07:44PM -0500, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Sat, Jan 15, 2011 at 10:25 AM, Noah Misch <noah@leadboat.com> wrote:
> >> Do you value test coverage so little?
> 
> > If you're asking whether I think real-world usability is more
> > important than test coverage, then yes.
> 
> Quite honestly, I'd be inclined to rip out most of the DEBUG messages I
> see in that regression test altogether.  They are useless, and so is the
> regression test itself.  An appropriate regression test would involve
> something more like checking that the relfilenode changed and then
> checking that the contained data is still sane.

This patch is the first of a series.  Divorced from the other patches, many of
the test cases exercise the same code path, making them redundant.  Even so, the
tests revealed a defect we released with 9.0; that seems sufficient to promote
them out of the "useless" bucket.

One can easily confirm by inspection that the relfilenode will change if and
only if the "rewriting" DEBUG message appears.  Your proposed direct comparison
of the relfilenode in the regression tests adds negligible sensitivity.  If that
were all, I'd call it a question of style.  However, a relfilenode comparison
does not distinguish no-op changes from changes entailing a verification scan.
A similar ambiguity would arise without the "foreign key" DEBUG message.

As for "checking that the contained data is still sane", what do you have in
mind?  After the test cases, I SELECT the table-under-test and choice catalog
entries.  If later patches in the series leave these expected outputs unchanged,
that confirms the continued sanity of the data.  Perhaps I should do this after
every test, or also test forced index scans.

nm

On Sun, Jan 16, 2011 at 12:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sat, Jan 15, 2011 at 10:25 AM, Noah Misch <noah@leadboat.com> wrote:
>>> Do you value test coverage so little?
>
>> If you're asking whether I think real-world usability is more
>> important than test coverage, then yes.
>
> Quite honestly, I'd be inclined to rip out most of the DEBUG messages I
> see in that regression test altogether.  They are useless, and so is the
> regression test itself.  An appropriate regression test would involve
> something more like checking that the relfilenode changed and then
> checking that the contained data is still sane.

From my point of view, the value of those messages is that if someone
is altering or clustering a large table, they might like to get a
series of messages: rewriting the table, rebuilding this index,
rebuilding that index, rewriting the toast table index, .... as a
crude sort of progress indication.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company