ATT_FOREIGN_TABLE and ATWrongRelkindError()

Started by Amit Langoteabout 10 years ago9 messages

Langote_Amit_f8@lab.ntt.co.jp

about 10 years ago

1 attachment(s)

Hi,

This may just be nitpicking but I noticed that ATWrongRelkindError() could
emit a better message in case of such errors during ALTER COLUMN DEFAULT
and ALTER COLUMN SET STORAGE than "%s is of the wrong type" which is what
it would emit now. Just need to add a couple of cases to the switch there:

+ case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
+     msg = _("\"%s\" is not a table, view or foreign table");
+     break;

+ case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
+     msg = _("\"%s\" is not a table, materialized view, or foreign table");
+     break;

Attached adds those.

Thanks,
Amit

Attachments:

at-wrong-relkind-error-fix.patchtext/x-diff; name=at-wrong-relkind-error-fix.patchDownload

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 403582c..a5bc508 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4336,6 +4336,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 		case ATT_TABLE | ATT_VIEW:
 			msg = _("\"%s\" is not a table or view");
 			break;
+		case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
+			msg = _("\"%s\" is not a table, view or foreign table");
+			break;
 		case ATT_TABLE | ATT_VIEW | ATT_MATVIEW | ATT_INDEX:
 			msg = _("\"%s\" is not a table, view, materialized view, or index");
 			break;
@@ -4345,6 +4348,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 		case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
 			msg = _("\"%s\" is not a table, materialized view, or index");
 			break;
+		case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
+			msg = _("\"%s\" is not a table, materialized view, or foreign table");
+			break;
 		case ATT_TABLE | ATT_FOREIGN_TABLE:
 			msg = _("\"%s\" is not a table or foreign table");
 			break;

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: Amit Langote (#1)

Re: ATT_FOREIGN_TABLE and ATWrongRelkindError()

On Wed, Oct 21, 2015 at 1:51 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

This may just be nitpicking but I noticed that ATWrongRelkindError() could
emit a better message in case of such errors during ALTER COLUMN DEFAULT
and ALTER COLUMN SET STORAGE than "%s is of the wrong type" which is what
it would emit now. Just need to add a couple of cases to the switch there:
+ case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
+     msg = _("\"%s\" is not a table, view or foreign table");
+     break;
+ case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
+     msg = _("\"%s\" is not a table, materialized view, or foreign table");
+     break;
Attached adds those.

Good catch. Committed and back-patched to 9.5.

(Yes, one of these problems goes back to 9.3, but it's a minor issue
so I didn't bother. If someone really feels the urge, have at it.)

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

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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

about 10 years ago

In reply to: Robert Haas (#2)

Re: ATT_FOREIGN_TABLE and ATWrongRelkindError()

On 2015/10/23 6:06, Robert Haas wrote:

On Wed, Oct 21, 2015 at 1:51 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Attached adds those.

Good catch. Committed and back-patched to 9.5.

(Yes, one of these problems goes back to 9.3, but it's a minor issue
so I didn't bother. If someone really feels the urge, have at it.)

Thanks, Robert!

Regards,
Amit

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

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

about 10 years ago

In reply to: Robert Haas (#2)

1 attachment(s)

Re: ATT_FOREIGN_TABLE and ATWrongRelkindError()

On 2015/10/23 6:06, Robert Haas wrote:

On Wed, Oct 21, 2015 at 1:51 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
This may just be nitpicking but I noticed that ATWrongRelkindError() could
emit a better message in case of such errors during ALTER COLUMN DEFAULT
and ALTER COLUMN SET STORAGE than "%s is of the wrong type" which is what
it would emit now. Just need to add a couple of cases to the switch there:
+ case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
+     msg = _("\"%s\" is not a table, view or foreign table");
+     break;
+ case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
+     msg = _("\"%s\" is not a table, materialized view, or foreign table");
+     break;
Attached adds those.

Good catch. Committed and back-patched to 9.5.

Thanks, Amit and Robert!

This is really really nitpicking, but I noticed that there is an
implicit rule concerning the message format in ATWrongRelkindError; if
more than two objects are present, the message is "\"%s\" is not a foo,
bar, or baz". ("or" is preceded by a comma!) So, would it be better
that the former is "\"%s\" is not a table, view, or foreign table"?

BTW, I found an incorrect error message in ATWrongRelkindError.
Attached is a patch for fixing the message.

Best regards,
Etsuro Fujita

Attachments:

ATWrongRelkindError-message.patchtext/x-patch; name=ATWrongRelkindError-message.patchDownload

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a5bc508..6436d0c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4358,7 +4358,7 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 			msg = _("\"%s\" is not a table, composite type, or foreign table");
 			break;
 		case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE:
-			msg = _("\"%s\" is not a table, materialized view, composite type, or foreign table");
+			msg = _("\"%s\" is not a table, materialized view, index, or foreign table");
 			break;
 		case ATT_VIEW:
 			msg = _("\"%s\" is not a view");

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

about 10 years ago

In reply to: Etsuro Fujita (#4)

Re: ATT_FOREIGN_TABLE and ATWrongRelkindError()

On 2015/10/23 18:51, Etsuro Fujita wrote:

On 2015/10/23 6:06, Robert Haas wrote:

Good catch. Committed and back-patched to 9.5.

Thanks, Amit and Robert!

This is really really nitpicking, but I noticed that there is an implicit
rule concerning the message format in ATWrongRelkindError; if more than
two objects are present, the message is "\"%s\" is not a foo, bar, or
baz". ("or" is preceded by a comma!) So, would it be better that the
former is "\"%s\" is not a table, view, or foreign table"?

Oops! Yeah, I missed the comma there. That seems like a generally
preferred punctuation rule (the comma before conjunction(s) I mean).

Thanks,
Amit

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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

about 10 years ago

In reply to: Amit Langote (#5)

1 attachment(s)

Re: ATT_FOREIGN_TABLE and ATWrongRelkindError()

On 2015/10/23 19:02, Amit Langote wrote:

On 2015/10/23 18:51, Etsuro Fujita wrote:

This is really really nitpicking, but I noticed that there is an implicit
rule concerning the message format in ATWrongRelkindError; if more than
two objects are present, the message is "\"%s\" is not a foo, bar, or
baz". ("or" is preceded by a comma!) So, would it be better that the
former is "\"%s\" is not a table, view, or foreign table"?

Oops! Yeah, I missed the comma there. That seems like a generally
preferred punctuation rule (the comma before conjunction(s) I mean).

Here is a patch rectifying that mistake.

Thanks,
Amit

Attachments:

add-comma-before-or.patchtext/x-diff; name=add-comma-before-or.patchDownload

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a5bc508..58a7313 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4337,7 +4337,7 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 			msg = _("\"%s\" is not a table or view");
 			break;
 		case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
-			msg = _("\"%s\" is not a table, view or foreign table");
+			msg = _("\"%s\" is not a table, view, or foreign table");
 			break;
 		case ATT_TABLE | ATT_VIEW | ATT_MATVIEW | ATT_INDEX:
 			msg = _("\"%s\" is not a table, view, materialized view, or index");

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: Etsuro Fujita (#4)

Re: ATT_FOREIGN_TABLE and ATWrongRelkindError()

On Fri, Oct 23, 2015 at 11:51 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

BTW, I found an incorrect error message in ATWrongRelkindError. Attached is
a patch for fixing the message.

Committed and back-patched to 9.3.

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

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

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: Amit Langote (#6)

Re: ATT_FOREIGN_TABLE and ATWrongRelkindError()

On Mon, Oct 26, 2015 at 3:12 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2015/10/23 19:02, Amit Langote wrote:

On 2015/10/23 18:51, Etsuro Fujita wrote:

This is really really nitpicking, but I noticed that there is an implicit
rule concerning the message format in ATWrongRelkindError; if more than
two objects are present, the message is "\"%s\" is not a foo, bar, or
baz". ("or" is preceded by a comma!) So, would it be better that the
former is "\"%s\" is not a table, view, or foreign table"?

Oops! Yeah, I missed the comma there. That seems like a generally
preferred punctuation rule (the comma before conjunction(s) I mean).

Here is a patch rectifying that mistake.

Committed.

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

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

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

about 10 years ago

In reply to: Robert Haas (#7)

Re: ATT_FOREIGN_TABLE and ATWrongRelkindError()

On 2015/10/28 20:10, Robert Haas wrote:

On Fri, Oct 23, 2015 at 11:51 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

BTW, I found an incorrect error message in ATWrongRelkindError. Attached is
a patch for fixing the message.

Committed and back-patched to 9.3.

Thanks!

Best regards,
Etsuro Fujita

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