Обсуждение: docs: note ownership requirement for refreshing materialized views

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

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

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

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

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!

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

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

> 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

Fair enough! Here's a new version.


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

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

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.

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

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

> 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

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