Thread: 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 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 TABLE
>>
>> > Is 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

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