On Wed, Aug 22, 2018 at 03:49:16PM +0000, Bossart, Nathan wrote: > 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.
Normal regression tests are less costly than isolation tests, so let's use them as possible. What you attached is covering only a portion of all the scenarios though, as it is as well interesting to see what happens if another user owns only the partitioned table, only one partition, and the partitioned as well as at least one partition. I have extended your patch as attached. It applies on top of HEAD. Once applied with the other patch one can easily stop the difference in behavior, and this stresses the ownership checks in vacuum_rel() and analyze_rel() as well. Perhaps we could begin by that? > We should probably return false here. Oh, my compiler complained here as well. Fixed it on my branch. -- Michael
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index c9be71ef60..57e451738c 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -124,6 +124,9 @@ DROP TABLE vactst; DROP TABLE vacparted; -- relation ownership, WARNING logs generated as all are skipped. CREATE TABLE vacowned (a int); +CREATE TABLE vacowned_parted (a int) PARTITION BY LIST (a); +CREATE TABLE vacowned_part1 PARTITION OF vacowned_parted FOR VALUES IN (1); +CREATE TABLE vacowned_part2 PARTITION OF vacowned_parted FOR VALUES IN (2); CREATE ROLE regress_vacuum; SET ROLE regress_vacuum; -- Simple table @@ -147,6 +150,102 @@ ANALYZE pg_catalog.pg_authid; WARNING: skipping "pg_authid" --- only superuser can analyze it VACUUM (ANALYZE) pg_catalog.pg_authid; WARNING: skipping "pg_authid" --- only superuser can vacuum it +-- partitioned table and its partitions, no ownership. +-- Relations are not listed in a single command to test ownership +-- independently. +VACUUM vacowned_parted; +WARNING: skipping "vacowned_parted" --- only table or database owner can vacuum it +WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +VACUUM vacowned_part1; +WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +VACUUM vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +ANALYZE vacowned_parted; +WARNING: skipping "vacowned_parted" --- only table or database owner can analyze it +WARNING: skipping "vacowned_part1" --- only table or database owner can analyze it +WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +ANALYZE vacowned_part1; +WARNING: skipping "vacowned_part1" --- only table or database owner can analyze it +ANALYZE vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +VACUUM (ANALYZE) vacowned_parted; +WARNING: skipping "vacowned_parted" --- only table or database owner can vacuum it +WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +VACUUM (ANALYZE) vacowned_part1; +WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +VACUUM (ANALYZE) vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +RESET ROLE; +-- Partitioned table and one partition owned by other user. +ALTER TABLE vacowned_parted OWNER TO regress_vacuum; +ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; +SET ROLE regress_vacuum; +VACUUM vacowned_parted; +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +VACUUM vacowned_part1; +VACUUM vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +ANALYZE vacowned_parted; +WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +ANALYZE vacowned_part1; +ANALYZE vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +VACUUM (ANALYZE) vacowned_parted; +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +VACUUM (ANALYZE) vacowned_part1; +VACUUM (ANALYZE) vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +RESET ROLE; +-- Only one partition owned by other user +ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; +SET ROLE regress_vacuum; +VACUUM vacowned_parted; +WARNING: skipping "vacowned_parted" --- only table or database owner can vacuum it +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +VACUUM vacowned_part1; +VACUUM vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +ANALYZE vacowned_parted; +WARNING: skipping "vacowned_parted" --- only table or database owner can analyze it +WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +ANALYZE vacowned_part1; +ANALYZE vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +VACUUM (ANALYZE) vacowned_parted; +WARNING: skipping "vacowned_parted" --- only table or database owner can vacuum it +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +VACUUM (ANALYZE) vacowned_part1; +VACUUM (ANALYZE) vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +RESET ROLE; +-- Only partitioned table owned by other user +ALTER TABLE vacowned_parted OWNER TO regress_vacuum; +ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; +SET ROLE regress_vacuum; +VACUUM vacowned_parted; +WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +VACUUM vacowned_part1; +WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +VACUUM vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +ANALYZE vacowned_parted; +WARNING: skipping "vacowned_part1" --- only table or database owner can analyze it +WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +ANALYZE vacowned_part1; +WARNING: skipping "vacowned_part1" --- only table or database owner can analyze it +ANALYZE vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +VACUUM (ANALYZE) vacowned_parted; +WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +VACUUM (ANALYZE) vacowned_part1; +WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +VACUUM (ANALYZE) vacowned_part2; +WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it RESET ROLE; DROP TABLE vacowned; +DROP TABLE vacowned_parted; DROP ROLE regress_vacuum; diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql index 0feff7c413..fc4c4a3bcc 100644 --- a/src/test/regress/sql/vacuum.sql +++ b/src/test/regress/sql/vacuum.sql @@ -99,6 +99,9 @@ DROP TABLE vacparted; -- relation ownership, WARNING logs generated as all are skipped. CREATE TABLE vacowned (a int); +CREATE TABLE vacowned_parted (a int) PARTITION BY LIST (a); +CREATE TABLE vacowned_part1 PARTITION OF vacowned_parted FOR VALUES IN (1); +CREATE TABLE vacowned_part2 PARTITION OF vacowned_parted FOR VALUES IN (2); CREATE ROLE regress_vacuum; SET ROLE regress_vacuum; -- Simple table @@ -113,6 +116,60 @@ VACUUM (ANALYZE) pg_catalog.pg_class; VACUUM pg_catalog.pg_authid; ANALYZE pg_catalog.pg_authid; VACUUM (ANALYZE) pg_catalog.pg_authid; +-- partitioned table and its partitions, no ownership. +-- Relations are not listed in a single command to test ownership +-- independently. +VACUUM vacowned_parted; +VACUUM vacowned_part1; +VACUUM vacowned_part2; +ANALYZE vacowned_parted; +ANALYZE vacowned_part1; +ANALYZE vacowned_part2; +VACUUM (ANALYZE) vacowned_parted; +VACUUM (ANALYZE) vacowned_part1; +VACUUM (ANALYZE) vacowned_part2; +RESET ROLE; +-- Partitioned table and one partition owned by other user. +ALTER TABLE vacowned_parted OWNER TO regress_vacuum; +ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; +SET ROLE regress_vacuum; +VACUUM vacowned_parted; +VACUUM vacowned_part1; +VACUUM vacowned_part2; +ANALYZE vacowned_parted; +ANALYZE vacowned_part1; +ANALYZE vacowned_part2; +VACUUM (ANALYZE) vacowned_parted; +VACUUM (ANALYZE) vacowned_part1; +VACUUM (ANALYZE) vacowned_part2; +RESET ROLE; +-- Only one partition owned by other user +ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; +SET ROLE regress_vacuum; +VACUUM vacowned_parted; +VACUUM vacowned_part1; +VACUUM vacowned_part2; +ANALYZE vacowned_parted; +ANALYZE vacowned_part1; +ANALYZE vacowned_part2; +VACUUM (ANALYZE) vacowned_parted; +VACUUM (ANALYZE) vacowned_part1; +VACUUM (ANALYZE) vacowned_part2; +RESET ROLE; +-- Only partitioned table owned by other user +ALTER TABLE vacowned_parted OWNER TO regress_vacuum; +ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; +SET ROLE regress_vacuum; +VACUUM vacowned_parted; +VACUUM vacowned_part1; +VACUUM vacowned_part2; +ANALYZE vacowned_parted; +ANALYZE vacowned_part1; +ANALYZE vacowned_part2; +VACUUM (ANALYZE) vacowned_parted; +VACUUM (ANALYZE) vacowned_part1; +VACUUM (ANALYZE) vacowned_part2; RESET ROLE; DROP TABLE vacowned; +DROP TABLE vacowned_parted; DROP ROLE regress_vacuum;
signature.asc
Description: PGP signature
