ALTER TYPE 0: Introduction; test cases

Started by Noah Mischover 15 years ago23 messageshackers
Jump to latest
#1Noah Misch
noah@leadboat.com

This begins the patch series for the design I recently proposed[1]http://archives.postgresql.org/pgsql-hackers/2010-12/msg02360.php 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

Attachments:

at0-debug-tests.patchtext/plain; charset=us-asciiDownload+2113-0
#2Simon Riggs
simon@2ndQuadrant.com
In reply to: Noah Misch (#1)
Re: ALTER TYPE 0: Introduction; test cases

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

#3Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#1)
Re: ALTER TYPE 0: Introduction; test cases

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

#4Noah Misch
noah@leadboat.com
In reply to: Simon Riggs (#2)
Re: ALTER TYPE 0: Introduction; test cases

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.

#5Noah Misch
noah@leadboat.com
In reply to: Robert Haas (#3)
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@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.

#6Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#4)
Re: ALTER TYPE 0: Introduction; test cases

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

#7Csaba Nagy
ncslists@googlemail.com
In reply to: Noah Misch (#4)
Re: ALTER TYPE 0: Introduction; test cases

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.

#8Simon Riggs
simon@2ndQuadrant.com
In reply to: Noah Misch (#4)
Re: ALTER TYPE 0: Introduction; test cases

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

#9Noah Misch
noah@leadboat.com
In reply to: Simon Riggs (#8)
Re: ALTER TYPE 0: Introduction; test cases

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.

#10Noah Misch
noah@leadboat.com
In reply to: Robert Haas (#6)
Re: ALTER TYPE 0: Introduction; test cases

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.

#11Simon Riggs
simon@2ndQuadrant.com
In reply to: Noah Misch (#9)
Re: ALTER TYPE 0: Introduction; test cases

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

#12Noah Misch
noah@leadboat.com
In reply to: Simon Riggs (#11)
Re: ALTER TYPE 0: Introduction; test cases

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

#13Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#12)
Re: ALTER TYPE 0: Introduction; test cases

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 chances of getting into 9.1 are not very good. So I'd focus on the index rebuild issue, which is more clear-cut.

...Robert

#14Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#5)
Re: ALTER TYPE 0: Introduction; test cases

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

#15Noah Misch
noah@leadboat.com
In reply to: Robert Haas (#14)
Re: ALTER TYPE 0: Introduction; test cases

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.

#16Noah Misch
noah@leadboat.com
In reply to: Robert Haas (#14)
Re: ALTER TYPE 0: Introduction; test cases

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.

Attachments:

at0v2-debug-tests.patchtext/plain; charset=us-asciiDownload+239-0
#17Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#16)
Re: ALTER TYPE 0: Introduction; test cases

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

#18Noah Misch
noah@leadboat.com
In reply to: Robert Haas (#17)
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@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?

#19Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#18)
Re: ALTER TYPE 0: Introduction; test cases

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

#20Noah Misch
noah@leadboat.com
In reply to: Robert Haas (#19)
Re: ALTER TYPE 0: Introduction; test cases

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.

Attachments:

at0v3-debug-tests.patchtext/plain; charset=us-asciiDownload+332-64
#21Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#19)
#22Noah Misch
noah@leadboat.com
In reply to: Tom Lane (#21)
#23Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#21)