Re: Allow CREATE OR REPLACE VIEW to rename the columns - Mailing list pgsql-hackers

On Thu, Oct 31, 2019 at 9:34 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
>
>
>
> On Thu, Oct 31, 2019 at 5:28 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
>>
>>
>>
>> On Thu, Oct 31, 2019 at 5:11 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
>>>
>>>
>>>
>>> On Thu, Oct 31, 2019 at 5:01 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>>>>
>>>> On Thu, Oct 31, 2019 at 7:59 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
>>>> >
>>>> >
>>>> >
>>>> > On Thu, Oct 31, 2019 at 12:32 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>>>> >>
>>>> >> On Thu, Oct 31, 2019 at 1:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> >> >
>>>> >> > Fujii Masao <masao.fujii@gmail.com> writes:
>>>> >> > > Currently CREATE OR REPLACE VIEW command fails if the column names
>>>> >> > > are changed.
>>>> >> >
>>>> >> > That is, I believe, intentional.  It's an effective aid to catching
>>>> >> > mistakes in view redefinitions, such as misaligning the new set of
>>>> >> > columns relative to the old.  That's particularly important given
>>>> >> > that we allow you to add columns during CREATE OR REPLACE VIEW.
>>>> >> > Consider the oversimplified case where you start with
>>>> >> >
>>>> >> > CREATE VIEW v AS SELECT 1 AS x, 2 AS y;
>>>> >> >
>>>> >> > and you want to add a column z, and you get sloppy and write
>>>> >> >
>>>> >> > CREATE OR REPLACE VIEW v AS SELECT 1 AS x, 3 AS z, 2 AS y;
>>>> >> >
>>>> >> > If we did not throw an error on this, references that formerly
>>>> >> > pointed to column y would now point to z (as that's still attnum 2),
>>>> >> > which is highly unlikely to be what you wanted.
>>>> >>
>>>> >> This example makes me wonder if the addtion of column by
>>>> >> CREATE OR REPLACE VIEW also has the same (or even worse) issue.
>>>> >> That is, it may increase the oppotunity for users' mistake.
>>>> >> I'm thinking the case where users mistakenly added new column
>>>> >> into the view when replacing the view definition. This mistake can
>>>> >> happen because CREATE OR REPLACE VIEW allows new column to
>>>> >> be added. But what's the worse is that, currently there is no way to
>>>> >> drop the column from the view, except recreation of the view.
>>>> >> Neither CREATE OR REPLACE VIEW nor ALTER TABLE support
>>>> >> the drop of the column from the view. So, to fix the mistake,
>>>> >> users would need to drop the view itself and recreate it. If there are
>>>> >> some objects depending the view, they also might need to be recreated.
>>>> >> This looks not good. Since the feature has been supported,
>>>> >> it's too late to say that, though...
>>>> >>
>>>> >> At least, the support for ALTER VIEW DROP COLUMN might be
>>>> >> necessary to alleviate that situation.
>>>> >>
>>>> >
>>>> > - Is this intentional not implemented the "RENAME COLUMN" statement for
>>>> > VIEW because it is implemented for Materialized View?
>>>>
>>>> Not sure that, but Tom's suggestion to support ALTER VIEW RENAME COLUMN
>>>> sounds reasonable whether we support the rename of columns when replacing
>>>> the view definition, or not. Attached is the patch that adds support for
>>>> ALTER VIEW RENAME COLUMN command.
>>>>
>>>> > I have made just a similar
>>>> > change to view and it works.
>>>>
>>>> Yeah, ISTM that we made the same patch at the same time. You changed gram.y,
>>>> but I added the following changes additionally.
>>>>
>>>> - Update the doc
>>>> - Add HINT message emit when CRAETE OR REPLACE VIEW fails to rename the columns
>>>> - Update tab-complete.c
>>>> - Add regression test
>>>>
>>>
>>> Oh, I just sent the patch to ask, good you do that in all the places.
>>>
>>>> One issue I've not addressed yet is about the command tag of
>>>> "ALTER VIEW RENAME COLUMN". Currently "ALTER TABLE" is returned as the tag
>>>> like ALTER MATERIALIZED VIEW RENAME COLUMN, but ISTM that "ALTER VIEW"
>>>> is better. I started the discussion about the command tag of
>>>> "ALTER MATERIALIZED VIEW" at another thread. I will update the patch according
>>>> to the result of that discussion.
>>>> https://postgr.es/m/CAHGQGwGUaC03FFdTFoHsCuDrrNvFvNVQ6xyd40==P25WvuBJjg@mail.gmail.com
>>>>
>>> Attached patch contain small change for ALTER MATERIALIZED VIEW.
>>>
>>
>> Hmm, my small change of "ALTER MATERIALIZED VIEW" does not work in some cases need more work on that.
>>
>
>
> The AlterObjectTypeCommandTag function just take one parameter, but to
> show  "ALTER MATERIALIZED VIEW" instead of ALTER TABLE we need to
> pass "relationType = OBJECT_MATVIEW" along with "renameType = OBJECT_COLUMN"
> and handle that in the function. The "AlterObjectTypeCommandTag" function has many
> calls. Do you think just for the command tag, we should do all this change?

Thanks for trying to address the issue!
As probably you've already noticed, the commit 979766c0af fixed the issue
thanks to your review. So we can review the patch that I posted.

Regards,

-- 
Fujii Masao