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

[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

[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

[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

[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

[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

[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

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