[Bossart, Nathan <bossartn@amazon.com>]

On 8/22/18, 12:37 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Wed, Aug 22, 2018 at 02:17:44AM +0000, Bossart, Nathan wrote:
>> I think this is doable by locking the table in SHARE mode.  That won't
>> conflict with the AccessShareLock that expand_vacuum_rel() obtains,
>> but it will conflict with the ShareUpdateExclusiveLock or
>> AccessExclusiveLock that vacuum_rel() takes.
>
> Good point.  Still is that really worth adding?  This implies a test
> which has at least two roles, one switching the ownership to the other
> and do so back-and-forth.  At least that should be on a different
> isolation spec file to not complicate the first one.

I think so, since this is the only ownership checks we do on
individual partitions.  Another simple way to test this would be to
create a partitioned table with a different owner than the partitions
and to run VACUUM as the partitioned table owner.  In this case, we
would still rely on the checks in vacuum_rel() and analyze_rel().  IMO
this is a reason to avoid skipping gathering the individual partitions
based upon the ownership of the partitioned table.  It's true that
this wouldn't fix the locking issue for partitions, but the
aforementioned edge case is still present with 0002 anyway.  Plus, it
would add a bit more consistency to partition handling in VACUUM.

I've attached a patch that applies on top of 0002 that adds a simple
test to exercise the checks in vacuum_rel() and analyze_rel().

+		/*
+		 * For VACUUM ANALYZE, both logs could show up, but just generate
+		 * information for VACUUM as that would be the first one to
+		 * process.
+		 */
+		return;

We should probably return false here.

Nathan



Attachments:

  [application/octet-stream] vacuum_permission_checks.patch (1.8K, ../DC186201-B01F-4A66-9EC4-F855A957C1F9@amazon.com/2-vacuum_permission_checks.patch)
  download | inline diff:
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index c9be71ef60..45e69573ce 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -119,9 +119,21 @@ ANALYZE (nonexistant-arg) does_not_exist;
 ERROR:  syntax error at or near "nonexistant"
 LINE 1: ANALYZE (nonexistant-arg) does_not_exist;
                  ^
+-- permission checks in vacuum_rel() and analyze_rel()
+CREATE ROLE vacparted_owner;
+ALTER TABLE vacparted OWNER TO vacparted_owner;
+SET ROLE vacparted_owner;
+VACUUM vacparted;
+WARNING:  skipping "vacparted1" --- only table or database owner can vacuum it
+ANALYZE vacparted;
+WARNING:  skipping "vacparted1" --- only table or database owner can analyze it
+VACUUM (ANALYZE) vacparted;
+WARNING:  skipping "vacparted1" --- only table or database owner can vacuum it
+RESET ROLE;
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
+DROP ROLE vacparted_owner;
 -- relation ownership, WARNING logs generated as all are skipped.
 CREATE TABLE vacowned (a int);
 CREATE ROLE regress_vacuum;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 0feff7c413..9e4d081da9 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -93,9 +93,19 @@ ANALYZE vactst (i), vacparted (does_not_exist);
 ANALYZE (VERBOSE) does_not_exist;
 ANALYZE (nonexistant-arg) does_not_exist;
 
+-- permission checks in vacuum_rel() and analyze_rel()
+CREATE ROLE vacparted_owner;
+ALTER TABLE vacparted OWNER TO vacparted_owner;
+SET ROLE vacparted_owner;
+VACUUM vacparted;
+ANALYZE vacparted;
+VACUUM (ANALYZE) vacparted;
+RESET ROLE;
+
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
+DROP ROLE vacparted_owner;
 
 -- relation ownership, WARNING logs generated as all are skipped.
 CREATE TABLE vacowned (a int);