public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v6 3/3] catcachebench 4+ messages / 3 participants [nested] [flat]
* [PATCH v6 3/3] catcachebench @ 2020-11-18 07:56 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Kyotaro Horiguchi @ 2020-11-18 07:56 UTC (permalink / raw) --- contrib/catcachebench/Makefile | 17 + contrib/catcachebench/catcachebench--0.0.sql | 14 + contrib/catcachebench/catcachebench.c | 330 +++++++++++++++++++ contrib/catcachebench/catcachebench.control | 6 + src/backend/utils/cache/catcache.c | 33 ++ src/backend/utils/cache/syscache.c | 2 +- 6 files changed, 401 insertions(+), 1 deletion(-) create mode 100644 contrib/catcachebench/Makefile create mode 100644 contrib/catcachebench/catcachebench--0.0.sql create mode 100644 contrib/catcachebench/catcachebench.c create mode 100644 contrib/catcachebench/catcachebench.control diff --git a/contrib/catcachebench/Makefile b/contrib/catcachebench/Makefile new file mode 100644 index 0000000000..0478818b25 --- /dev/null +++ b/contrib/catcachebench/Makefile @@ -0,0 +1,17 @@ +MODULE_big = catcachebench +OBJS = catcachebench.o + +EXTENSION = catcachebench +DATA = catcachebench--0.0.sql +PGFILEDESC = "catcachebench - benchmark for catcache pruning feature" + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = contrib/catcachebench +top_builddir = ../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/contrib/catcachebench/catcachebench--0.0.sql b/contrib/catcachebench/catcachebench--0.0.sql new file mode 100644 index 0000000000..ea9cd62abb --- /dev/null +++ b/contrib/catcachebench/catcachebench--0.0.sql @@ -0,0 +1,14 @@ +/* contrib/catcachebench/catcachebench--0.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION catcachebench" to load this file. \quit + +CREATE FUNCTION catcachebench(IN type int) +RETURNS double precision +AS 'MODULE_PATHNAME', 'catcachebench' +LANGUAGE C STRICT VOLATILE; + +CREATE FUNCTION catcachereadstats(OUT catid int, OUT reloid oid, OUT searches bigint, OUT hits bigint, OUT neg_hits bigint) +RETURNS SETOF record +AS 'MODULE_PATHNAME', 'catcachereadstats' +LANGUAGE C STRICT VOLATILE; diff --git a/contrib/catcachebench/catcachebench.c b/contrib/catcachebench/catcachebench.c new file mode 100644 index 0000000000..b5a4d794ed --- /dev/null +++ b/contrib/catcachebench/catcachebench.c @@ -0,0 +1,330 @@ +/* + * catcachebench: test code for cache pruning feature + */ +/* #define CATCACHE_STATS */ +#include "postgres.h" +#include "catalog/pg_type.h" +#include "catalog/pg_statistic.h" +#include "executor/spi.h" +#include "funcapi.h" +#include "libpq/pqsignal.h" +#include "utils/catcache.h" +#include "utils/syscache.h" +#include "utils/timestamp.h" + +Oid tableoids[10000]; +int ntables = 0; +int16 attnums[1000]; +int natts = 0; + +PG_MODULE_MAGIC; + +double catcachebench1(void); +double catcachebench2(void); +double catcachebench3(void); +void collectinfo(void); +void catcachewarmup(void); + +PG_FUNCTION_INFO_V1(catcachebench); +PG_FUNCTION_INFO_V1(catcachereadstats); + +extern void CatalogCacheFlushCatalog2(Oid catId); +extern int64 catcache_called; +extern CatCache *SysCache[]; + +typedef struct catcachestatsstate +{ + TupleDesc tupd; + int catId; +} catcachestatsstate; + +Datum +catcachereadstats(PG_FUNCTION_ARGS) +{ + catcachestatsstate *state_data = NULL; + FuncCallContext *fctx; + + if (SRF_IS_FIRSTCALL()) + { + TupleDesc tupdesc; + MemoryContext mctx; + + fctx = SRF_FIRSTCALL_INIT(); + mctx = MemoryContextSwitchTo(fctx->multi_call_memory_ctx); + + state_data = palloc(sizeof(catcachestatsstate)); + + /* Build a tuple descriptor for our result type */ + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) + elog(ERROR, "return type must be a row type"); + + state_data->tupd = tupdesc; + state_data->catId = 0; + + fctx->user_fctx = state_data; + + MemoryContextSwitchTo(mctx); + } + + fctx = SRF_PERCALL_SETUP(); + state_data = fctx->user_fctx; + + if (state_data->catId < SysCacheSize) + { + Datum values[5]; + bool nulls[5]; + HeapTuple resulttup; + Datum result; + int catId = state_data->catId++; + + memset(nulls, 0, sizeof(nulls)); + memset(values, 0, sizeof(values)); + values[0] = Int16GetDatum(catId); + values[1] = ObjectIdGetDatum(SysCache[catId]->cc_reloid); +#ifdef CATCACHE_STATS + values[2] = Int64GetDatum(SysCache[catId]->cc_searches); + values[3] = Int64GetDatum(SysCache[catId]->cc_hits); + values[4] = Int64GetDatum(SysCache[catId]->cc_neg_hits); +#endif + resulttup = heap_form_tuple(state_data->tupd, values, nulls); + result = HeapTupleGetDatum(resulttup); + + SRF_RETURN_NEXT(fctx, result); + } + + SRF_RETURN_DONE(fctx); +} + +Datum +catcachebench(PG_FUNCTION_ARGS) +{ + int testtype = PG_GETARG_INT32(0); + double ms; + + collectinfo(); + + /* flush the catalog -- safe? don't mind. */ + CatalogCacheFlushCatalog2(StatisticRelationId); + + switch (testtype) + { + case 0: + catcachewarmup(); /* prewarm of syscatalog */ + PG_RETURN_NULL(); + case 1: + ms = catcachebench1(); break; + case 2: + ms = catcachebench2(); break; + case 3: + ms = catcachebench3(); break; + default: + elog(ERROR, "Invalid test type: %d", testtype); + } + + PG_RETURN_DATUM(Float8GetDatum(ms)); +} + +/* + * fetch all attribute entires of all tables. + */ +double +catcachebench1(void) +{ + int t, a; + instr_time start, + duration; + + PG_SETMASK(&BlockSig); + INSTR_TIME_SET_CURRENT(start); + for (t = 0 ; t < ntables ; t++) + { + for (a = 0 ; a < natts ; a++) + { + HeapTuple tup; + + tup = SearchSysCache3(STATRELATTINH, + ObjectIdGetDatum(tableoids[t]), + Int16GetDatum(attnums[a]), + BoolGetDatum(false)); + /* should be null, but.. */ + if (HeapTupleIsValid(tup)) + ReleaseSysCache(tup); + } + } + INSTR_TIME_SET_CURRENT(duration); + INSTR_TIME_SUBTRACT(duration, start); + PG_SETMASK(&UnBlockSig); + + return INSTR_TIME_GET_MILLISEC(duration); +}; + +/* + * fetch all attribute entires of a table 6000 times. + */ +double +catcachebench2(void) +{ + int t, a; + instr_time start, + duration; + + PG_SETMASK(&BlockSig); + INSTR_TIME_SET_CURRENT(start); + for (t = 0 ; t < 240000 ; t++) + { + for (a = 0 ; a < natts ; a++) + { + HeapTuple tup; + + tup = SearchSysCache3(STATRELATTINH, + ObjectIdGetDatum(tableoids[0]), + Int16GetDatum(attnums[a]), + BoolGetDatum(false)); + /* should be null, but.. */ + if (HeapTupleIsValid(tup)) + ReleaseSysCache(tup); + } + } + INSTR_TIME_SET_CURRENT(duration); + INSTR_TIME_SUBTRACT(duration, start); + PG_SETMASK(&UnBlockSig); + + return INSTR_TIME_GET_MILLISEC(duration); +}; + +/* + * fetch all attribute entires of all tables twice with having expiration + * happen. + */ +double +catcachebench3(void) +{ + const int clock_step = 1000; + int i, t, a; + instr_time start, + duration; + + PG_SETMASK(&BlockSig); + INSTR_TIME_SET_CURRENT(start); + for (i = 0 ; i < 4 ; i++) + { + int ct = clock_step; + + for (t = 0 ; t < ntables ; t++) + { + /* + * catcacheclock is updated by transaction timestamp, so needs to + * be updated by other means for this test to work. Here I choosed + * to update the clock every 1000 tables scan. + */ + if (--ct < 0) + { + SetCatCacheClock(GetCurrentTimestamp()); + ct = clock_step; + } + for (a = 0 ; a < natts ; a++) + { + HeapTuple tup; + + tup = SearchSysCache3(STATRELATTINH, + ObjectIdGetDatum(tableoids[t]), + Int16GetDatum(attnums[a]), + BoolGetDatum(false)); + /* should be null, but.. */ + if (HeapTupleIsValid(tup)) + ReleaseSysCache(tup); + } + } + } + INSTR_TIME_SET_CURRENT(duration); + INSTR_TIME_SUBTRACT(duration, start); + PG_SETMASK(&UnBlockSig); + + return INSTR_TIME_GET_MILLISEC(duration); +}; + +void +catcachewarmup(void) +{ + int t, a; + + /* load up catalog tables */ + for (t = 0 ; t < ntables ; t++) + { + for (a = 0 ; a < natts ; a++) + { + HeapTuple tup; + + tup = SearchSysCache3(STATRELATTINH, + ObjectIdGetDatum(tableoids[t]), + Int16GetDatum(attnums[a]), + BoolGetDatum(false)); + /* should be null, but.. */ + if (HeapTupleIsValid(tup)) + ReleaseSysCache(tup); + } + } +} + +void +collectinfo(void) +{ + int ret; + Datum values[10000]; + bool nulls[10000]; + Oid types0[] = {OIDOID}; + int i; + + ntables = 0; + natts = 0; + + SPI_connect(); + /* collect target tables */ + ret = SPI_execute("select oid from pg_class where relnamespace = (select oid from pg_namespace where nspname = \'test\')", + true, 0); + if (ret != SPI_OK_SELECT) + elog(ERROR, "Failed 1"); + if (SPI_processed == 0) + elog(ERROR, "no relation found in schema \"test\""); + if (SPI_processed > 10000) + elog(ERROR, "too many relation found in schema \"test\""); + + for (i = 0 ; i < SPI_processed ; i++) + { + heap_deform_tuple(SPI_tuptable->vals[i], SPI_tuptable->tupdesc, + values, nulls); + if (nulls[0]) + elog(ERROR, "Failed 2"); + + tableoids[ntables++] = DatumGetObjectId(values[0]); + } + SPI_finish(); + elog(DEBUG1, "%d tables found", ntables); + + values[0] = ObjectIdGetDatum(tableoids[0]); + nulls[0] = false; + SPI_connect(); + ret = SPI_execute_with_args("select attnum from pg_attribute where attrelid = (select oid from pg_class where oid = $1)", + 1, types0, values, NULL, true, 0); + if (SPI_processed == 0) + elog(ERROR, "no attribute found in table %d", tableoids[0]); + if (SPI_processed > 10000) + elog(ERROR, "too many relation found in table %d", tableoids[0]); + + /* collect target attributes. assuming all tables have the same attnums */ + for (i = 0 ; i < SPI_processed ; i++) + { + int16 attnum; + + heap_deform_tuple(SPI_tuptable->vals[i], SPI_tuptable->tupdesc, + values, nulls); + if (nulls[0]) + elog(ERROR, "Failed 3"); + attnum = DatumGetInt16(values[0]); + + if (attnum > 0) + attnums[natts++] = attnum; + } + SPI_finish(); + elog(DEBUG1, "%d attributes found", natts); +} diff --git a/contrib/catcachebench/catcachebench.control b/contrib/catcachebench/catcachebench.control new file mode 100644 index 0000000000..3fc9d2e420 --- /dev/null +++ b/contrib/catcachebench/catcachebench.control @@ -0,0 +1,6 @@ +# catcachebench + +comment = 'benchmark for catcache pruning' +default_version = '0.0' +module_pathname = '$libdir/catcachebench' +relocatable = true diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c index 611b65168d..aabea861ce 100644 --- a/src/backend/utils/cache/catcache.c +++ b/src/backend/utils/cache/catcache.c @@ -767,6 +767,39 @@ CatalogCacheFlushCatalog(Oid catId) CACHE_elog(DEBUG2, "end of CatalogCacheFlushCatalog call"); } + +/* FUNCTION FOR BENCHMARKING */ +void +CatalogCacheFlushCatalog2(Oid catId) +{ + slist_iter iter; + + CACHE_elog(DEBUG2, "CatalogCacheFlushCatalog called for %u", catId); + + slist_foreach(iter, &CacheHdr->ch_caches) + { + CatCache *cache = slist_container(CatCache, cc_next, iter.cur); + + /* Does this cache store tuples of the target catalog? */ + if (cache->cc_reloid == catId) + { + /* Yes, so flush all its contents */ + ResetCatalogCache(cache); + + /* Tell inval.c to call syscache callbacks for this cache */ + CallSyscacheCallbacks(cache->id, 0); + + cache->cc_nbuckets = 128; + pfree(cache->cc_bucket); + cache->cc_bucket = palloc0(128 * sizeof(dlist_head)); + elog(LOG, "Catcache reset"); + } + } + + CACHE_elog(DEBUG2, "end of CatalogCacheFlushCatalog call"); +} +/* END: FUNCTION FOR BENCHMARKING */ + /* * InitCatCache * diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c index e4dc4ee34e..b60416ec63 100644 --- a/src/backend/utils/cache/syscache.c +++ b/src/backend/utils/cache/syscache.c @@ -994,7 +994,7 @@ static const struct cachedesc cacheinfo[] = { } }; -static CatCache *SysCache[SysCacheSize]; +CatCache *SysCache[SysCacheSize]; static bool CacheInitialized = false; -- 2.27.0 ----Next_Part(Thu_Jan_14_17_32_27_2021_995)---- ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX @ 2023-01-09 22:51 Nathan Bossart <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Nathan Bossart @ 2023-01-09 22:51 UTC (permalink / raw) To: Ted Yu <[email protected]>; +Cc: Jeff Davis <[email protected]>; Pavel Luzanov <[email protected]>; Justin Pryzby <[email protected]>; pgsql-hackers On Tue, Jan 03, 2023 at 03:45:49PM -0800, Nathan Bossart wrote: > I'd like to get this fixed, but I have yet to hear thoughts on the > suggested approach. I'll proceed with adjusting the tests and > documentation shortly unless someone objects. As promised, here is a new version of the patch with adjusted tests and documentation. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v4-0001-fix-MAINTAIN-privs.patch (25.6K, ../../20230109225157.GA1288965@nathanxps13/2-v4-0001-fix-MAINTAIN-privs.patch) download | inline diff: From e46033659ec3fd2bfeac7d816364b72eccb55410 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Mon, 9 Jan 2023 14:29:24 -0800 Subject: [PATCH v4 1/1] fix MAINTAIN privs --- doc/src/sgml/ref/analyze.sgml | 5 ++- doc/src/sgml/ref/cluster.sgml | 20 +++++++-- doc/src/sgml/ref/lock.sgml | 5 ++- doc/src/sgml/ref/reindex.sgml | 9 +++- doc/src/sgml/ref/vacuum.sgml | 8 +++- src/backend/catalog/partition.c | 22 ++++++++++ src/backend/catalog/toasting.c | 32 +++++++++++++++ src/backend/commands/cluster.c | 35 +++++++++++----- src/backend/commands/indexcmds.c | 10 ++--- src/backend/commands/lockcmds.c | 9 ++++ src/backend/commands/tablecmds.c | 41 ++++++++++++++++++- src/backend/commands/vacuum.c | 7 ++-- src/include/catalog/partition.h | 1 + src/include/catalog/pg_class.h | 1 + src/include/catalog/toasting.h | 1 + src/include/commands/tablecmds.h | 1 + .../expected/cluster-conflict-partition.out | 6 ++- .../specs/cluster-conflict-partition.spec | 5 +-- src/test/regress/expected/cluster.out | 4 +- src/test/regress/expected/vacuum.out | 18 -------- src/test/regress/sql/cluster.sql | 2 + 21 files changed, 189 insertions(+), 53 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index a26834da4f..2f94e89cb0 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -156,7 +156,10 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea analyze all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>ANALYZE</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) + with privileges of <literal>pg_maintain</literal>.) If a role has + permission to <command>ANALYZE</command> a partitioned table, it is also + permitted to <command>ANALYZE</command> each of its partitions, regardless + of whether the role has the aforementioned privileges on the partition. <command>ANALYZE</command> will skip over any tables that the calling user does not have permission to analyze. </para> diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 145101e6a5..5616293eef 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -69,10 +69,7 @@ CLUSTER [VERBOSE] <para> <command>CLUSTER</command> without any parameter reclusters all the previously-clustered tables in the current database that the calling user - owns or has the <literal>MAINTAIN</literal> privilege for, or all such tables - if called by a superuser or a role with privileges of the - <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. This form of <command>CLUSTER</command> cannot be + has privileges for. This form of <command>CLUSTER</command> cannot be executed inside a transaction block. </para> @@ -134,6 +131,21 @@ CLUSTER [VERBOSE] <refsect1> <title>Notes</title> + <para> + To cluster a table, one must have the <literal>MAINTAIN</literal> privilege + on the table or be the table's owner, a superuser, or a role with + privileges of the + <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> + role. If a role has permission to <command>CLUSTER</command> a partitioned + table, it is also permitted to <command>CLUSTER</command> each of its + partitions, regardless of whether the role has the aforementioned + privileges on the partition. Similarly, if a role has permission to + <command>CLUSTER</command> the main relation of a <acronym>TOAST</acronym> + table, it is also permitted to <command>CLUSTER</command> its + <acronym>TOAST</acronym> table. <command>CLUSTER</command> will skip over + any tables that the calling user does not have permission to cluster. + </para> + <para> In cases where you are accessing single rows randomly within a table, the actual order of the data in the diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index d9c5bf9a1d..21a9f88c70 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -176,7 +176,10 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] or <literal>TRUNCATE</literal> privileges on the target table. All other forms of <command>LOCK</command> are allowed with table-level <literal>UPDATE</literal>, <literal>DELETE</literal>, - or <literal>TRUNCATE</literal> privileges. + or <literal>TRUNCATE</literal> privileges. If a role has permission to + lock a partitioned table, it is also permitted to lock each of its + partitions, regardless of whether the role has the aforementioned + privileges on the partition. </para> <para> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 192513f34e..aa95c8743e 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -306,7 +306,14 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DA indexes on shared catalogs will be skipped unless the user owns the catalog (which typically won't be the case), has privileges of the <literal>pg_maintain</literal> role, or has the <literal>MAINTAIN</literal> - privilege on the catalog. Of course, superusers can always reindex anything. + privilege on the catalog. If a role has permission to + <command>REINDEX</command> a partitioned table, it is also permitted to + <command>REINDEX</command> each of its partitions, regardless of whether the + role has the aforementioned privileges on the partition. Similarly, if a + role has permission to <command>REINDEX</command> the main relation of a + <acronym>TOAST</acronym> table, it is also permitted to + <command>REINDEX</command> its <acronym>TOAST</acronym> table. Of course, + superusers can always reindex anything. </para> <para> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 8fa8421847..2e48757882 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -401,7 +401,13 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet vacuum all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>VACUUM</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) + with privileges of <literal>pg_maintain</literal>.) If a role has + permission to <command>VACUUM</command> a partitioned table, it is also + permitted to <command>VACUUM</command> each of its partitions, regardless + of whether the role has the aforementioned privileges on the partition. + Similarly, if a role has permission to <command>VACUUM</command> the main + relation of a <acronym>TOAST</acronym> table, it is also permitted to + <command>VACUUM</command> its <acronym>TOAST</acronym> table. <command>VACUUM</command> will skip over any tables that the calling user does not have permission to vacuum. </para> diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index f8780ce57d..5d2af2d0e7 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -119,6 +119,28 @@ get_partition_parent_worker(Relation inhRel, Oid relid, bool *detach_pending) return result; } +/* + * get_partition_root + * Obtain the partitioned table of a given relation + * + * Note: Because this function assumes that the relation whose OID is passed as + * an argument and each ancestor will have precisely one parent, it should only + * be called when it is known that the relation is a partition. + */ +Oid +get_partition_root(Oid relid) +{ + List *ancestors = get_partition_ancestors(relid); + Oid root = InvalidOid; + + if (list_length(ancestors) > 0) + root = llast_oid(ancestors); + + list_free(ancestors); + + return root; +} + /* * get_partition_ancestors * Obtain ancestors of given relation diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c index ab12b0b9de..36be5ab2e4 100644 --- a/src/backend/catalog/toasting.c +++ b/src/backend/catalog/toasting.c @@ -14,6 +14,7 @@ */ #include "postgres.h" +#include "access/genam.h" #include "access/heapam.h" #include "access/toast_compression.h" #include "access/xact.h" @@ -32,6 +33,7 @@ #include "nodes/makefuncs.h" #include "storage/lock.h" #include "utils/builtins.h" +#include "utils/fmgroids.h" #include "utils/rel.h" #include "utils/syscache.h" @@ -413,3 +415,33 @@ needs_toast_table(Relation rel) /* Otherwise, let the AM decide. */ return table_relation_needs_toast_table(rel); } + +/* + * Get the main relation for the given TOAST table. + */ +Oid +get_toast_parent(Oid relid) +{ + Relation rel; + SysScanDesc scan; + ScanKeyData key[1]; + Oid result = InvalidOid; + HeapTuple tuple; + + rel = table_open(RelationRelationId, AccessShareLock); + + ScanKeyInit(&key[0], + Anum_pg_class_reltoastrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(relid)); + + scan = systable_beginscan(rel, ClassToastRelidIndexId, true, NULL, 1, key); + tuple = systable_getnext(scan); + if (HeapTupleIsValid(tuple)) + result = ((Form_pg_class) GETSTRUCT(tuple))->oid; + systable_endscan(scan); + + table_close(rel, AccessShareLock); + + return result; +} diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index f11691aff7..0ef0edec63 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, static List *get_tables_to_cluster(MemoryContext cluster_context); static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid); +static bool cluster_is_permitted_for_relation(Oid relid, Oid userid); /*--------------------------------------------------------------------------- @@ -366,8 +367,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params) if (recheck) { /* Check that the user still has privileges for the relation */ - if (!object_ownercheck(RelationRelationId, tableOid, save_userid) && - pg_class_aclcheck(tableOid, save_userid, ACL_MAINTAIN) != ACLCHECK_OK) + if (!cluster_is_permitted_for_relation(tableOid, save_userid)) { relation_close(OldHeap, AccessExclusiveLock); goto out; @@ -1645,8 +1645,7 @@ get_tables_to_cluster(MemoryContext cluster_context) index = (Form_pg_index) GETSTRUCT(indexTuple); - if (!object_ownercheck(RelationRelationId, index->indrelid, GetUserId()) && - pg_class_aclcheck(index->indrelid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) + if (!cluster_is_permitted_for_relation(index->indrelid, GetUserId())) continue; /* Use a permanent memory context for the result list */ @@ -1694,12 +1693,11 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) if (get_rel_relkind(indexrelid) != RELKIND_INDEX) continue; - /* Silently skip partitions which the user has no access to. */ - if (!object_ownercheck(RelationRelationId, relid, GetUserId()) && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) || - IsSharedRelation(relid))) - continue; + /* + * We already checked that the user has privileges to CLUSTER the + * partitioned table when we locked it earlier, so there's no need to + * check the privileges again here. + */ /* Use a permanent memory context for the result list */ old_context = MemoryContextSwitchTo(cluster_context); @@ -1714,3 +1712,20 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) return rtcs; } + +/* + * Return whether userid has privileges to CLUSTER relid. If not, this + * function emits a WARNING. + */ +static bool +cluster_is_permitted_for_relation(Oid relid, Oid userid) +{ + if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK || + has_parent_privs(relid, userid, ACL_MAINTAIN)) + return true; + + ereport(WARNING, + (errmsg("permission denied to cluster \"%s\", skipping it", + get_rel_name(relid)))); + return false; +} diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 8afc006f89..31195ddb54 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2796,9 +2796,9 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, /* Check permissions */ table_oid = IndexGetRelation(relId, true); - if (!object_ownercheck(RelationRelationId, relId, GetUserId()) && - OidIsValid(table_oid) && - pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) + if (OidIsValid(table_oid) && + pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && + !has_parent_privs(table_oid, GetUserId(), ACL_MAINTAIN)) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, relation->relname); @@ -3016,8 +3016,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, * permission checks at the beginning of this routine. */ if (classtuple->relisshared && - !object_ownercheck(RelationRelationId, relid, GetUserId()) && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && + !has_parent_privs(relid, GetUserId(), ACL_MAINTAIN)) continue; /* diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 99e68bff85..8ec13242ad 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,6 +19,7 @@ #include "catalog/namespace.h" #include "catalog/pg_inherits.h" #include "commands/lockcmds.h" +#include "commands/tablecmds.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "parser/parse_clause.h" @@ -305,5 +306,13 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid) aclresult = pg_class_aclcheck(reloid, userid, aclmask); + /* + * If this is a partition or a TOAST table, check permissions of the parent + * table if needed. + */ + if (aclresult != ACLCHECK_OK && + has_parent_privs(reloid, userid, ACL_MAINTAIN)) + aclresult = ACLCHECK_OK; + return aclresult; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 1db3bd9e2e..59493b553a 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -16918,12 +16918,49 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, errmsg("\"%s\" is not a table or materialized view", relation->relname))); /* Check permissions */ - if (!object_ownercheck(RelationRelationId, relId, GetUserId()) && - pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) + if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && + !has_parent_privs(relId, GetUserId(), ACL_MAINTAIN)) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, relation->relname); } +/* + * If relid is a partition, return whether userid has any of the privileges + * specified in acl on its partitioned table. + * + * If relid is a TOAST table, return whether userid has any of the privileges + * specified in acl on its main relation. + */ +bool +has_parent_privs(Oid relid, Oid userid, AclMode acl) +{ + /* + * If this is a partition, check the permissions on its partitioned table. + */ + if (get_rel_relispartition(relid)) + { + Oid root = get_partition_root(relid); + + if (OidIsValid(root) && + pg_class_aclcheck(root, userid, acl) == ACLCHECK_OK) + return true; + } + + /* + * If this is a TOAST table, check the permissions on the main relation. + */ + if (get_rel_relkind(relid) == RELKIND_TOASTVALUE) + { + Oid parent = get_toast_parent(relid); + + if (OidIsValid(parent) && + pg_class_aclcheck(parent, userid, acl) == ACLCHECK_OK) + return true; + } + + return false; +} + /* * Callback to RangeVarGetRelidExtended() for TRUNCATE processing. */ diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index c4ed7efce3..ceb4cbbaf7 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -41,6 +41,7 @@ #include "catalog/pg_namespace.h" #include "commands/cluster.h" #include "commands/defrem.h" +#include "commands/tablecmds.h" #include "commands/vacuum.h" #include "miscadmin.h" #include "nodes/makefuncs.h" @@ -600,9 +601,9 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, * - the role has been granted the MAINTAIN privilege on the relation *---------- */ - if (object_ownercheck(RelationRelationId, relid, GetUserId()) || - (object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK) + if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK || + has_parent_privs(relid, GetUserId(), ACL_MAINTAIN)) return true; relname = NameStr(reltuple->relname); diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h index 79ce711802..abe6cf94b5 100644 --- a/src/include/catalog/partition.h +++ b/src/include/catalog/partition.h @@ -20,6 +20,7 @@ #define HASH_PARTITION_SEED UINT64CONST(0x7A5B22367996DCFD) extern Oid get_partition_parent(Oid relid, bool even_if_detached); +extern Oid get_partition_root(Oid relid); extern List *get_partition_ancestors(Oid relid); extern Oid index_get_partition(Relation partition, Oid indexId); extern List *map_partition_varattnos(List *expr, int fromrel_varno, diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h index 2d1bb7af3a..c57500e327 100644 --- a/src/include/catalog/pg_class.h +++ b/src/include/catalog/pg_class.h @@ -155,6 +155,7 @@ typedef FormData_pg_class *Form_pg_class; DECLARE_UNIQUE_INDEX_PKEY(pg_class_oid_index, 2662, ClassOidIndexId, on pg_class using btree(oid oid_ops)); DECLARE_UNIQUE_INDEX(pg_class_relname_nsp_index, 2663, ClassNameNspIndexId, on pg_class using btree(relname name_ops, relnamespace oid_ops)); DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, ClassTblspcRelfilenodeIndexId, on pg_class using btree(reltablespace oid_ops, relfilenode oid_ops)); +DECLARE_INDEX(pg_class_reltoastrelid_index, 2173, ClassToastRelidIndexId, on pg_class using btree(reltoastrelid oid_ops)); #ifdef EXPOSE_TO_CLIENT_CODE diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h index 5880ec6752..69f7fcc613 100644 --- a/src/include/catalog/toasting.h +++ b/src/include/catalog/toasting.h @@ -26,5 +26,6 @@ extern void AlterTableCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode); extern void BootstrapToastTable(char *relName, Oid toastOid, Oid toastIndexOid); +extern Oid get_toast_parent(Oid relid); #endif /* TOASTING_H */ diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 2e717fa815..eef7cc6cfe 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -98,6 +98,7 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit, extern void RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); +extern bool has_parent_privs(Oid relid, Oid userid, AclMode acl); extern void RangeVarCallbackOwnsRelation(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out index 7acb675c97..8d21276996 100644 --- a/src/test/isolation/expected/cluster-conflict-partition.out +++ b/src/test/isolation/expected/cluster-conflict-partition.out @@ -22,14 +22,16 @@ starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_res step s1_begin: BEGIN; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; step s2_auth: SET ROLE regress_cluster_part; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> step s1_commit: COMMIT; +step s2_cluster: <... completed> step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset step s1_begin: BEGIN; step s2_auth: SET ROLE regress_cluster_part; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> step s1_commit: COMMIT; +step s2_cluster: <... completed> step s2_reset: RESET ROLE; diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec index 5091f684a9..ae38cb4ee3 100644 --- a/src/test/isolation/specs/cluster-conflict-partition.spec +++ b/src/test/isolation/specs/cluster-conflict-partition.spec @@ -27,11 +27,8 @@ step s2_auth { SET ROLE regress_cluster_part; } step s2_cluster { CLUSTER cluster_part_tab USING cluster_part_ind; } step s2_reset { RESET ROLE; } -# CLUSTER on the parent waits if locked, passes for all cases. +# CLUSTER waits if locked, passes for all cases. permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset - -# When taking a lock on a partition leaf, CLUSTER on the parent skips -# the leaf, passes for all cases. permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 542c2e098c..2eec483eaa 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -352,7 +352,9 @@ INSERT INTO clstr_3 VALUES (1); -- this user can only cluster clstr_1 and clstr_3, but the latter -- has not been clustered SET SESSION AUTHORIZATION regress_clstr_user; +SET client_min_messages = ERROR; -- order of "skipping" warnings may vary CLUSTER; +RESET client_min_messages; SELECT * FROM clstr_1 UNION ALL SELECT * FROM clstr_2 UNION ALL SELECT * FROM clstr_3; @@ -513,7 +515,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a -----------+---------- ptnowner | t ptnowner1 | f - ptnowner2 | t + ptnowner2 | f (3 rows) DROP TABLE ptnowner; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index d860be0e20..458adee7f8 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -353,20 +353,14 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; SET ROLE regress_vacuum; VACUUM vacowned_parted; -WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; -WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; -WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; -WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; -WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; -WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; @@ -395,26 +389,14 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; -WARNING: permission denied to vacuum "vacowned_part1", skipping it -WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; -WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; -WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; -WARNING: permission denied to analyze "vacowned_part1", skipping it -WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; -WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; -WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; -WARNING: permission denied to vacuum "vacowned_part1", skipping it -WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; -WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; -WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; DROP TABLE vacowned; DROP TABLE vacowned_parted; diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql index 6cb9c926c0..a4cfaae807 100644 --- a/src/test/regress/sql/cluster.sql +++ b/src/test/regress/sql/cluster.sql @@ -145,7 +145,9 @@ INSERT INTO clstr_3 VALUES (1); -- this user can only cluster clstr_1 and clstr_3, but the latter -- has not been clustered SET SESSION AUTHORIZATION regress_clstr_user; +SET client_min_messages = ERROR; -- order of "skipping" warnings may vary CLUSTER; +RESET client_min_messages; SELECT * FROM clstr_1 UNION ALL SELECT * FROM clstr_2 UNION ALL SELECT * FROM clstr_3; -- 2.25.1 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX @ 2023-01-13 19:56 Jeff Davis <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Jeff Davis @ 2023-01-13 19:56 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; Ted Yu <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; Justin Pryzby <[email protected]>; pgsql-hackers On Mon, 2023-01-09 at 14:51 -0800, Nathan Bossart wrote: > On Tue, Jan 03, 2023 at 03:45:49PM -0800, Nathan Bossart wrote: > > I'd like to get this fixed, but I have yet to hear thoughts on the > > suggested approach. I'll proceed with adjusting the tests and > > documentation shortly unless someone objects. > > As promised, here is a new version of the patch with adjusted tests > and > documentation. The current patch doesn't handle the case properly where you have partitions that have toast tables. An easy fix by recursing, but I think we might want to do things differently anyway. I'm hesitant to add an index to pg_class just for the privilege checks on toast tables, and I don't think we need to. Instead, we can just skip the privilege check on a toast table if it's not referenced directly, because we already checked the privileges on the parent, and we still hold the session lock so nothing strange should have happened. I'll look into that, but so far it looks like it should come out cleanly for toast tables. The way you're checking privileges on the partitioned tables is fine. -- Jeff Davis PostgreSQL Contributor Team - AWS ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX @ 2023-01-13 20:33 Nathan Bossart <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Nathan Bossart @ 2023-01-13 20:33 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: Ted Yu <[email protected]>; Pavel Luzanov <[email protected]>; Justin Pryzby <[email protected]>; pgsql-hackers On Fri, Jan 13, 2023 at 11:56:03AM -0800, Jeff Davis wrote: > I'm hesitant to add an index to pg_class just for the privilege checks > on toast tables, and I don't think we need to. I bet this index will be useful for more than just these privilege checks (e.g., autovacuum currently creates a hash table for the toast-to-main-relation mapping), but I do understand the hesitation. > Instead, we can just > skip the privilege check on a toast table if it's not referenced > directly, because we already checked the privileges on the parent, and > we still hold the session lock so nothing strange should have happened. That would fix the problem in the original complaint, but it wouldn't allow for vacuuming toast tables directly if you only have MAINTAIN privileges on the main relation. If you can vacuum the toast table indirectly via the main relation, shouldn't it be possible to vacuum it directly? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-01-13 20:33 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-18 07:56 [PATCH v6 3/3] catcachebench Kyotaro Horiguchi <[email protected]> 2023-01-09 22:51 Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX Nathan Bossart <[email protected]> 2023-01-13 19:56 ` Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX Jeff Davis <[email protected]> 2023-01-13 20:33 ` Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox