The command tag of "ALTER MATERIALIZED VIEW RENAME COLUMN"
Hi,
The command tag of ALTER MATERIALIZED VIEW is basically
"ALTER MATERIALIZED VIEW". For example,
=# ALTER MATERIALIZED VIEW test ALTER COLUMN j SET STATISTICS 100;
ALTER MATERIALIZED VIEW
=# ALTER MATERIALIZED VIEW test OWNER TO CURRENT_USER;
ALTER MATERIALIZED VIEW
=# ALTER MATERIALIZED VIEW test RENAME TO hoge;
ALTER MATERIALIZED VIEW
This is ok and looks intuitive to users. But I found that the command tag of
ALTER MATERIALIZED VIEW RENAME COLUMN is "ALTER TABLE", not "ALTER VIEW".
=# ALTER MATERIALIZED VIEW hoge RENAME COLUMN j TO x;
ALTER TABLE
Is this intentional? Or bug?
Regards,
--
Fujii Masao
Fujii Masao <masao.fujii@gmail.com> writes:
... I found that the command tag of
ALTER MATERIALIZED VIEW RENAME COLUMN is "ALTER TABLE", not "ALTER VIEW".
=# ALTER MATERIALIZED VIEW hoge RENAME COLUMN j TO x;
ALTER TABLE
Is this intentional? Or bug?
Seems like an oversight.
regards, tom lane
On Thu, Oct 31, 2019 at 6:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
... I found that the command tag of
ALTER MATERIALIZED VIEW RENAME COLUMN is "ALTER TABLE", not "ALTER VIEW".=# ALTER MATERIALIZED VIEW hoge RENAME COLUMN j TO x;
ALTER TABLEIs this intentional? Or bug?
Seems like an oversight.
regards, tom lane
The same issue is with ALTER FOREIGN TABLE
# ALTER FOREIGN TABLE ft RENAME COLUMN a to t;
ALTER TABLE
# ALTER MATERIALIZED VIEW mv RENAME COLUMN a to r;
ALTER TABLE
Attached patch fixes that for ALTER VIEW , ALTER MATERIALIZED VIEW and
ALTER FOREIGN TABLE
# ALTER MATERIALIZED VIEW mv RENAME COLUMN a to r;
ALTER MATERIALIZED VIEW
# ALTER FOREIGN TABLE ft RENAME COLUMN a to t;
ALTER FOREIGN TABLE
--
Ibrar Ahmed
Attachments:
001_alter_tag_ibrar_v1.patchapplication/octet-stream; name=001_alter_tag_ibrar_v1.patchDownload
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index c6faa6619d..16699a86ef 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1952,7 +1952,7 @@ UtilityContainsQuery(Node *parsetree)
* This covers most cases where ALTER is used with an ObjectType enum.
*/
static const char *
-AlterObjectTypeCommandTag(ObjectType objtype)
+AlterObjectTypeCommandTag(ObjectType objtype, ObjectType subtype)
{
const char *tag;
@@ -1971,7 +1971,26 @@ AlterObjectTypeCommandTag(ObjectType objtype)
tag = "ALTER COLLATION";
break;
case OBJECT_COLUMN:
- tag = "ALTER TABLE";
+ {
+ switch (subtype)
+ {
+ case OBJECT_TABLE:
+ tag = "ALTER TABLE";
+ break;
+ case OBJECT_FOREIGN_TABLE:
+ tag = "ALTER FOREIGN TABLE";
+ break;
+ case OBJECT_VIEW:
+ tag = "ALTER VIEW";
+ break;
+ case OBJECT_MATVIEW:
+ tag = "ALTER MATERIALIZED VIEW";
+ break;
+ default:
+ tag = "ALTER TABLE";
+ break;
+ }
+ }
break;
case OBJECT_CONVERSION:
tag = "ALTER CONVERSION";
@@ -2405,27 +2424,27 @@ CreateCommandTag(Node *parsetree)
break;
case T_RenameStmt:
- tag = AlterObjectTypeCommandTag(((RenameStmt *) parsetree)->renameType);
+ tag = AlterObjectTypeCommandTag(((RenameStmt *) parsetree)->renameType, ((RenameStmt *) parsetree)->relationType);
break;
case T_AlterObjectDependsStmt:
- tag = AlterObjectTypeCommandTag(((AlterObjectDependsStmt *) parsetree)->objectType);
+ tag = AlterObjectTypeCommandTag(((AlterObjectDependsStmt *) parsetree)->objectType, 0);
break;
case T_AlterObjectSchemaStmt:
- tag = AlterObjectTypeCommandTag(((AlterObjectSchemaStmt *) parsetree)->objectType);
+ tag = AlterObjectTypeCommandTag(((AlterObjectSchemaStmt *) parsetree)->objectType, 0);
break;
case T_AlterOwnerStmt:
- tag = AlterObjectTypeCommandTag(((AlterOwnerStmt *) parsetree)->objectType);
+ tag = AlterObjectTypeCommandTag(((AlterOwnerStmt *) parsetree)->objectType, 0);
break;
case T_AlterTableMoveAllStmt:
- tag = AlterObjectTypeCommandTag(((AlterTableMoveAllStmt *) parsetree)->objtype);
+ tag = AlterObjectTypeCommandTag(((AlterTableMoveAllStmt *) parsetree)->objtype, 0);
break;
case T_AlterTableStmt:
- tag = AlterObjectTypeCommandTag(((AlterTableStmt *) parsetree)->relkind);
+ tag = AlterObjectTypeCommandTag(((AlterTableStmt *) parsetree)->relkind, 0);
break;
case T_AlterDomainStmt:
On Fri, Nov 1, 2019 at 6:34 AM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
On Thu, Oct 31, 2019 at 6:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
... I found that the command tag of
ALTER MATERIALIZED VIEW RENAME COLUMN is "ALTER TABLE", not "ALTER VIEW".=# ALTER MATERIALIZED VIEW hoge RENAME COLUMN j TO x;
ALTER TABLEIs this intentional? Or bug?
Seems like an oversight.
Thanks for the check!
The same issue is with ALTER FOREIGN TABLE
Yes.
Attached patch fixes that for ALTER VIEW , ALTER MATERIALIZED VIEW and ALTER FOREIGN TABLE
You introduced subtype in your patch, but I think it's better and simpler
to just give relationType to AlterObjectTypeCommandTag()
if renaming the columns (i.e., renameType = OBJECT_COLUMN).
To avoid this kind of oversight about command tag, I'd like to add regression
tests to make sure that SQL returns valid and correct command tag.
But currently there seems no mechanism for such test, in regression
test. Right??
Maybe we will need that mechanism.
Regards,
--
Fujii Masao
Attachments:
cmdtag_of_alter_mv_v1.patchapplication/octet-stream; name=cmdtag_of_alter_mv_v1.patchDownload
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index c6faa6619d..3f7ded33d3 100644
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
***************
*** 2405,2411 **** CreateCommandTag(Node *parsetree)
break;
case T_RenameStmt:
! tag = AlterObjectTypeCommandTag(((RenameStmt *) parsetree)->renameType);
break;
case T_AlterObjectDependsStmt:
--- 2405,2418 ----
break;
case T_RenameStmt:
! /*
! * When the column is renamed, the command tag is created
! * from its relation type
! */
! tag = AlterObjectTypeCommandTag(
! ((RenameStmt *) parsetree)->renameType == OBJECT_COLUMN ?
! ((RenameStmt *) parsetree)->relationType :
! ((RenameStmt *) parsetree)->renameType);
break;
case T_AlterObjectDependsStmt:
On Fri, Nov 1, 2019 at 8:00 AM Fujii Masao <masao.fujii@gmail.com> wrote:
On Fri, Nov 1, 2019 at 6:34 AM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
On Thu, Oct 31, 2019 at 6:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
... I found that the command tag of
ALTER MATERIALIZED VIEW RENAME COLUMN is "ALTER TABLE", not "ALTERVIEW".
=# ALTER MATERIALIZED VIEW hoge RENAME COLUMN j TO x;
ALTER TABLEIs this intentional? Or bug?
Seems like an oversight.
Thanks for the check!
The same issue is with ALTER FOREIGN TABLE
Yes.
Attached patch fixes that for ALTER VIEW , ALTER MATERIALIZED VIEW and
ALTER FOREIGN TABLE
You introduced subtype in your patch, but I think it's better and simpler
to just give relationType to AlterObjectTypeCommandTag()
if renaming the columns (i.e., renameType = OBJECT_COLUMN).That's works perfectly along with future oversight about the command tag.
To avoid this kind of oversight about command tag, I'd like to add
regression
tests to make sure that SQL returns valid and correct command tag.
But currently there seems no mechanism for such test, in regression
test. Right??
Do we really need a regression test cases for such small oversights?
Maybe we will need that mechanism.
Regards,
--
Fujii Masao
--
Ibrar Ahmed
On Fri, Nov 01, 2019 at 02:17:03PM +0500, Ibrar Ahmed wrote:
Do we really need a regression test cases for such small oversights?
It is possible to get the command tags using an event trigger... But
that sounds hack-ish.
--
Michael
On Sat, Nov 2, 2019 at 4:40 PM Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Nov 01, 2019 at 02:17:03PM +0500, Ibrar Ahmed wrote:
Do we really need a regression test cases for such small oversights?
It is possible to get the command tags using an event trigger... But
that sounds hack-ish.
Yes, so if simple test mechanism to check command tag exists,
it would be helpful.
I'm thinking to commit the patch. But I have one question; is it ok to
back-patch? Since the patch changes the command tags for some commands,
for example, which might break the existing event trigger functions
using TG_TAG if we back-patch it. Or we should guarantee the compatibility of
command tag within the same major version?
Regards,
--
Fujii Masao
Fujii Masao <masao.fujii@gmail.com> writes:
I'm thinking to commit the patch. But I have one question; is it ok to
back-patch? Since the patch changes the command tags for some commands,
for example, which might break the existing event trigger functions
using TG_TAG if we back-patch it. Or we should guarantee the compatibility of
command tag within the same major version?
I would not back-patch this. I don't think it's enough of a bug
to justify taking any compatibility risks for.
regards, tom lane
On Tue, Nov 5, 2019 at 11:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
I'm thinking to commit the patch. But I have one question; is it ok to
back-patch? Since the patch changes the command tags for some commands,
for example, which might break the existing event trigger functions
using TG_TAG if we back-patch it. Or we should guarantee the compatibility of
command tag within the same major version?I would not back-patch this. I don't think it's enough of a bug
to justify taking any compatibility risks for.
+1
I committed the patch only to the master. Thanks!
Regards,
--
Fujii Masao