Re: [HACKERS] ALTER TABLE behind-the-scenes effects' CONTEXT

2016-03-15 Thread David Steele
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
da...@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


Re: [HACKERS] ALTER TABLE behind-the-scenes effects' CONTEXT

2016-01-30 Thread Marko Tiikkaja

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


Re: [HACKERS] ALTER TABLE behind-the-scenes effects' CONTEXT

2016-01-20 Thread Simon Riggs
On 5 January 2016 at 06:45, Simon Riggs  wrote:

> On 4 January 2016 at 20:44, Alvaro Herrera 
> 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 Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [HACKERS] ALTER TABLE behind-the-scenes effects' CONTEXT

2016-01-04 Thread Pavel Stehule
2016-01-04 21:44 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > 2015-10-05 0:08 GMT+02:00 Marko Tiikkaja :
> >
> > > 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


>
> 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 Herrerahttp://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>


Re: [HACKERS] ALTER TABLE behind-the-scenes effects' CONTEXT

2016-01-04 Thread Alvaro Herrera
Pavel Stehule wrote:
> 2015-10-05 0:08 GMT+02:00 Marko Tiikkaja :
> 
> > 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 Herrerahttp://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


Re: [HACKERS] ALTER TABLE behind-the-scenes effects' CONTEXT

2016-01-04 Thread Simon Riggs
On 4 January 2016 at 20:44, Alvaro Herrera  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 Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [HACKERS] ALTER TABLE behind-the-scenes effects' CONTEXT

2015-10-04 Thread Pavel Stehule
2015-10-05 0:08 GMT+02:00 Marko Tiikkaja :

> 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


>
> .m
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>


[HACKERS] ALTER TABLE behind-the-scenes effects' CONTEXT

2015-10-04 Thread Marko Tiikkaja

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
*** 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];
Relationrel;
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 = 
+ 
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] =