docs: note ownership requirement for refreshing materialized views

Started by Dian Fayover 7 years ago15 messages

dian.m.fay@gmail.com

over 7 years ago

1 attachment(s)

hi all! I discovered today that the REFRESH MATERIALIZED VIEW
documentation doesn't mention that only the owner (or a superuser) may
actually perform the refresh operation. This patch adds a note to that
effect.

Dian Fay

Attachments:

0001-docs-note-ownership-requirement-for-refreshing-mater.patchtext/x-patch; name=0001-docs-note-ownership-requirement-for-refreshing-mater.patchDownload

diff --git a/doc/src/sgml/ref/refresh_materialized_view.sgml b/doc/src/sgml/ref/refresh_materialized_view.sgml
index 9cf01a25a5..f001a661e1 100644
--- a/doc/src/sgml/ref/refresh_materialized_view.sgml
+++ b/doc/src/sgml/ref/refresh_materialized_view.sgml
@@ -91,6 +91,10 @@ REFRESH MATERIALIZED VIEW [ CONCURRENTLY ] <replaceable class="parameter">name</
  <refsect1>
   <title>Notes</title>
 
+  <para>
+   Only the owner of a materialized view or a superuser can refresh it.
+  </para>
+
   <para>
    While the default index for future
    <xref linkend="sql-cluster"/>

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Dian Fay (#1)

Re: docs: note ownership requirement for refreshing materialized views

On Wed, Aug 15, 2018 at 07:46:49PM -0400, Dian Fay wrote:

hi all! I discovered today that the REFRESH MATERIALIZED VIEW documentation
doesn't mention that only the owner (or a superuser) may actually perform
the refresh operation. This patch adds a note to that effect.

I think that's a good idea. I would rewrite that a bit differently, like:
To refresh a materialized view, one must be the materalized view's owner
or a superuser.

Thougts or objections?
--
Michael

Jonathan S. Katz

jonathan.katz@excoventures.com

over 7 years ago

In reply to: Dian Fay (#1)

1 attachment(s)

Re: docs: note ownership requirement for refreshing materialized views

Hi Dian,

On Aug 15, 2018, at 7:46 PM, Dian Fay <dian.m.fay@gmail.com> wrote:

hi all! I discovered today that the REFRESH MATERIALIZED VIEW documentation doesn't mention that only the owner (or a superuser) may actually perform the refresh operation. This patch adds a note to that effect.

I played around with this feature a bit and did see this was the case.
Also while playing around I noticed the error message was as such:

test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blah

But it’s not a relation, it’s a materialized view. I attached a patch
that I think should fix this. Kudos to Dave Cramer who was
sitting next to me helping me to locate files and confirm assumptions.

Jonathan

Attachments:

0001-treat-refresh-mat-view-as-mat-view.patchapplication/octet-stream; name=0001-treat-refresh-mat-view-as-mat-view.patch; x-unix-mode=0644Download

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 87f5e95827..a769e38829 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4138,6 +4138,7 @@ RefreshMatViewStmt:
 			REFRESH MATERIALIZED VIEW opt_concurrently qualified_name opt_with_data
 				{
 					RefreshMatViewStmt *n = makeNode(RefreshMatViewStmt);
+					n->relkind = OBJECT_MATVIEW;
 					n->concurrent = $4;
 					n->relation = $5;
 					n->skipData = !($6);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 07ab1a3dde..2998f401bb 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3217,6 +3217,7 @@ typedef struct RefreshMatViewStmt
 	bool		concurrent;		/* allow concurrent access? */
 	bool		skipData;		/* true for WITH NO DATA */
 	RangeVar   *relation;		/* relation to insert into */
+	ObjectType	relkind;		/* OBJECT_MATVIEW */
 } RefreshMatViewStmt;
 
 /* ----------------------

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Jonathan S. Katz (#3)

Re: docs: note ownership requirement for refreshing materialized views

On Wed, Aug 15, 2018 at 09:06:34PM -0400, Jonathan S. Katz wrote:

I played around with this feature a bit and did see this was the case.
Also while playing around I noticed the error message was as such:

test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blah

But it’s not a relation, it’s a materialized view. I attached a patch
that I think should fix this. Kudos to Dave Cramer who was
sitting next to me helping me to locate files and confirm assumptions.

A relation may be a materialized view, no? The ACL check happens in
RangeVarCallbackOwnsTable by the way (look at ExecRefreshMatView in
matview.c).
--
Michael

Dian Fay

dian.m.fay@gmail.com

over 7 years ago

In reply to: Michael Paquier (#2)

Re: docs: note ownership requirement for refreshing materialized views

I feel resorting to the infinitive asks more involvement of the reader,
while leading with the responsible role(s) helps shortcut the process of
determining whether what follows is relevant. Efficiency is always a
virtue, although this is admittedly more than a little academic for a
one-sentence addition!

Show quoted text

On 8/15/18 9:03 PM, Michael Paquier wrote:

On Wed, Aug 15, 2018 at 07:46:49PM -0400, Dian Fay wrote:

hi all! I discovered today that the REFRESH MATERIALIZED VIEW documentation
doesn't mention that only the owner (or a superuser) may actually perform
the refresh operation. This patch adds a note to that effect.

I think that's a good idea. I would rewrite that a bit differently, like:
To refresh a materialized view, one must be the materalized view's owner
or a superuser.

Thougts or objections?
--
Michael

Tom Lane

tgl@sss.pgh.pa.us

over 7 years ago

In reply to: Dian Fay (#5)

Re: docs: note ownership requirement for refreshing materialized views

Dian Fay <dian.m.fay@gmail.com> writes:

I feel resorting to the infinitive asks more involvement of the reader,
while leading with the responsible role(s) helps shortcut the process of
determining whether what follows is relevant. Efficiency is always a
virtue, although this is admittedly more than a little academic for a
one-sentence addition!

I think Michael's point is that this formulation is unlike what we have
elsewhere for similar statements. Looking around, it seems like the
typical phraseology would be more like

"You must own the materialized view to use REFRESH MATERIALIZED VIEW."

It is not really customary to call out the superuser exception
explicitly, because if we did, we'd be mentioning superusers in every
other sentence. The point is covered by existing documentation that
says something to the effect of superusers bypassing all permissions
checks; and I think there's also a statement somewhere about superusers
implicitly being members of every role, which is a different way of
arriving at the same conclusion.

In any case, it's definitely an oversight that the REFRESH reference
page fails to address permissions at all. +1 for adding something.

regards, tom lane

Jonathan S. Katz

jonathan.katz@excoventures.com

over 7 years ago

In reply to: Michael Paquier (#4)

Re: docs: note ownership requirement for refreshing materialized views

On Aug 15, 2018, at 9:15 PM, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Aug 15, 2018 at 09:06:34PM -0400, Jonathan S. Katz wrote:

I played around with this feature a bit and did see this was the case.
Also while playing around I noticed the error message was as such:

test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blah

But it’s not a relation, it’s a materialized view. I attached a patch
that I think should fix this. Kudos to Dave Cramer who was
sitting next to me helping me to locate files and confirm assumptions.

A relation may be a materialized view, no? The ACL check happens in
RangeVarCallbackOwnsTable by the way (look at ExecRefreshMatView in
matview.c).

Comment on the RangeVarCallbackOwnsTable func (abbr):

/*
* This is intended as a callback for RangeVarGetRelidExtended(). It allows
* the relation to be locked only if (1) it's a plain table, materialized
* view, or TOAST table and (2) the current user is the owner (or the
* superuser). This meets the permission-checking needs of CLUSTER, REINDEX
* TABLE, and REFRESH MATERIALIZED VIEW; we expose it here so that it can be
* used by all.
*/

So it’s sharing the permission checking needs amongst all of those commands.

As a user I could be confused if I saw the above error message, esp. because
the behavior of REFRESH .. is specific to materialized views.

Jonathan

Dian Fay

dian.m.fay@gmail.com

over 7 years ago

In reply to: Tom Lane (#6)

1 attachment(s)

Re: docs: note ownership requirement for refreshing materialized views

Fair enough! Here's a new version.

Show quoted text

On 8/16/18 12:07 AM, Tom Lane wrote:

Dian Fay <dian.m.fay@gmail.com> writes:

I feel resorting to the infinitive asks more involvement of the reader,
while leading with the responsible role(s) helps shortcut the process of
determining whether what follows is relevant. Efficiency is always a
virtue, although this is admittedly more than a little academic for a
one-sentence addition!

I think Michael's point is that this formulation is unlike what we have
elsewhere for similar statements. Looking around, it seems like the
typical phraseology would be more like

"You must own the materialized view to use REFRESH MATERIALIZED VIEW."

It is not really customary to call out the superuser exception
explicitly, because if we did, we'd be mentioning superusers in every
other sentence. The point is covered by existing documentation that
says something to the effect of superusers bypassing all permissions
checks; and I think there's also a statement somewhere about superusers
implicitly being members of every role, which is a different way of
arriving at the same conclusion.

In any case, it's definitely an oversight that the REFRESH reference
page fails to address permissions at all. +1 for adding something.

regards, tom lane

Attachments:

0001-docs-note-ownership-requirement-for-refreshing-mater.patchtext/x-patch; name=0001-docs-note-ownership-requirement-for-refreshing-mater.patchDownload

diff --git a/doc/src/sgml/ref/refresh_materialized_view.sgml b/doc/src/sgml/ref/refresh_materialized_view.sgml
index 9cf01a25a5..f001a661e1 100644
--- a/doc/src/sgml/ref/refresh_materialized_view.sgml
+++ b/doc/src/sgml/ref/refresh_materialized_view.sgml
@@ -91,6 +91,10 @@ REFRESH MATERIALIZED VIEW [ CONCURRENTLY ] <replaceable class="parameter">name</
  <refsect1>
   <title>Notes</title>
 
+  <para>
+   You must own the materialized view in order to use REFRESH MATERIALIZED VIEW.
+  </para>
+
   <para>
    While the default index for future
    <xref linkend="sql-cluster"/>

Jonathan S. Katz

jonathan.katz@excoventures.com

over 7 years ago

In reply to: Jonathan S. Katz (#7)

Re: docs: note ownership requirement for refreshing materialized views

On Aug 16, 2018, at 1:05 AM, Jonathan S. Katz <jonathan.katz@excoventures.com> wrote:

On Aug 15, 2018, at 9:15 PM, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Aug 15, 2018 at 09:06:34PM -0400, Jonathan S. Katz wrote:

I played around with this feature a bit and did see this was the case.
Also while playing around I noticed the error message was as such:

test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blah

But it’s not a relation, it’s a materialized view. I attached a patch
that I think should fix this. Kudos to Dave Cramer who was
sitting next to me helping me to locate files and confirm assumptions.

A relation may be a materialized view, no? The ACL check happens in
RangeVarCallbackOwnsTable by the way (look at ExecRefreshMatView in
matview.c).

Comment on the RangeVarCallbackOwnsTable func (abbr):

/*
* This is intended as a callback for RangeVarGetRelidExtended(). It allows
* the relation to be locked only if (1) it's a plain table, materialized
* view, or TOAST table and (2) the current user is the owner (or the
* superuser). This meets the permission-checking needs of CLUSTER, REINDEX
* TABLE, and REFRESH MATERIALIZED VIEW; we expose it here so that it can be
* used by all.
*/

So it’s sharing the permission checking needs amongst all of those commands.

As a user I could be confused if I saw the above error message, esp. because
the behavior of REFRESH .. is specific to materialized views.

With encouragement from Dave, let me demonstrate what the proposed patch
does to fix the behavior. The steps, running from my “jkatz” user:

CREATE ROLE bar LOGIN;
CREATE TABLE a (x int);
CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
\c - bar
REFRESH MATERIALIZED VIEW b;
ERROR: must be owner of materialized view b

vs. the existing error message which I posted further upthread.

Thanks,

Jonathan

#10

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Dian Fay (#8)

Re: docs: note ownership requirement for refreshing materialized views

On Thu, Aug 16, 2018 at 09:36:29AM -0400, Dian Fay wrote:

Fair enough! Here's a new version.

Thanks, pushed. Instead of putting the new sentence in the "Notes"
section, I have added it in the "Description" section, which is more
consistent with other commands, like DROP MATERIALIZED VIEW, etc.
--
Michael

#11

Dave Cramer

davecramer@gmail.com

over 7 years ago

In reply to: Jonathan S. Katz (#9)

Re: docs: note ownership requirement for refreshing materialized views

Dave Cramer

On Thu, 16 Aug 2018 at 18:27, Jonathan S. Katz <
jonathan.katz@excoventures.com> wrote:

On Aug 16, 2018, at 1:05 AM, Jonathan S. Katz <
jonathan.katz@excoventures.com> wrote:

On Aug 15, 2018, at 9:15 PM, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Aug 15, 2018 at 09:06:34PM -0400, Jonathan S. Katz wrote:

I played around with this feature a bit and did see this was the case.
Also while playing around I noticed the error message was as such:

test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blah

But it’s not a relation, it’s a materialized view. I attached a patch
that I think should fix this. Kudos to Dave Cramer who was
sitting next to me helping me to locate files and confirm assumptions.

A relation may be a materialized view, no? The ACL check happens in
RangeVarCallbackOwnsTable by the way (look at ExecRefreshMatView in
matview.c).

Comment on the RangeVarCallbackOwnsTable func (abbr):

/*
* This is intended as a callback for RangeVarGetRelidExtended(). It
allows
* the relation to be locked only if (1) it's a plain table,
materialized
* view, or TOAST table and (2) the current user is the owner (or the
* superuser). This meets the permission-checking needs of CLUSTER,
REINDEX
* TABLE, and REFRESH MATERIALIZED VIEW; we expose it here so that it
can be
* used by all.
*/

So it’s sharing the permission checking needs amongst all of those
commands.

As a user I could be confused if I saw the above error message, esp.
because
the behavior of REFRESH .. is specific to materialized views.

With encouragement from Dave, let me demonstrate what the proposed patch
does to fix the behavior. The steps, running from my “jkatz” user:

CREATE ROLE bar LOGIN;
CREATE TABLE a (x int);
CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
\c - bar
REFRESH MATERIALIZED VIEW b;
ERROR: must be owner of materialized view b

vs. the existing error message which I posted further upthread.

Thanks,

Jonathan

So it seems this patch is being ignored in this thread.
AFAICT, in aclcheck_error() the only way to get to the following sub case
in the case ACLCHECK_NOT_OWNER

case OBJECT_MATVIEW:
msg = gettext_noop("must be owner of materialized view %s");
break;

is if the patch is applied ?

Regards,

Dave

#12

Dian Fay

dian.m.fay@gmail.com

over 7 years ago

In reply to: Dave Cramer (#11)

Re: docs: note ownership requirement for refreshing materialized views

Jonathan's patch seems like a good idea to me from a user POV, but then
I just showed up the other day so I don't really have anything of
substance to add.

Show quoted text

On 8/17/18 9:08 AM, Dave Cramer wrote:

Dave Cramer

On Thu, 16 Aug 2018 at 18:27, Jonathan S. Katz
<jonathan.katz@excoventures.com
<mailto:jonathan.katz@excoventures.com>> wrote:

On Aug 16, 2018, at 1:05 AM, Jonathan S. Katz
<jonathan.katz@excoventures.com
<mailto:jonathan.katz@excoventures.com>> wrote:

On Aug 15, 2018, at 9:15 PM, Michael Paquier
<michael@paquier.xyz <mailto:michael@paquier.xyz>> wrote:

On Wed, Aug 15, 2018 at 09:06:34PM -0400, Jonathan S. Katz wrote:

I played around with this feature a bit and did see this was
the case.
Also while playing around I noticed the error message was as such:

test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blah

But it’s not a relation, it’s a materialized view. I attached a
patch
that I think should fix this. Kudos to Dave Cramer who was
sitting next to me helping me to locate files and confirm
assumptions.

A relation may be a materialized view, no? The ACL check happens in
RangeVarCallbackOwnsTable by the way (look at ExecRefreshMatView in
matview.c).

Comment on the RangeVarCallbackOwnsTable func (abbr):

   /*
    * This is intended as a callback for
RangeVarGetRelidExtended(). It allows
    * the relation to be locked only if (1) it's a plain table,
materialized
    * view, or TOAST table and (2) the current user is the owner
(or the
    * superuser). This meets the permission-checking needs of
CLUSTER, REINDEX
    * TABLE, and REFRESH MATERIALIZED VIEW; we expose it here so
that it can be
    * used by all.
    */

So it’s sharing the permission checking needs amongst all of
those commands.

As a user I could be confused if I saw the above error message,
esp. because
the behavior of REFRESH .. is specific to materialized views.

With encouragement from Dave, let me demonstrate what the proposed
patch
does to fix the behavior. The steps, running from my “jkatz” user:

CREATE ROLE bar LOGIN;
CREATE TABLE a (x int);
CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
\c - bar
REFRESH MATERIALIZED VIEW b;
ERROR: must be owner of materialized view b

vs. the existing error message which I posted further upthread.

Thanks,

Jonathan

So it seems this patch is being ignored in this thread.
AFAICT, in aclcheck_error() the only way to get to the following sub
case in the case ACLCHECK_NOT_OWNER

case OBJECT_MATVIEW:
msg = gettext_noop("must be owner of materialized view %s");
break;

is if the patch is applied ?

Regards,

Dave

#13

Tom Lane

tgl@sss.pgh.pa.us

over 7 years ago

In reply to: Dave Cramer (#11)

Re: docs: note ownership requirement for refreshing materialized views

Dave Cramer <davecramer@gmail.com> writes:

So it seems this patch is being ignored in this thread.

Well, Jonathan did kind of hijack what appears to be a thread about
documentation (with an already-committed fix).

I'd suggest reposting that patch in its own thread and adding it to
the next CF.

regards, tom lane

#14

Jonathan S. Katz

jonathan.katz@excoventures.com

over 7 years ago

In reply to: Tom Lane (#13)

Re: docs: note ownership requirement for refreshing materialized views

On Aug 17, 2018, at 9:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dave Cramer <davecramer@gmail.com> writes:

So it seems this patch is being ignored in this thread.

Well, Jonathan did kind of hijack what appears to be a thread about
documentation (with an already-committed fix).

I apologize if it was interpreted as hijacking, I had brought it up in
my initial reply to Dian, as I’ve seen others do similarly on threads.

I'd suggest reposting that patch in its own thread and adding it to
the next CF.

I will go ahead and do this.

Thanks, and thank you for the patch Dian.

Jonathan

#15

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Jonathan S. Katz (#14)

Re: docs: note ownership requirement for refreshing materialized views

On Fri, Aug 17, 2018 at 05:12:42PM -0400, Jonathan S. Katz wrote:

On Aug 17, 2018, at 9:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Dave Cramer <davecramer@gmail.com> writes:

So it seems this patch is being ignored in this thread.

Well, Jonathan did kind of hijack what appears to be a thread about
documentation (with an already-committed fix).

I apologize if it was interpreted as hijacking, I had brought it up in
my initial reply to Dian, as I’ve seen others do similarly on threads.

Don't worry, we all learn here ;)
I saw your patch, looked at it a couple of seconds and got really
surprised by its logic, moving to something else before lookng back at
it.

I'd suggest reposting that patch in its own thread and adding it to
the next CF.

I will go ahead and do this.

Thanks for spawning a different thread. That helps in attracting the
correct attention to the correct issues.
--
Michael