Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fqea4-0004GC-JB for pgsql-hackers@arkaria.postgresql.org; Fri, 17 Aug 2018 13:14:20 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fqea2-0001dj-Nh for pgsql-hackers@arkaria.postgresql.org; Fri, 17 Aug 2018 13:14:18 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fqea2-0001dc-Du for pgsql-hackers@lists.postgresql.org; Fri, 17 Aug 2018 13:14:18 +0000 Received: from mail-it0-x234.google.com ([2607:f8b0:4001:c0b::234]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fqeZy-0006R3-V0 for pgsql-hackers@lists.postgresql.org; Fri, 17 Aug 2018 13:14:17 +0000 Received: by mail-it0-x234.google.com with SMTP id 139-v6so11689471itf.0 for ; Fri, 17 Aug 2018 06:14:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language; bh=14yWnLfKb82TDZbTrVipp/gCMDelm0fJD5qBEo8JXvw=; b=JRiOjl7UVBxRZdFkVyl09apsGBtXlEirKs7aJUnvj7rKFZiJN5kw43UGqWlx9LJd6R SoFxpudvhwyiYZA9r7pAfAUlBOZzJhKze3eSL+6MePV7Ea3d7Ll7X4wsZMgwhcXu3bng 1ZWN3YY8cwRA1RzzTvE6LR1iKkNwbNZOjeZUix6nUPQGLu/6pNLIp1LEQtvjlQmVkkcd I8ggn6DOz5a0epUNX59sIkrjoumvS08cN83A4lbZAkmVbs2mOtXRWBG9yOZNrWuurBKY jItBhiKuNDGJ3p+zDubKqY37Jn+lMbjyr1uCKre+QChXiKUz9PqLCx5qe9XLJaAO38yM Xz5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=14yWnLfKb82TDZbTrVipp/gCMDelm0fJD5qBEo8JXvw=; b=gh4rQpxHbNR5y74723ylP4l2Svl/tpUsVdUtSJliQCXxBNhiu+EgIGTbKsqQp7wY70 CxcmCQlfN6vvFI/hur941VCeELR8iiM0YsqPlEaCJstZAH/k4S5HA6tvnlrAUs0SzyBi PQw3ZBM2zsaSiJoX4cbPqeUuNS++U0jjtSTj7J/X/Km/bxODdpssLiw1dPTEKvDMzUTY wjc1ogFjvhynieHEnnJ/XYrzK0KQV/7WCM3hfyWkHSQ6q7YE23Towi5R7BDbDHG7BHUI PqtTuHN3x/q9Gi5029VB8RdCdtCtKajkEgtHov1ZxJ8dvOMk2331XiOx9Gj6kogmaZ0j k4XA== X-Gm-Message-State: AOUpUlF5INNcHwp3cIXg7zKpQ9hsmycYbizbfiLsI8zApJ3GQUcVKvpH 8ce+k4gfKH2hHL6U4oId5axWFOlSM94= X-Google-Smtp-Source: AA+uWPwQG/2BwC/1JWb57kFI1lKINaRoSVEhnHfdaMslD3+g13iOS0/GBeGqbmti57MZ3DubnDza5Q== X-Received: by 2002:a24:3ac3:: with SMTP id m186-v6mr4438164itm.106.1534511652413; Fri, 17 Aug 2018 06:14:12 -0700 (PDT) Received: from [10.0.0.9] (108-72-92-36.lightspeed.livnmi.sbcglobal.net. [108.72.92.36]) by smtp.gmail.com with ESMTPSA id q137-v6sm734860iod.23.2018.08.17.06.14.11 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 17 Aug 2018 06:14:11 -0700 (PDT) Subject: Re: docs: note ownership requirement for refreshing materialized views To: Dave Cramer , Jonathan Katz Cc: Michael Paquier , PostgreSQL Hackers References: <745abbd2-a1a0-ead8-2cb2-768c16747d97@gmail.com> <55498B5B-0155-4B0E-9B97-23167F8CB380@excoventures.com> <20180816011534.GK3681@paquier.xyz> <862321D6-495D-4A62-94D2-17D6EDF87A41@excoventures.com> From: Dian Fay Message-ID: <009dad19-e8bb-1e98-2899-06451f1db443@gmail.com> Date: Fri, 17 Aug 2018 09:14:10 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------4BD58950C183E79E04D171F1" Content-Language: en-HK List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk This is a multi-part message in MIME format. --------------4BD58950C183E79E04D171F1 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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 > > wrote: > > >> On Aug 16, 2018, at 1:05 AM, Jonathan S. Katz >> > > wrote: >> >>> >>> On Aug 15, 2018, at 9:15 PM, Michael Paquier >>> > 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 --------------4BD58950C183E79E04D171F1 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit

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> 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
--------------4BD58950C183E79E04D171F1--