Re: ALTER TABLE behind-the-scenes effects' CONTEXT

Pavel Stehule wrote:
> 2015-10-05 0:08 GMT+02:00 Marko Tiikkaja <marko(at)joh(dot)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