Обсуждение: wrong hint message for ALTER FOREIGN TABLE

I noticed that ALTER FOREIGN TABLE RENAME TO emits a wrong hint message
if the object was not a foreign table.  ISTM that the hint message is
not necessary there. Attached patch removes the hint message.

Steps to reproduce the situation:

postgres=# CREATE FOREIGN TABLE foo () SERVER file_server;
postgres=# ALTER FOREIGN TABLE foo RENAME TO bar;
ERROR:  "foo" is not a foreign table
HINT:  Use ALTER FOREIGN TABLE instead.

Regards,
--
Shigeru Hanada

On 25 April 2011 10:06, Shigeru Hanada <hanada@metrosystems.co.jp> wrote:
> I noticed that ALTER FOREIGN TABLE RENAME TO emits a wrong hint message if
> the object was not a foreign table.  ISTM that the hint message is not
> necessary there. Attached patch removes the hint message.
>
> Steps to reproduce the situation:
>
> postgres=# CREATE FOREIGN TABLE foo () SERVER file_server;
> postgres=# ALTER FOREIGN TABLE foo RENAME TO bar;
> ERROR:  "foo" is not a foreign table
> HINT:  Use ALTER FOREIGN TABLE instead.

Don't you mean that you created a regular table first, then tried to
rename it as a foreign table?  Your example here will be successful
without the error.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(2011/04/25 19:34), Thom Brown wrote:
> Don't you mean that you created a regular table first, then tried to
> rename it as a foreign table?  Your example here will be successful
> without the error.

Oops, you are right.
Right procedure to reproduce is:

postgres=# CREATE TABLE foo(c1 int);
CREATE TABLE
postgres=# ALTER FOREIGN TABLE foo RENAME TO bar;
ERROR:  "foo" is not a foreign table
HINT:  Use ALTER FOREIGN TABLE instead.

Regards,
-- 
Shigeru Hanada

Shigeru Hanada <hanada@metrosystems.co.jp> writes:
> I noticed that ALTER FOREIGN TABLE RENAME TO emits a wrong hint message 
> if the object was not a foreign table.  ISTM that the hint message is 
> not necessary there. Attached patch removes the hint message.

Surely it would be better to make the hint correct (ie, "Use ALTER TABLE")
rather than just nuke it?
        regards, tom lane

On Mon, Apr 25, 2011 at 10:17 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Shigeru Hanada <hanada@metrosystems.co.jp> writes:
>> I noticed that ALTER FOREIGN TABLE RENAME TO emits a wrong hint message
>> if the object was not a foreign table.  ISTM that the hint message is
>> not necessary there. Attached patch removes the hint message.
>
> Surely it would be better to make the hint correct (ie, "Use ALTER TABLE")
> rather than just nuke it?

The sequence of steps that he posted wasn't actually right.  See
subsequent emails on the thread.  The patch seems trivially correct,
since this is obviously schizophrenic:
         ereport(ERROR,                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),                  errmsg("\"%s\" is not a
foreigntable", 
!                         RelationGetRelationName(targetrelation)),
!                  errhint("Use ALTER FOREIGN TABLE instead.")));

It's not a... so I should use a... erp.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> The sequence of steps that he posted wasn't actually right.

Yes, I did read the thread.

> The patch seems trivially correct,
> since this is obviously schizophrenic:

>           ereport(ERROR,
>                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                    errmsg("\"%s\" is not a foreign table",
> !                         RelationGetRelationName(targetrelation)),
> !                  errhint("Use ALTER FOREIGN TABLE instead.")));

Well, it's a pretty obvious global-search-and-replace error, but I fail
to see the advantage of just deleting the hint.  I was thinking

-             errhint("Use ALTER FOREIGN TABLE instead.")));
+             errhint("Use ALTER TABLE instead.")));

As per the other thread today, this advice would usually be correct,
so I think that not offering any advice at all would be a step down from
that.
        regards, tom lane

On Mon, Apr 25, 2011 at 7:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> The sequence of steps that he posted wasn't actually right.
>
> Yes, I did read the thread.

OK.

>> The patch seems trivially correct,
>> since this is obviously schizophrenic:
>
>>               ereport(ERROR,
>>                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>>                                errmsg("\"%s\" is not a foreign table",
>> !                                             RelationGetRelationName(targetrelation)),
>> !                              errhint("Use ALTER FOREIGN TABLE instead.")));
>
> Well, it's a pretty obvious global-search-and-replace error, but I fail
> to see the advantage of just deleting the hint.  I was thinking
>
> -                        errhint("Use ALTER FOREIGN TABLE instead.")));
> +                        errhint("Use ALTER TABLE instead.")));
>
> As per the other thread today, this advice would usually be correct,
> so I think that not offering any advice at all would be a step down from
> that.

Well, currently ALTER TABLE will work even if the argument is a view
or sequence, but I view that as a backwards-compatibility kludge we
should be looking to move away from, not something we want to further
bake in.  However, I'm out of time to bikeshed on this issue, so fix
it however you like.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 25, 2011 at 7:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As per the other thread today, this advice would usually be correct,
>> so I think that not offering any advice at all would be a step down from
>> that.

> Well, currently ALTER TABLE will work even if the argument is a view
> or sequence, but I view that as a backwards-compatibility kludge we
> should be looking to move away from, not something we want to further
> bake in.  However, I'm out of time to bikeshed on this issue, so fix
> it however you like.

Well, actually, having looked at the proposed patch in context I now
agree with Shigeru-san's fix:
   /*    * For compatibility with prior releases, we don't complain if ALTER TABLE    * or ALTER INDEX is used to
renamesome other type of relation.  But    * ALTER SEQUENCE/VIEW/FOREIGN TABLE are only to be used with relations of
*that type.    */   if (reltype == OBJECT_SEQUENCE && relkind != RELKIND_SEQUENCE)       ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),               errmsg("\"%s\" is not a sequence",
RelationGetRelationName(targetrelation))));
   if (reltype == OBJECT_VIEW && relkind != RELKIND_VIEW)       ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),               errmsg("\"%s\" is not a view",
RelationGetRelationName(targetrelation))));
   if (reltype == OBJECT_FOREIGN_TABLE && relkind != RELKIND_FOREIGN_TABLE)       ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),               errmsg("\"%s\" is not a foreign table",
RelationGetRelationName(targetrelation)),               errhint("Use ALTER FOREIGN TABLE instead.")));
 
   /*    * Don't allow ALTER TABLE on composite types. We want people to use ALTER    * TYPE for that.    */   if
(relkind== RELKIND_COMPOSITE_TYPE)       ereport(ERROR,               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
 errmsg("\"%s\" is a composite type",                       RelationGetRelationName(targetrelation)),
errhint("UseALTER TYPE instead.")));
 

If we haven't felt a need for HINTs for the ALTER SEQUENCE or ALTER VIEW
cases, it seems unlikely that we need one for ALTER FOREIGN TABLE.
Probably whoever wrote this was analogizing to the ALTER TABLE/TYPE case
after it, but that's not the same kind of situation, as evidenced by the
fact that the primary error message is worded differently.
        regards, tom lane