Horton

Thread

Commits

  1. Correct the command tags for ALTER ... RENAME COLUMN.

  1. The command tag of "ALTER MATERIALIZED VIEW RENAME COLUMN"

    Fujii Masao <masao.fujii@gmail.com> — 2019-10-31T10:38:35Z

    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

  2. Re: The command tag of "ALTER MATERIALIZED VIEW RENAME COLUMN"

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-31T13:56:17Z

    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

  3. Re: The command tag of "ALTER MATERIALIZED VIEW RENAME COLUMN"

    Ibrar Ahmed <ibrar.ahmad@gmail.com> — 2019-10-31T21:33:53Z

    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.
>
>                         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

  4. Re: The command tag of "ALTER MATERIALIZED VIEW RENAME COLUMN"

    Fujii Masao <masao.fujii@gmail.com> — 2019-11-01T03:00:31Z

    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

  5. Re: The command tag of "ALTER MATERIALIZED VIEW RENAME COLUMN"

    Ibrar Ahmed <ibrar.ahmad@gmail.com> — 2019-11-01T09:17:03Z

    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 "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).
>
> 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

  6. Re: The command tag of "ALTER MATERIALIZED VIEW RENAME COLUMN"

    Michael Paquier <michael@paquier.xyz> — 2019-11-02T07:40:35Z

    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

  7. Re: The command tag of "ALTER MATERIALIZED VIEW RENAME COLUMN"

    Fujii Masao <masao.fujii@gmail.com> — 2019-11-05T05:39:51Z

    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

  8. Re: The command tag of "ALTER MATERIALIZED VIEW RENAME COLUMN"

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-05T14:19:20Z

    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

  9. Re: The command tag of "ALTER MATERIALIZED VIEW RENAME COLUMN"

    Fujii Masao <masao.fujii@gmail.com> — 2019-11-06T03:56:44Z

    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