Thread: Re: [BUGS] BUG #8542: Materialized View with another column_name does not work?

"t.katsumata1122@gmail.com" <t.katsumata1122@gmail.com> wrote:

> I'm testing the Materialized View.
> When I've tried to create materialized view with specified
> column_name, I got an ERROR.
>
> example:
> - Creating original table
> CREATE TABLE t ( i int );
>
> - Creating materialized view with column_name
> CREATE MATERIALIZED VIEW mv_t(ii) AS SELECT * FROM t;
>
> And then, I got a bellow ERROR.
> ----
> ERROR:  SELECT rule's target entry 1 has different column name from "ii"
> ----
>
> I did not get any ERROR with non materialized view.
> CREATE VIEW mv_t(ii) AS SELECT * FROM t;
>
> Is this a bug or restriction for Materialized View?

It's a bug.  Will fix in the next 9.3 minor release.

Moving the discussion to the -hackers list to discuss the fix.

This bug was introduced in fb60e7296c2cf15195802b4596496b179bdc905a
based on this feedback:

http://www.postgresql.org/message-id/20600.1363022702@sss.pgh.pa.us

I picked the wrong response to that feedback.  Attached is a patch
which fixes things along the alternative lines suggested.  This
includes a regression test to ensure that this doesn't get broken
again.

If there are no objections I'll apply this within a few days.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Oct 31, 2013 at 10:22 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
> "t.katsumata1122@gmail.com" <t.katsumata1122@gmail.com> wrote:
> If there are no objections I'll apply this within a few days.
I am not sure that adding a boolean flag introducing a concept related
to matview inside checkRuleResultList is the best approach to solve
that. checkRuleResultList is something related only to rules, and has
nothing related to matviews in it yet. I have been pondering myself
about this issue, and wouldn't it be better to directly enforce
targetList in checkRuleResultList call at an upper level with the
column name list given by users if there is any instead of the list of
columns of the SELECT query?

After looking closely at the code this looks to be a better approach
from a general viewpoint. I didn't got the time to write a patch but
this is the conclusion I reached after some analysis at least...
-- 
Michael

<div dir="ltr">CREATE MATERIALIZED VIEW statement ends up being CREATE TABLE AS statement underneath with table type
matview.In that case, why don't I see special treatment only for materialized view and not CTAS in general, which
allowscolumn names to specified like the case in the bug reported.<br /></div><div class="gmail_extra"><br /><br /><div
class="gmail_quote">OnFri, Nov 1, 2013 at 3:52 AM, Kevin Grittner <span dir="ltr"><<a
href="mailto:kgrittn@ymail.com"target="_blank">kgrittn@ymail.com</a>></span> wrote:<br /><blockquote
class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">"<a
href="mailto:t.katsumata1122@gmail.com">t.katsumata1122@gmail.com</a>"<<a
href="mailto:t.katsumata1122@gmail.com">t.katsumata1122@gmail.com</a>>wrote:<br /><br /> > I'm testing the
MaterializedView.<br /> > When I've tried to create materialized view with specified<br /> > column_name, I got
anERROR.<br /> ><br /> > example:<br /> > - Creating original table<br /> > CREATE TABLE t ( i int );<br />
><br/> > - Creating materialized view with column_name<br /> > CREATE MATERIALIZED VIEW mv_t(ii) AS SELECT *
FROMt;<br /> ><br /> > And then, I got a bellow ERROR.<br /> > ----<br /> > ERROR:  SELECT rule's target
entry1 has different column name from "ii"<br /> > ----<br /> ><br /> > I did not get any ERROR with non
materializedview.<br /> > CREATE VIEW mv_t(ii) AS SELECT * FROM t;<br /> ><br /> > Is this a bug or
restrictionfor Materialized View?<br /><br /> It's a bug.  Will fix in the next 9.3 minor release.<br /><br /> Moving
thediscussion to the -hackers list to discuss the fix.<br /><br /> This bug was introduced in
fb60e7296c2cf15195802b4596496b179bdc905a<br/> based on this feedback:<br /><br /><a
href="http://www.postgresql.org/message-id/20600.1363022702@sss.pgh.pa.us"
target="_blank">http://www.postgresql.org/message-id/20600.1363022702@sss.pgh.pa.us</a><br/><br /> I picked the wrong
responseto that feedback.  Attached is a patch<br /> which fixes things along the alternative lines suggested.  This<br
/>includes a regression test to ensure that this doesn't get broken<br /> again.<br /><br /> If there are no objections
I'llapply this within a few days.<br /><br /> --<br /> Kevin Grittner<br /> EDB: <a href="http://www.enterprisedb.com"
target="_blank">http://www.enterprisedb.com</a><br/> The Enterprise PostgreSQL Company<br /><br /> --<br /> Sent via
pgsql-hackersmailing list (<a href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br /> To
makechanges to your subscription:<br /><a href="http://www.postgresql.org/mailpref/pgsql-hackers"
target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/><br /></blockquote></div><br /><br clear="all"
/><br/>-- <br /><div dir="ltr">Best Wishes,<br />Ashutosh Bapat<br />EnterpriseDB Corporation<br />The Postgres
DatabaseCompany<br /></div></div> 

Michael Paquier <michael.paquier@gmail.com> wrote:

> I am not sure that adding a boolean flag introducing a concept
> related to matview inside checkRuleResultList is the best
> approach to solve that. checkRuleResultList is something related
> only to rules, and has nothing related to matviews in it yet.

Well, I was tempted to keep that concept in DefineQueryRewrite()
where the call is made, by calling  the new bool something like
requireColumnNameMatch and not having checkRuleResultList()
continue to use isSelect for that purpose at all.
DefineQueryRewrite() already references RELKIND_RELATION once,
RELKIND_MATVIEW twice, and RELKIND_VIEW three times, so it would
hardly be introducing a new concept there.  I did it the way I did
in the posted patch because it seemed to more nearly match what Tom
was suggesting.

> I have been pondering myself about this issue, and wouldn't it be
> better to directly enforce targetList in checkRuleResultList call
> at an upper level with the column name list given by users if
> there is any instead of the list of columns of the SELECT query?
>
> After looking closely at the code this looks to be a better
> approach from a general viewpoint. I didn't got the time to write
> a patch but this is the conclusion I reached after some analysis
> at least...

That doesn't sound like something we could back-patch.

Anyway, I'm not sure why that would be better.  We have rewrite
rules on three kinds of relations -- tables, views, and
materialized views.  In general, a materialized view tends to
support the properties of both a view and a table, with a few
points in the rewrite code that need to pay attention to which kind
of relation the rule applies to.  Among the various validations for
the rewrite, this one validation (matching column names) does not
apply to a non-SELECT rule with a RETURNING clause or to a SELECT
rule which populates a materialized view.  If you were to move the
code to exclude this validation for the latter case, wouldn't you
also need to move the validation for the former case?  Where would
you put that?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote:

> CREATE MATERIALIZED VIEW statement ends up being CREATE TABLE AS
> statement underneath with table type matview. In that case, why
> don't I see special treatment only for materialized view and not
> CTAS in general, which allows column names to specified like the
> case in the bug reported.

While the initial population of the matview is a lot like CTAS (and
so far shares some code), the critical difference is that CTAS
doesn't store a rule to support repopulating the table.  This issue
comes up in the validation of that rule for the matview.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn@ymail.com> wrote:
> Michael Paquier <michael.paquier@gmail.com> wrote:
>
>> I am not sure that adding a boolean flag introducing a concept
>> related to matview inside checkRuleResultList is the best
>> approach to solve that. checkRuleResultList is something related
>> only to rules, and has nothing related to matviews in it yet.
>
> Well, I was tempted to keep that concept in DefineQueryRewrite()
> where the call is made, by calling  the new bool something like
> requireColumnNameMatch and not having checkRuleResultList()
> continue to use isSelect for that purpose at all.
> DefineQueryRewrite() already references RELKIND_RELATION once,
> RELKIND_MATVIEW twice, and RELKIND_VIEW three times, so it would
> hardly be introducing a new concept there.

Upon reflection, that seemed to be cleaner.  Pushed fix that way.
Not much of a change from the previously posted patch, but attached
here in case anyone wants to argue against this approach.

Thanks for the report!

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company