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

On Tue, Jul 26, 2016 at 1:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)gmail(dot)com> writes:
>> On Tue, Jul 26, 2016 at 12:32 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> It's possible that the most reasonable fix is to move matview refreshes to
>>> after the ACL restoration step. That would wreak a lot of havoc with
>>> the current view of a dump as being tripartite (pre-data/data/post-data),
>>> so it might be more work than we want to do. But it seems like the
>>> logically soundest thing.
>
>> That is exactly what the patch I posted yesterday does. Peter was
>> suggesting that wasn't good enough
>
> Well, he's right: just minimally rearranging the dump order is not enough.
> For one thing, I'm pretty sure this patch will fail to fix the bug in a
> parallel restore, because restore_toc_entries_parallel has hard-wired
> knowledge that ACLs can be done last and serially.

I think I covered that. I tested all dump formats, including
parallel; but it is possible there was something wrong with my
testing.

> Even if the fix works
> in parallel restore, it would result in doing matview refreshes serially
> not in parallel, which is a pretty tough nut to swallow.

ok

> And I'm not
> really convinced that the patch preserves dependency ordering requirements
> at all, even in plain serial restore.

Pre-patch, it passes the sorted objects twice -- once for
everything except ACLs and again for ACLs. I just made sure that
refreshes sorted past ACLs and moved them to the ACL phase. I
don't see why that would disturb the order in which the objects are
passed either time; just what the filtering is on each pass.

> The core of the problem here, I think, is that pg_dump has never had any
> real notion of dependency ordering for ACLs, since it's always been
> sufficient to dump them at the very end in arbitrary order. If we're now
> going to make objects-with-dependencies also dependent on ACLs, I'm afraid
> that raises the bar quite a lot in terms of needing to treat ACLs as
> real, dependency-sorted objects.

I was wondering why we went outside the normal ordering logic for
ACLs -- seems like a pretty ugly hack, but I didn't figure it was
the job of this patch to fix that.

> I believe the thrust of Peter's suggestion is to try to avoid biting that
> bullet, by instead instituting a rule of "dump an object's ACL immediately
> after the object". But to make this work with the read-only-table
> scenario, it would have to be something like "dump an object's GRANTs
> immediately after the object, but if there are any self-revokes, dump
> those last as we always have".

I see what you're saying, but read-only-tables aren't the problem.
The only problem this patch leaves, I think, is that a matview for
which objects it depends on have changed since the last REFRESH (or
CREATE WITH DATA) might not be able to refresh at the end; but I
don't see how you avoid that with policies, functions, or views --
so I'm not clear why we care more about ACLs in that regard.

> With either approach, I'm afraid we're talking about a patch much larger
> and more invasive than what's here :-(

Well, this allows the example from the bug report to run
successfully. It seems to me to generate dumps that are clearly
better that what are generated without the patch; so perhaps this
is worthwhile for now (with more appropriate tests, of course)
while we sort out the perfect solution for somewhere down the line.

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