Date: Sun, 16 Jun 2019 13:20:21 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
	Tomas Vondra <tomas.vondra@postgresql.org>,
	pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Rework the pg_statistic_ext catalog
Message-ID: <20190616112021.2mncnuj26sbsbnzx@development>
References: <E1hcHz2-0005mY-5A@gemulon.postgresql.org>
 <8833.1560647898@sss.pgh.pa.us>
 <20156.1560650744@sss.pgh.pa.us>
 <CAEZATCWWX5O57d=MHY=MqyJe-xHG2pbi75LKLjo6aZLP3zsaow@mail.gmail.com>
 <20190616085651.d2hxuiqlojewed5j@development>
 <20190616100328.shl2hhuy2pup6ges@development>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20190616100328.shl2hhuy2pup6ges@development>
User-Agent: NeoMutt/20180716-1444-295967
Precedence: bulk

On Sun, Jun 16, 2019 at 12:03:28PM +0200, Tomas Vondra wrote:
>On Sun, Jun 16, 2019 at 10:56:51AM +0200, Tomas Vondra wrote:
>>On Sun, Jun 16, 2019 at 07:18:58AM +0100, Dean Rasheed wrote:
>>>On Sun, 16 Jun 2019, 03:05 Tom Lane, <tgl@sss.pgh.pa.us> wrote:
>>>
>>>>I wrote:
>>>>>Tomas Vondra <tomas.vondra@postgresql.org> writes:
>>>>>>Rework the pg_statistic_ext catalog
>>>>
>>>>>So ... not one of the buildfarm members that are running TAP tests
>>>>>likes this. ...
>>>>>I think probably what's happening is that pg_dump is still trying to dump
>>>>>directly from the catalog, when what it needs to do now is dump from the
>>>>>view, in case it's not running as superuser.
>>>>
>>>>I experimented with extracting the required data from the view, and
>>>>there are at least two show-stopper problems:
>>>>
>>>>* The view doesn't expose pg_statistic_ext.oid, which pg_dump has to have
>>>>for dependency tracking purposes.  I think we could just add it though.
>>>>Now that OIDs are ordinary columns it won't even look very odd.
>>>>
>>>>* Rather than just not exposing the critical data for stats you don't
>>>>have privilege to read, the view doesn't expose anything at all.
>>>>I do not think that's acceptable; it creates a significant hazard of
>>>>data loss during pg_dump, for no very good reason.  What we should
>>>>be doing, IMO, is still showing all the rows but filling the data-value
>>>>columns with nulls in rows where the caller can't access the underlying
>>>>data.
>>>>
>>>>
>>>
>>>Hang on. Isn't the real problem that we should be revoking public access
>>>from pg_statistic_ext_data, not pg_statistic_ext? Normal users should still
>>>be able to see the stats definitions in the catalog table, but not the
>>>stats data, so I think pg_dump wouldn't need changing.
>>>
>>
>>Yeah, that's pretty much why we split the catalog - to allow pg_dump to
>>read the definitions, without exposing the stats too. I'll look into
>>this and push a fix to unbreak the buildfarm (sorry about that).
>>
>
>OK, the bug here seems to be pretty silly - the GRANT in system_views
>does this:
>
>GRANT SELECT (oid, stxrelid, stxname, stxnamespace, stxowner, stxkeys, stxkind)
>   ON pg_statistic_ext TO public;
>
>but pg_dump also uses tableoid column. After adding it to the GRANT I
>can run pg_dump using regular user just fine. I'll push this fix
>shortly.
>

OK, it seems the buildfarm is getting green again. I'm not sure why I'm
seeing the failure in 011_crash_recovery.pl, while the buildfarm does
not. Perhaps my environment is borked in some way? Not sure.

Anyway, I'm wondering if we need the REVOKE/GRANT on pg_statistic_ext at
all, and if we do if we should just remove the list of columns, and
grant SELECT on all. All the sensitive columns were moved to another
catalog, after all.

That also reminds me the patch should have revoked access to the new
_data catalog.


regards

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