BUG #13907: Restore materialized view throw permission denied

On Tue, Feb 2, 2016 at 7:14 PM, <marian.krucina@gmail.com> wrote:

restore (9.4.5, 9.5.0) or pg_upgrade (9.4.5 to 9.5.0) fail on CREATE
MATERIALIZED VIEW.
This is similar to:
/messages/by-id/11166.1424357659@sss.pgh.pa.us

Problem is, when view runs as user definer.
Is possible move 'CREATE MATERIALIZED VIEW' in a dump to end?

This is one of those issues where it would be cool to only plan and
execute the query creating the materialized view query with NO DATA
without planning and executing it first, and do the actual planning
and execution at the first refresh. A similar problem was discussed
here:
/messages/by-id/20160115175546.2968.6033@wrigleys.postgresql.org

Thought I don't think that it is that straight-forward, the relation
defined using a CTAS uses column information directly derived from the
query planned, so the fix would be to extract the necessary
information for those columns: collation, column name, type OID and
typemod without the need of the existing routines.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

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

This is one of those issues where it would be cool to only plan and
execute the query creating the materialized view query with NO DATA
without planning and executing it first, and do the actual planning
and execution at the first refresh. A similar problem was discussed
here:
/messages/by-id/20160115175546.2968.6033@wrigleys.postgresql.org

Thought I don't think that it is that straight-forward, the relation
defined using a CTAS uses column information directly derived from the
query planned, so the fix would be to extract the necessary
information for those columns: collation, column name, type OID and
typemod without the need of the existing routines.

I think you want to be looking at the way that CREATE VIEW accomplishes
the same task. See DefineView and DefineVirtualRelation. It might be
worth refactoring those a bit to allow code sharing.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Tue, Jun 14, 2016 at 10:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think you want to be looking at the way that CREATE VIEW accomplishes
the same task. See DefineView and DefineVirtualRelation. It might be
worth refactoring those a bit to allow code sharing.

Ah, right! I forgot that views already solve this problem in its way,
and that's definitely something that we should do.

[code lookup]

OK I see now how to wrap that. I'll come up with something that can be
back-patched and think about the refactoring carefully.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Wed, Jun 15, 2016 at 4:37 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Tue, Jun 14, 2016 at 10:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think you want to be looking at the way that CREATE VIEW accomplishes
the same task. See DefineView and DefineVirtualRelation. It might be
worth refactoring those a bit to allow code sharing.

Ah, right! I forgot that views already solve this problem in its way,
and that's definitely something that we should do.

[code lookup]

OK I see now how to wrap that. I'll come up with something that can be
back-patched and think about the refactoring carefully.

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

Any objections to those points? If there are none, I'll move ahead
with a more intelligent and refactored patch.
--
Michael

Michael Paquier <michael.paquier@gmail.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.

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.

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.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Fri, Jun 17, 2016 at 1:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael.paquier@gmail.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

On Fri, Jun 17, 2016 at 2:59 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

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?

Patch is added to the next commit fest. I don't want this thing to be
lost in translation:
https://commitfest.postgresql.org/10/652/
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Michael Paquier <michael.paquier@gmail.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

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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Mon, Jun 27, 2016 at 1:35 PM, 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.

I'm thinking of applying something like the attached to fix the
rest of this bug.

Thoughts?

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

On 7/25/16 4:09 PM, Kevin Grittner wrote:

On Mon, Jun 27, 2016 at 1:35 PM, 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.

I'm thinking of applying something like the attached to fix the
rest of this bug.

The reason that ACLs are restored last is that they could contain owner
self-revokes. So anything that you run after the ACLs could fail
because of that. I think a more complete fix would be to split up the
ACL restores into the general part, which you would run right after the
object is restored, and the owner revokes, which you would run last.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Mon, Jul 25, 2016 at 8:37 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 7/25/16 4:09 PM, Kevin Grittner wrote:

On Mon, Jun 27, 2016 at 1:35 PM, 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.

I'm thinking of applying something like the attached to fix the
rest of this bug.

The reason that ACLs are restored last is that they could contain owner
self-revokes. So anything that you run after the ACLs could fail
because of that. I think a more complete fix would be to split up the
ACL restores into the general part, which you would run right after the
object is restored, and the owner revokes, which you would run last.

So you are suggesting that restoring from pg_dump output should
generate materialized view data under a different security context
than would be used by a REFRESH statement on the source database?

Anything you can add to clarify what you mean by "owner
self-revokes" and why the security should be different when the
matview data is refreshed at the end of the restore should be
different from what would happen on the database which was backed
up might be helpful.

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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Kevin Grittner <kgrittn@gmail.com> writes:

So you are suggesting that restoring from pg_dump output should
generate materialized view data under a different security context
than would be used by a REFRESH statement on the source database?

Yes. Consider the following simple example (done by a non-superuser
named joe):

create table joes_table(f1 int);
insert into joes_table values(1);
revoke insert on joes_table from joe;

pg_dump is required to be able to restore the state of this table
correctly. It will fail to do so if it issues the revoke before
loading data. The same issue applies to all data loading,
including refreshing matviews.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Tue, Jul 26, 2016 at 8:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Kevin Grittner <kgrittn@gmail.com> writes:

So you are suggesting that restoring from pg_dump output should
generate materialized view data under a different security context
than would be used by a REFRESH statement on the source database?

Yes. Consider the following simple example (done by a non-superuser
named joe):

create table joes_table(f1 int);
insert into joes_table values(1);
revoke insert on joes_table from joe;

pg_dump is required to be able to restore the state of this table
correctly. It will fail to do so if it issues the revoke before
loading data. The same issue applies to all data loading,
including refreshing matviews.

test=# create role joe;
CREATE ROLE
test=# set role joe;
SET
test=> create table joes_table(f1 int);
CREATE TABLE
test=> insert into joes_table values(1);
INSERT 0 1
test=> revoke insert on joes_table from joe;
REVOKE
test=> create materialized view joes_mv as select * from joes_table;
SELECT 1
test=> revoke insert on joes_mv from joe;
REVOKE
test=> refresh materialized view joes_mv;
REFRESH MATERIALIZED VIEW

The problem we're having restoring the matview state is that not
all ACLs needed for *SELECT* permissions are in place in time. I
am not seeing the problem with self-revoke on REFRESH. What am I
missing?

Of course, since the REVOKE on the matview has no affect, it should
probably not be allowed. I could add an error for the attempt.

One other question about the patch was what I did for testing. It
seemed like a good idea to have dump/restore tests, but I don't see
how to do that without leaving a role or two lingering in the
cluster. Is that allowed? (I see that I need to DIE first, to
prevent errors on multiple runs, but otherwise?)

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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Kevin Grittner <kgrittn@gmail.com> writes:

The problem we're having restoring the matview state is that not
all ACLs needed for *SELECT* permissions are in place in time. I
am not seeing the problem with self-revoke on REFRESH. What am I
missing?

Uh ... that the owner might revoke his own SELECT privilege?

One other question about the patch was what I did for testing. It
seemed like a good idea to have dump/restore tests, but I don't see
how to do that without leaving a role or two lingering in the
cluster. Is that allowed?

In make installcheck, no, it is absolutely not. I'd suggest thinking
about testing this in the pg_dump TAP tests, instead, where we're just
creating a private database instance.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Tue, Jul 26, 2016 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Kevin Grittner <kgrittn@gmail.com> writes:

The problem we're having restoring the matview state is that not
all ACLs needed for *SELECT* permissions are in place in time. I
am not seeing the problem with self-revoke on REFRESH. What am I
missing?

Uh ... that the owner might revoke his own SELECT privilege?

What about policies that might have changed since the latest
REFRESH before dump? How about views or functions that might have
changed? If I'm understanding what you want, the only way to get
that would be to COPY matview data the same as a table, and have a
way to load it in before locking down the matview from direct
changes. That seems to me like it would completely compromise
future ability to incrementally maintain matviews, since you could
not have any assurance that the state of a matview matched any
particular derivation from the sources, matched to a point where
changes could be played forward.

One other question about the patch was what I did for testing. It
seemed like a good idea to have dump/restore tests, but I don't see
how to do that without leaving a role or two lingering in the
cluster. Is that allowed?

In make installcheck, no, it is absolutely not. I'd suggest thinking
about testing this in the pg_dump TAP tests, instead, where we're just
creating a private database instance.

ok

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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Kevin Grittner <kgrittn@gmail.com> writes:

On Tue, Jul 26, 2016 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Uh ... that the owner might revoke his own SELECT privilege?

What about policies that might have changed since the latest
REFRESH before dump? How about views or functions that might have
changed?

I think you're setting up straw men. No one has argued that we should
dump and restore the verbatim current contents of a matview. But there
is a longstanding expectation that pg_dump be able to dump and restore
the contents of read-only tables, and what you proposed breaks that.
That won't do.

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.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Tue, Jul 26, 2016 at 12:32 PM, Tom Lane <tgl@sss.pgh.pa.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 and that we should "split up the
ACL restores into the general part, which you would run right after
the object is restored, and the owner revokes, which you would run
last." I guess the idea is that we would refresh the matviews in
between those two phases.

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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Kevin Grittner <kgrittn@gmail.com> writes:

On Tue, Jul 26, 2016 at 12:32 PM, Tom Lane <tgl@sss.pgh.pa.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. 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. And I'm not
really convinced that the patch preserves dependency ordering requirements
at all, even in plain serial restore.

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

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

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs