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

On Tue, Jun 28, 2016 at 3:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm planning to apply the attached revision as soon as I get done
> back-porting it.  Main differences from your version:

Thanks for looking at that!

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

OK. I have not skipped the rewrite phase in the case of my patch
because my thoughts of the moment were potential side damages with
rules. But after playing more with it I am not seeing any problems in
skipping it.

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

OK. No problems with that, my point was indeed to ease backpatching.
That's an exercise difficult enough, there is no need to make it more
difficult.

> * 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;

Good idea. I haven't thought about triggering an error to be honest. I
got in mind first to create a plpgsql function that raises a NOTICE
message to show that the thing got planned or not, but gave up as that
was proving to be ugly for the purpose.

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

OK. That's really neater this way.

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

Yes, agreed.

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

Nah, that does not seem worth it to me..
--
Michael