Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Sun, Jan 16, 2011 at 12:07 PM, Tom Lane wrote: > Robert Haas writes: >> On Sat, Jan 15, 2011 at 10:25 AM, Noah Misch 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Sun, Jan 16, 2011 at 12:07:44PM -0500, Tom Lane wrote: > Robert Haas writes: > > On Sat, Jan 15, 2011 at 10:25 AM, Noah Misch 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
Robert Haas writes: > On Sat, Jan 15, 2011 at 10:25 AM, Noah Misch 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 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.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5790f87..6f5ffb3 100644
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***
*** 1420,1425 index_build(Relation heapRelation,
--- 1420,1435
procedure = indexRelation->rd_am->ambuild;
Assert(RegProcedureIsValid(procedure));
+ if (indexInfo->ii_ToastForRelName != NULL)
+ ereport(DEBUG1,
+ (errmsg("building TOAST index for table \"%s\"",
+
indexInfo->ii_ToastForRelName)));
+ else
+ ereport(DEBUG1,
+ (errmsg("building index \"%s\" on table \"%s\"",
+
RelationGetRelationName(indexRelation),
+
RelationGetRelationName(heapRelation;
+
/*
* Switch to the table owner's userid, so that any index functions are
run
* as that user. Also lock down security-restricted operations and
***
*** 2412,2418 IndexGetRelation(Oid indexId)
* reindex_index - This routine is used to recreate a single index
*/
void
! reindex_index(Oid indexId, bool skip_constraint_checks)
{
RelationiRel,
heapRelation,
--- 2422,2429
* reindex_index - This routine is used to recreate a single index
*/
void
! reindex_index(Oid indexId, const char *toastFor,
! bool skip_constraint_checks)
{
RelationiRel,
heapRelation,
***
*** 2470,2475 reindex_index(Oid indexId, bool skip_constraint_checks)
--- 2481,2489
indexInfo->ii_ExclusionStrats = NULL;
}
+ /* Pass the name of relation this TOAST index serves, if any. */
+ indexInfo->ii_ToastForRelName = toastFor;
+
/* We'll build a new physical relation for the index */
RelationSetNewRelfilenode(iRel, InvalidTransactionId);
***
*** 2529,2534 reindex_index(Oid indexId, bool skip_constraint_checks)
--- 2543,2551
* reindex_relation - This routine is used to recreate all indexes
* of a relation (and optionally its toast relation too, if any).
*
+ * If this is a TOAST relation, toastFor may bear the parent relation name,
+ * facilitating improved messages.
+ *
* If heap_rebuilt is true, then the relation was just completely rebuilt by
* an operation such as VACUUM FULL or CLUSTER, and therefore its indexes are
* inconsistent with it. This makes things tricky if the relation is a system
***
*** 2548,2554 reindex_index(Oid indexId, bool skip_constraint_checks)
* CommandCounterIncrement will occur after each index rebuild.
*/
bool
! reindex_relation(Oid relid, bool toast_too, bool heap_rebuilt)
{
Relationrel;
Oid toast_relid;
--- 2565,2572
* CommandCounterIncrement will occur after each index rebuild.
*/
bool
! reindex_relation(Oid relid, const char *toastFor,
!bool toast_too, bool heap_rebuilt)
{
Relationrel;
Oid toast_relid;
***
*** 2625,2631 reindex_relation(Oid relid, bool toast_too, bool
heap_rebuilt)
if (is_pg_class)
RelationSetIndexList(rel, doneIndexes,
InvalidOid);
! reindex_index(indexOid, heap_rebuilt);
CommandCounterIncrement();
--- 2643,2649
if (is_pg_class)
RelationSetIndexList(rel, doneIndexes,
InvalidOid);
! reindex_index(indexOid, toastFor, heap_rebuilt);
CommandCounterIncrement();
***
*** 2648,2658 reindex_relation(Oid relid, bool toast_too, bool
heap_rebuilt)
if (is_pg_class)
RelationSetIndexList(rel, indexIds, ClassOidIndexId);
- /*
-* Close rel, but continue to hold the lock.
-*/
- heap_close(rel, NoLock);
-
result = (indexIds != NIL);
/*
--- 2666,2671
***
*** 2660,2666 reindex_relation(Oid relid, bool toast_too, bool
heap_rebuilt)
* still hold the lock on the master table.
*/
if (toast_too && OidIsValid(toast_relid))
! result
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Sat, Jan 15, 2011 at 10:25 AM, Noah Misch 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 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?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Sat, Jan 15, 2011 at 1:30 AM, Noah Misch 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 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.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5790f87..8ff6927 100644
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***
*** 1420,1425 index_build(Relation heapRelation,
--- 1420,1430
procedure = indexRelation->rd_am->ambuild;
Assert(RegProcedureIsValid(procedure));
+ ereport(IsToastRelation(indexRelation) ? DEBUG2 : DEBUG1,
+ (errmsg("building index \"%s\" on table \"%s\"",
+ RelationGetRelationName(indexRelation),
+
RelationGetRelationName(heapRelation;
+
/*
* Switch to the table owner's userid, so that any index functions are
run
* as that user. Also lock down security-restricted operations and
diff --git a/src/backend/commands/index f3bd565..e3921e4 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 3443,3448 ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap,
LOCKMODE lockmode)
--- 3443,3457
List *dropped_attrs = NIL;
ListCell *lc;
+ if (newrel)
+ ereport(DEBUG1,
+ (errmsg("rewriting table \"%s\"",
+
RelationGetRelationName(oldrel;
+ else
+ ereport(DEBUG1,
+ (errmsg("verifying table \"%s\"",
+
RelationGetRelationName(oldrel;
+
econtext = GetPerTupleExprContext(estate);
/*
***
*** 3836,3841 ATTypedTableRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd,
--- 3845,3854
* Eventually, we'd like to propagate the check or rewrite operation
* into other such tables, but for now, just error out if we find any.
*
+ * 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.
+ *
* Caller should provide either a table name or a type name (not both) to
* report in the error message, if any.
*
***
*** 5789,5794 validateForeignKeyConstraint(Constraint *fkconstraint,
--- 5802,5811
HeapTuple tuple;
Trigger trig;
+ ereport(DEBUG1,
+ (errmsg("validating foreign key constraint \"%s\"",
+ fkconstraint->conname)));
+
/*
* Build a trigger call structure; we'll need it e
Re: [HACKERS] 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 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 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Tue, Jan 11, 2011 at 7:25 AM, Noah Misch 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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 +, 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Jan 11, 2011, at 8:50 AM, Noah Misch 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Tue, Jan 11, 2011 at 01:17:23PM +, Simon Riggs wrote: > On Tue, 2011-01-11 at 08:06 -0500, Noah Misch wrote: > > On Tue, Jan 11, 2011 at 12:37:28PM +, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 +, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Tue, Jan 11, 2011 at 12:37:28PM +, 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Tue, Jan 11, 2011 at 7:14 AM, Noah Misch 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Tue, Jan 11, 2011 at 09:24:46AM +, 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TYPE 0: Introduction; test cases
On Sun, Jan 9, 2011 at 4:59 PM, 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: > > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
