Re: BUG #13907: Restore materialized view throw permission denied

On Fri, Jun 17, 2016 at 1:28 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> So, I have been able to build the attached WIP patch proving that this
>> is able to work correctly. There is no real refactoring done yet, but
>> this passes regression tests and tutti-quanti. By the way, there are
>> three points I am wondering about:
>
>> 1) EXPLAIN ANALYZE is able to work with CTAS and create matview. I am
>> thinking that it would be better not to touch those to not impact
>> existing applications. By that I mean that EXPLAIN CREATE MATVIEW WITH
>> NO DATA would still run the planner and executor in explain.c
>
> Agreed, that needs to not break.

So, left untouched.

>> 2) CTAS has a WITH NO DATA option, and it would be really weird to use
>> the planner/executor code path when this option is used for this case.
>> So I'd like to use the same method for both matviews and normal
>> relations to simplify things and make the code more consistent.
>
> Seems reasonable, depending on how invasive you have to be.

Check. It is actually not that invasive. At least I have found that
this is what this code should do naturally.

>> 3) In this WIP patch, the command tag is CREATE MATERIALIZED VIEW if
>> WITH NO DATA is used. I am planning to use SELECT 0 in all cases to
>> keep things consistent with what is on HEAD and back-branches.
>
> Meh, can't get excited about that. If it's easy, okay, but arguably
> the current behavior is just an implementation artifact itself.
> I wouldn't go far out of your way to keep it.

Okay, I just suggested that because I thought people would care about
it. A couple of years back when rewriting CTAS on a fork of Postgres I
got complains from users regarding such a change because that was not
consistent :) Not doing it makes the code more simple and readable, so
let's go with the normal command tags then.

Attached is a new patch, with the promised refactoring, more
regression tests, etc. After some thoughts, I have arrived to the
conclusion that it is better to limit the footprint of this patch in
views.c. So I have created a routine makeColumnDef that is used for
views, ctas and matviews, but I am letting the creation of the column
definition list separated as each code path has slight differences
when building it. Things could be made more shared on HEAD but that
would be really intrusive for back branches, and I have kept that in
mind for this patch.

Comments?
--
Michael