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

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> 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.

I'm planning to apply the attached revision as soon as I get done
back-porting it. Main differences from your version:

* We can, and I think should, skip the rewriting phase too in the WITH NO
DATA case. Rewriting should never change a query's exposed result
rowtype, and any other side-effects it might have are likely to be bad
for our purposes.

* Rather than add a goto, I put the existing code sequence into the if's
else block. This will probably cause me some back-patching pain, but
I don't like uglifying code just to make back-patch simpler.

* The regression test cases you added seem not entirely on point, because
they pass just fine against HEAD. I don't object to them, but I added
this to exercise the desired behavior change:

-- make sure that create WITH NO DATA does not plan the query (bug #13907)
create materialized view mvtest_error as select 1/0 as x; -- fail
create materialized view mvtest_error as select 1/0 as x with no data;
refresh materialized view mvtest_error; -- fail here
drop materialized view mvtest_error;

* I also got rid of the static variable CreateAsReladdr, which while
not related to the immediate problem is ugly and dangerous. (It'd
cause a failure in a nested-CREATE-TABLE-AS situation, which would
be unusual but surely isn't forbidden.)

I spent a little bit of time wondering whether we couldn't get rid of
having intorel_startup create the relation at all, instead always doing
it in ExecCreateTableAs(). But that doesn't work conveniently for
CREATE TABLE AS EXECUTE, since there's no query tree handy in that case.
You could imagine moving the build-ColumnDefs-from-a-TupleDesc logic
to someplace else that's only used for CREATE TABLE AS EXECUTE, but
it's not clear to me that it's worth further refactoring just to make
the WITH DATA and WITH NO DATA cases a bit more alike.

regards, tom lane