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+27-8
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+9-9
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