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.

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