ALTER TABLE behind-the-scenes effects' CONTEXT
Hi,
In the past I've found the error message in cases such as this somewhat
less helpful than it could be:
=# CREATE TABLE qqq (a int);
=# CREATE UNIQUE INDEX IF NOT EXISTS qqq_a_idx ON qqq(a);
=# ALTER TABLE qqq ALTER COLUMN a TYPE json USING NULL;
ERROR: data type json has no default operator class for access method
"btree"
HINT: You must specify an operator class for the index or define a
default operator class for the data type.
The attached patch adds a CONTEXT line to index and constraint rebuilds,
e.g:
CONTEXT: while rebuilding index qqq_a_idx
Any feedback welcome.
.m
Attachments:
alter_column_bhs_effects_context.v0.patchtext/plain; charset=UTF-8; name=alter_column_bhs_effects_context.v0.patch; x-mac-creator=0; x-mac-type=0Download
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 82,87 ****
--- 82,88 ----
#include "storage/smgr.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+ #include "utils/elog.h"
#include "utils/fmgroids.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
***************
*** 309,314 **** static void ATController(AlterTableStmt *parsetree,
--- 310,316 ----
Relation rel, List *cmds, bool recurse, LOCKMODE lockmode);
static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
bool recurse, bool recursing, LOCKMODE lockmode);
+ static void ATRewriteSubcommandErrorCallback(void *arg);
static void ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode);
static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
AlterTableCmd *cmd, LOCKMODE lockmode);
***************
*** 3373,3378 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
--- 3375,3399 ----
tab->subcmds[pass] = lappend(tab->subcmds[pass], cmd);
}
+ static void
+ ATRewriteSubcommandErrorCallback(void *arg)
+ {
+ AlterTableCmd *subcmd = (AlterTableCmd *) arg;
+
+ switch (subcmd->subtype)
+ {
+ case AT_ReAddIndex:
+ errcontext("while rebuilding index %s", subcmd->name);
+ break;
+ case AT_ReAddConstraint:
+ errcontext("while rebuilding constraint %s", subcmd->name);
+ break;
+ default:
+ /* keep compiler quiet */
+ (void) 0;
+ }
+ }
+
/*
* ATRewriteCatalogs
*
***************
*** 3402,3407 **** ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
--- 3423,3429 ----
List *subcmds = tab->subcmds[pass];
Relation rel;
ListCell *lcmd;
+ ErrorContextCallback errcallback;
if (subcmds == NIL)
continue;
***************
*** 3411,3418 **** ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
*/
rel = relation_open(tab->relid, NoLock);
foreach(lcmd, subcmds)
! ATExecCmd(wqueue, tab, rel, (AlterTableCmd *) lfirst(lcmd), lockmode);
/*
* After the ALTER TYPE pass, do cleanup work (this is not done in
--- 3433,3451 ----
*/
rel = relation_open(tab->relid, NoLock);
+ errcallback.callback = ATRewriteSubcommandErrorCallback;
+ errcallback.previous = error_context_stack;
+ error_context_stack = &errcallback;
+
foreach(lcmd, subcmds)
! {
! AlterTableCmd *subcmd = (AlterTableCmd *) lfirst(lcmd);
!
! errcallback.arg = subcmd;
! ATExecCmd(wqueue, tab, rel, subcmd, lockmode);
! }
!
! error_context_stack = errcallback.previous;
/*
* After the ALTER TYPE pass, do cleanup work (this is not done in
***************
*** 8682,8687 **** ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
--- 8715,8721 ----
newcmd = makeNode(AlterTableCmd);
newcmd->subtype = AT_ReAddIndex;
+ newcmd->name = stmt->idxname;
newcmd->def = (Node *) stmt;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
***************
*** 8712,8717 **** ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
--- 8746,8752 ----
RelationRelationId, 0);
cmd->subtype = AT_ReAddIndex;
+ cmd->name = indstmt->idxname;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
***************
*** 8734,8739 **** ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
--- 8769,8775 ----
!rewrite && tab->rewrite == 0)
TryReuseForeignKey(oldId, con);
cmd->subtype = AT_ReAddConstraint;
+ cmd->name = con->conname;
tab->subcmds[AT_PASS_OLD_CONSTR] =
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1737,1742 **** alter table anothertab alter column atcol1 type boolean
--- 1737,1743 ----
using case when atcol1 % 2 = 0 then true else false end; -- fails
ERROR: operator does not exist: boolean <= integer
HINT: No operator matches the given name and argument type(s). You might need to add explicit type casts.
+ CONTEXT: while rebuilding constraint anothertab_chk
alter table anothertab drop constraint anothertab_chk;
alter table anothertab drop constraint anothertab_chk; -- fails
ERROR: constraint "anothertab_chk" of relation "anothertab" does not exist
2015-10-05 0:08 GMT+02:00 Marko Tiikkaja <marko@joh.to>:
Hi,
In the past I've found the error message in cases such as this somewhat
less helpful than it could be:=# CREATE TABLE qqq (a int);
=# CREATE UNIQUE INDEX IF NOT EXISTS qqq_a_idx ON qqq(a);
=# ALTER TABLE qqq ALTER COLUMN a TYPE json USING NULL;
ERROR: data type json has no default operator class for access method
"btree"
HINT: You must specify an operator class for the index or define a
default operator class for the data type.The attached patch adds a CONTEXT line to index and constraint rebuilds,
e.g:CONTEXT: while rebuilding index qqq_a_idx
Any feedback welcome.
I prefer using DETAIL field for this case.
Regards
Pavel
Show quoted text
.m
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Pavel Stehule wrote:
2015-10-05 0:08 GMT+02:00 Marko Tiikkaja <marko@joh.to>:
In the past I've found the error message in cases such as this somewhat
less helpful than it could be:=# CREATE TABLE qqq (a int);
=# CREATE UNIQUE INDEX IF NOT EXISTS qqq_a_idx ON qqq(a);
=# ALTER TABLE qqq ALTER COLUMN a TYPE json USING NULL;
ERROR: data type json has no default operator class for access method
"btree"
HINT: You must specify an operator class for the index or define a
default operator class for the data type.The attached patch adds a CONTEXT line to index and constraint rebuilds,
e.g:CONTEXT: while rebuilding index qqq_a_idx
I prefer using DETAIL field for this case.
I think DETAIL doesn't work for this; the advantage of CONTEXT is that
it can be set not at the location of the actual error but at the calling
code, by setting up a callback. I think Marko got that right.
I'd add some quoting to the object name in the message, and make sure we
don't crash if the name isn't set up (i.e. test for nullness). But
also, why do we set the name only in ATPostAlterTypeParse()? Maybe
there are more ALTER TABLE subcommands that should be setting something
up? In cases where multiple subcommands are being run, it might be
useful to see which one caused a certain error message.
I think some additional tests wouldn't hurt.
I await feedback from Simon Riggs, who set himself up as reviewer a
couple of days ago. Simon, do you also intend to be committer? If so,
please mark yourself as such in the commitfest app.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2016-01-04 21:44 GMT+01:00 Alvaro Herrera <alvherre@2ndquadrant.com>:
Pavel Stehule wrote:
2015-10-05 0:08 GMT+02:00 Marko Tiikkaja <marko@joh.to>:
In the past I've found the error message in cases such as this somewhat
less helpful than it could be:=# CREATE TABLE qqq (a int);
=# CREATE UNIQUE INDEX IF NOT EXISTS qqq_a_idx ON qqq(a);
=# ALTER TABLE qqq ALTER COLUMN a TYPE json USING NULL;
ERROR: data type json has no default operator class for access method
"btree"
HINT: You must specify an operator class for the index or define a
default operator class for the data type.The attached patch adds a CONTEXT line to index and constraint
rebuilds,
e.g:
CONTEXT: while rebuilding index qqq_a_idx
I prefer using DETAIL field for this case.
I think DETAIL doesn't work for this; the advantage of CONTEXT is that
it can be set not at the location of the actual error but at the calling
code, by setting up a callback. I think Marko got that right.
ok
Regards
Pavel
Show quoted text
I'd add some quoting to the object name in the message, and make sure we
don't crash if the name isn't set up (i.e. test for nullness). But
also, why do we set the name only in ATPostAlterTypeParse()? Maybe
there are more ALTER TABLE subcommands that should be setting something
up? In cases where multiple subcommands are being run, it might be
useful to see which one caused a certain error message.I think some additional tests wouldn't hurt.
I await feedback from Simon Riggs, who set himself up as reviewer a
couple of days ago. Simon, do you also intend to be committer? If so,
please mark yourself as such in the commitfest app.--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4 January 2016 at 20:44, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Maybe
there are more ALTER TABLE subcommands that should be setting something
up? In cases where multiple subcommands are being run, it might be
useful to see which one caused a certain error message.
I like the patch.
We should have a message for each subcommand, since there are many that run
for a long time and we support the optimization of allowing many
subcommands together at once.
There should also be a comment about making name a requirement for any
subcommand.
I think some additional tests wouldn't hurt.
Each subcommand message should be generated at least once in tests.
I await feedback from Simon Riggs, who set himself up as reviewer a
couple of days ago. Simon, do you also intend to be committer? If so,
please mark yourself as such in the commitfest app.
Happy to be the committer on this.
--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 5 January 2016 at 06:45, Simon Riggs <simon@2ndquadrant.com> wrote:
On 4 January 2016 at 20:44, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:Maybe
there are more ALTER TABLE subcommands that should be setting something
up? In cases where multiple subcommands are being run, it might be
useful to see which one caused a certain error message.I like the patch.
We should have a message for each subcommand, since there are many that
run for a long time and we support the optimization of allowing many
subcommands together at once.There should also be a comment about making name a requirement for any
subcommand.I think some additional tests wouldn't hurt.
Each subcommand message should be generated at least once in tests.
I await feedback from Simon Riggs, who set himself up as reviewer a
couple of days ago. Simon, do you also intend to be committer? If so,
please mark yourself as such in the commitfest app.Happy to be the committer on this.
Marko, I was/am waiting for an updated patch. Could you comment please?
--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2016-01-21 04:17, Simon Riggs wrote:
Marko, I was/am waiting for an updated patch. Could you comment please?
Sorry, I've not found time to work on this recently.
Thanks for everyone's comments so far. I'll move this to the next CF
and try and get an updated patch done in time for that one.
.m
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/30/16 10:52 AM, Marko Tiikkaja wrote:
On 2016-01-21 04:17, Simon Riggs wrote:
Marko, I was/am waiting for an updated patch. Could you comment please?
Sorry, I've not found time to work on this recently.
Thanks for everyone's comments so far. I'll move this to the next CF
and try and get an updated patch done in time for that one.
There's been no activity on this thread for while and no new patch was
submitted for the CF.
I have marked it "returned with feedback".
--
-David
david@pgmasters.net
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers