public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v27 09/11] pg_ls_*/pg_stat_file to show file *type*.. 3+ messages / 3 participants [nested] [flat]
* [PATCH v27 09/11] pg_ls_*/pg_stat_file to show file *type*.. @ 2020-03-31 19:40 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2020-03-31 19:40 UTC (permalink / raw) ..not just "isdir" Also show special file types, now that their type is shown. --- doc/src/sgml/func.sgml | 26 +++++---- src/backend/utils/adt/genfile.c | 58 ++++++++++++++++---- src/include/catalog/pg_proc.dat | 36 ++++++------ src/test/regress/expected/misc_functions.out | 30 +++++----- src/test/regress/output/tablespace.source | 8 +-- src/test/regress/sql/misc_functions.sql | 4 +- 6 files changed, 101 insertions(+), 61 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index a01971d5fe..0a7083efac 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25808,7 +25808,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> For each file in the specified directory, list the file and its @@ -26858,13 +26858,13 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> For each file in the server's log directory, return the file's name, along with the metadata columns returned by <function>pg_stat_file</function>. - Filenames beginning with a dot and special file types are excluded. + Filenames beginning with a dot are excluded. </para> <para> This function is restricted to superusers and members of @@ -26886,13 +26886,13 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> For each file in the server's write-ahead log (WAL) directory, list the file's name along with the metadata columns returned by <function>pg_stat_file</function>. - Filenames beginning with a dot and special files types are excluded. + Filenames beginning with a dot are excluded. </para> <para> This function is restricted to superusers and members of @@ -26914,14 +26914,14 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> For each file in the server's WAL archive status directory (<filename>pg_wal/archive_status</filename>), list the file's name along with the metadata columns returned by <function>pg_stat_file</function>. - Filenames beginning with a dot and special file types are excluded. + Filenames beginning with a dot are excluded. </para> <para> This function is restricted to superusers and members of @@ -26944,7 +26944,7 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> For each file in the temporary directory within the given @@ -26954,7 +26954,7 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size Directories are used for temporary files shared by parallel processes. If <parameter>tablespace</parameter> is not provided, the <literal>pg_default</literal> tablespace is examined. - Filenames beginning with a dot and special file types are excluded. + Filenames beginning with a dot are excluded. </para> <para> This function is restricted to superusers and members of @@ -27028,13 +27028,15 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8'); <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> Returns a record containing the file's size, last access time stamp, last modification time stamp, last file status change time stamp (Unix - platforms only), file creation time stamp (Windows only), and a flag - indicating if it is a directory. + platforms only), file creation time stamp (Windows only), and a + character representing the file's type: regular (-), directory (d), link + or junction (l), character device (c), block device (b), fifo (p), + socket (s) or other/unknown (?). </para> <para> This function is restricted to superusers by default, but other users diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index 17e05b853e..af666d658b 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -36,11 +36,12 @@ #include "utils/syscache.h" #include "utils/timestamp.h" +static char get_file_type(mode_t mode, const char *path); static void values_from_stat(struct stat *fst, const char *path, Datum *values, bool *nulls); static Datum pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags); -#define LS_DIR_ISDIR (1<<0) /* Show column: isdir */ +#define LS_DIR_TYPE (1<<0) /* Show column: type */ #define LS_DIR_METADATA (1<<1) /* Show columns: mtime, size */ #define LS_DIR_MISSING_OK (1<<2) /* Ignore ENOENT if the toplevel dir is missing */ #define LS_DIR_SKIP_DOT_DIRS (1<<3) /* Do not show . or .. */ @@ -49,7 +50,7 @@ static Datum pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags #define LS_DIR_SKIP_SPECIAL (1<<6) /* Do not show special file types */ /* Shortcut for common behavior */ -#define LS_DIR_COMMON (LS_DIR_SKIP_HIDDEN | LS_DIR_SKIP_SPECIAL | LS_DIR_METADATA) +#define LS_DIR_COMMON (LS_DIR_SKIP_HIDDEN | LS_DIR_METADATA) /* * Convert a "text" filename argument to C string, and check it's allowable. @@ -402,6 +403,43 @@ pg_read_binary_file_all(PG_FUNCTION_ARGS) return pg_read_binary_file(fcinfo); } +/* Return a character indicating the type of file, or '?' if unknown type */ +static char +get_file_type(mode_t mode, const char *path) +{ + if (S_ISREG(mode)) + return '-'; + + if (S_ISDIR(mode)) + return 'd'; +#ifndef WIN32 + if (S_ISLNK(mode)) + return 'l'; +#else + if (pgwin32_is_junction(path)) + return 'l'; +#endif + +#ifdef S_ISCHR + if (S_ISCHR(mode)) + return 'c'; +#endif +#ifdef S_ISBLK + if (S_ISBLK(mode)) + return 'b'; +#endif +#ifdef S_ISFIFO + if (S_ISFIFO(mode)) + return 'p'; +#endif +#ifdef S_ISSOCK + if (S_ISSOCK(mode)) + return 's'; +#endif + + return '?'; +} + /* * Populate values and nulls from fst and path. * Used for pg_stat_file() and pg_ls_dir_files() @@ -421,7 +459,7 @@ values_from_stat(struct stat *fst, const char *path, Datum *values, bool *nulls) nulls[3] = true; values[4] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_ctime)); #endif - values[5] = BoolGetDatum(S_ISDIR(fst->st_mode)); + values[5] = CharGetDatum(get_file_type(fst->st_mode, path)); } /* @@ -471,7 +509,7 @@ pg_stat_file(PG_FUNCTION_ARGS) TupleDescInitEntry(tupdesc, (AttrNumber) 5, "creation", TIMESTAMPTZOID, -1, 0); TupleDescInitEntry(tupdesc, (AttrNumber) 6, - "isdir", BOOLOID, -1, 0); + "type", CHAROID, -1, 0); BlessTupleDesc(tupdesc); memset(nulls, false, sizeof(nulls)); @@ -540,10 +578,10 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags) MemoryContext oldcontext; TypeFuncClass tuptype ; - /* isdir depends on metadata */ - Assert(!(flags&LS_DIR_ISDIR) || (flags&LS_DIR_METADATA)); - /* Unreasonable to show isdir and skip dirs */ - Assert(!(flags&LS_DIR_ISDIR) || !(flags&LS_DIR_SKIP_DIRS)); + /* type depends on metadata */ + Assert(!(flags&LS_DIR_TYPE) || (flags&LS_DIR_METADATA)); + /* Unreasonable to show type and skip dirs XXX */ + Assert(!(flags&LS_DIR_TYPE) || !(flags&LS_DIR_SKIP_DIRS)); /* check the optional arguments */ if (PG_NARGS() == 3) @@ -741,7 +779,7 @@ pg_ls_dir_metadata(PG_FUNCTION_ARGS) char *dirname = convert_and_check_filename(PG_GETARG_TEXT_PP(0)); return pg_ls_dir_files(fcinfo, dirname, - LS_DIR_METADATA | LS_DIR_SKIP_SPECIAL | LS_DIR_ISDIR); + LS_DIR_METADATA | LS_DIR_TYPE); } /* @@ -756,5 +794,5 @@ pg_ls_dir_metadata_1arg(PG_FUNCTION_ARGS) char *dirname = convert_and_check_filename(PG_GETARG_TEXT_PP(0)); return pg_ls_dir_files(fcinfo, dirname, - LS_DIR_METADATA | LS_DIR_SKIP_SPECIAL | LS_DIR_ISDIR); + LS_DIR_METADATA | LS_DIR_TYPE); } diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 491422c9f5..0a9d1e46bd 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -6302,16 +6302,16 @@ { oid => '2623', descr => 'get information about file', proname => 'pg_stat_file', provolatile => 'v', prorettype => 'record', proargtypes => 'text', - proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', + proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{i,o,o,o,o,o,o}', - proargnames => '{filename,size,access,modification,change,creation,isdir}', + proargnames => '{filename,size,access,modification,change,creation,type}', prosrc => 'pg_stat_file_1arg' }, { oid => '3307', descr => 'get information about file', proname => 'pg_stat_file', provolatile => 'v', prorettype => 'record', proargtypes => 'text bool', - proallargtypes => '{text,bool,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', + proallargtypes => '{text,bool,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{i,i,o,o,o,o,o,o}', - proargnames => '{filename,missing_ok,size,access,modification,change,creation,isdir}', + proargnames => '{filename,missing_ok,size,access,modification,change,creation,type}', prosrc => 'pg_stat_file' }, { oid => '2624', descr => 'read text from a file', proname => 'pg_read_file', provolatile => 'v', prorettype => 'text', @@ -11493,41 +11493,41 @@ { oid => '3353', descr => 'list files in the log directory', proname => 'pg_ls_logdir', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => '', - proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}', - proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_logdir' }, + proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{o,o,o,o,o,o,o}', + proargnames => '{name,size,access,modification,change,creation,type}', prosrc => 'pg_ls_logdir' }, { oid => '3354', descr => 'list of files in the WAL directory', proname => 'pg_ls_waldir', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => '', - proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}', - proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_waldir' }, + proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{o,o,o,o,o,o,o}', + proargnames => '{name,size,access,modification,change,creation,type}', prosrc => 'pg_ls_waldir' }, { oid => '5031', descr => 'list of files in the archive_status directory', proname => 'pg_ls_archive_statusdir', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', - proargtypes => '', proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', - proargmodes => '{o,o,o,o,o,o,o}', proargnames => '{name,size,access,modification,change,creation,isdir}', + proargtypes => '', proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', + proargmodes => '{o,o,o,o,o,o,o}', proargnames => '{name,size,access,modification,change,creation,type}', prosrc => 'pg_ls_archive_statusdir' }, { oid => '5029', descr => 'list files in the pgsql_tmp directory', proname => 'pg_ls_tmpdir', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => '', - proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}', - proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_tmpdir_noargs' }, + proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{o,o,o,o,o,o,o}', + proargnames => '{name,size,access,modification,change,creation,type}', prosrc => 'pg_ls_tmpdir_noargs' }, { oid => '5030', descr => 'list files in the pgsql_tmp directory', proname => 'pg_ls_tmpdir', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => 'oid', - proallargtypes => '{oid,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}', - proargnames => '{tablespace,name,size,access,modification,change,creation,isdir}', + proallargtypes => '{oid,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{i,o,o,o,o,o,o,o}', + proargnames => '{tablespace,name,size,access,modification,change,creation,type}', prosrc => 'pg_ls_tmpdir_1arg' }, { oid => '8450', descr => 'list directory with metadata', proname => 'pg_ls_dir_metadata', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => 'text bool bool', - proallargtypes => '{text,bool,bool,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,i,i,o,o,o,o,o,o,o}', - proargnames => '{dirname,missing_ok,include_dot_dirs,filename,size,access,modification,change,creation,isdir}', + proallargtypes => '{text,bool,bool,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{i,i,i,o,o,o,o,o,o,o}', + proargnames => '{dirname,missing_ok,include_dot_dirs,filename,size,access,modification,change,creation,type}', prosrc => 'pg_ls_dir_metadata' }, { oid => '8451', descr => 'list directory with metadata', proname => 'pg_ls_dir_metadata', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => 'text', - proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}', - proargnames => '{dirname,filename,size,access,modification,change,creation,isdir}', + proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{i,o,o,o,o,o,o,o}', + proargnames => '{dirname,filename,size,access,modification,change,creation,type}', prosrc => 'pg_ls_dir_metadata_1arg' }, # hash partitioning constraint function diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index ddd35b79f5..db3d64de3b 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -170,8 +170,8 @@ select count(*) > 0 as ok from (select pg_ls_waldir()) ss; -- Test not-run-to-completion cases. select * from pg_ls_waldir() limit 0; - name | size | access | modification | change | creation | isdir -------+------+--------+--------------+--------+----------+------- + name | size | access | modification | change | creation | type +------+------+--------+--------------+--------+----------+------ (0 rows) select count(*) > 0 as ok from (select * from pg_ls_waldir() limit 1) ss; @@ -234,32 +234,32 @@ select pg_ls_dir('does not exist'); -- fails with missingok=false ERROR: could not open directory "does not exist": No such file or directory -- Check that expected columns are present select * from pg_stat_file('.') limit 0; - size | access | modification | change | creation | isdir -------+--------+--------------+--------+----------+------- + size | access | modification | change | creation | type +------+--------+--------------+--------+----------+------ (0 rows) -- This tests the missing_ok parameter, which causes pg_ls_tmpdir to succeed even if the tmpdir doesn't exist yet -- The name='' condition is never true, so the function runs to completion but returns zero rows. select * from pg_ls_tmpdir() where name='Does not exist'; - name | size | access | modification | change | creation | isdir -------+------+--------+--------------+--------+----------+------- + name | size | access | modification | change | creation | type +------+------+--------+--------------+--------+----------+------ (0 rows) -select filename, isdir from pg_ls_dir_metadata('.') where filename='.'; - filename | isdir -----------+------- - . | t +select filename, type from pg_ls_dir_metadata('.') where filename='.'; + filename | type +----------+------ + . | d (1 row) -select filename, isdir from pg_ls_dir_metadata('.', false, false) where filename='.'; -- include_dot_dirs=false - filename | isdir -----------+------- +select filename, type from pg_ls_dir_metadata('.', false, false) where filename='.'; -- include_dot_dirs=false + filename | type +----------+------ (0 rows) -- Check that expected columns are present select * from pg_ls_dir_metadata('.') limit 0; - filename | size | access | modification | change | creation | isdir -----------+------+--------+--------------+--------+----------+------- + filename | size | access | modification | change | creation | type +----------+------+--------+--------------+--------+----------+------ (0 rows) -- diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source index 34f874d3f1..21f3e68392 100644 --- a/src/test/regress/output/tablespace.source +++ b/src/test/regress/output/tablespace.source @@ -17,15 +17,15 @@ CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@'; -- The name='' condition is never true, so the function runs to completion but returns zero rows. -- The query is written to ERROR if the tablespace doesn't exist, rather than silently failing to call pg_ls_tmpdir() SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1) AS b , pg_ls_tmpdir(oid) AS c WHERE c.name='Does not exist'; - name | size | access | modification | change | creation | isdir -------+------+--------+--------------+--------+----------+------- + name | size | access | modification | change | creation | type +------+------+--------+--------------+--------+----------+------ (0 rows) -- This tests the missing_ok parameter. If that's not functioning, this would ERROR if the logdir doesn't exist yet. -- The name='' condition is never true, so the function runs to completion but returns zero rows. SELECT * FROM pg_ls_logdir() WHERE name='Does not exist'; - name | size | access | modification | change | creation | isdir -------+------+--------+--------------+--------+----------+------- + name | size | access | modification | change | creation | type +------+------+--------+--------------+--------+----------+------ (0 rows) -- try setting and resetting some properties for the new tablespace diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index c169e527d9..55bed7b9e0 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -81,9 +81,9 @@ select * from pg_stat_file('.') limit 0; -- The name='' condition is never true, so the function runs to completion but returns zero rows. select * from pg_ls_tmpdir() where name='Does not exist'; -select filename, isdir from pg_ls_dir_metadata('.') where filename='.'; +select filename, type from pg_ls_dir_metadata('.') where filename='.'; -select filename, isdir from pg_ls_dir_metadata('.', false, false) where filename='.'; -- include_dot_dirs=false +select filename, type from pg_ls_dir_metadata('.', false, false) where filename='.'; -- include_dot_dirs=false -- Check that expected columns are present select * from pg_ls_dir_metadata('.') limit 0; -- 2.17.0 --19uQFt6ulqmgNgg1 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v27-0010-Preserve-pg_stat_file-isdir.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs @ 2024-03-26 16:59 Nathan Bossart <[email protected]> 2024-03-26 18:16 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Tom Lane <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Nathan Bossart @ 2024-03-26 16:59 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]> Here is a new version of the patch that I feel is in decent shape. On Mon, Mar 25, 2024 at 10:16:47AM -0500, Nathan Bossart wrote: > On Mon, Mar 25, 2024 at 11:08:39AM -0400, Tom Lane wrote: >> * The magic constants (crossover list length and bloom filter size) >> need some testing to see if there are better values. They should >> probably be made into named #defines, too. I suspect, with little >> proof, that the bloom filter size isn't particularly critical --- but >> I know we pulled the crossover of 1000 out of thin air, and I have >> no certainty that it's even within an order of magnitude of being a >> good choice. > > I'll try to construct a couple of tests to see if we can determine a proper > order of magnitude. I spent some time trying to get some ballpark figures but have thus far been unsuccessful. Even if I was able to get good numbers, I'm not sure how much they'd help us, as we'll still need to decide how much overhead we are willing to take in comparison to the linear search. I don't think ~1000 is an unreasonable starting point, as it seems generally more likely that you will have many more roles to process at that point than if the threshold was, say, 100. And if the threshold is too high (e.g., 10,000), this optimization will only kick in for the most extreme cases, so we'd likely be leaving a lot on the table. But, I will be the first to admit that my reasoning here is pretty unscientific, and I'm open to suggestions for how to make it less so. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v3-0001-Optimize-roles_is_member_of-with-a-Bloom-filter.patch (4.4K, ../../20240326165918.GA3350222@nathanxps13/2-v3-0001-Optimize-roles_is_member_of-with-a-Bloom-filter.patch) download | inline diff: From 95fe6e811990895acb9f79544f502408ac472203 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Mon, 25 Mar 2024 23:17:08 -0500 Subject: [PATCH v3 1/1] Optimize roles_is_member_of() with a Bloom filter. When the list of roles gathered by roles_is_member_of() grows very large, a Bloom filter is created to help avoid some linear searches through the list. The threshold for creating the Bloom filter is set arbitrarily high and may require future adjustment. Suggested-by: Tom Lane Reviewed-by: Tom Lane Discussion: https://postgr.es/m/CAGvXd3OSMbJQwOSc-Tq-Ro1CAz%3DvggErdSG7pv2s6vmmTOLJSg%40mail.gmail.com --- src/backend/utils/adt/acl.c | 71 +++++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 3 deletions(-) diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index cf5d08576a..7c4b642ebd 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -36,6 +36,7 @@ #include "common/hashfn.h" #include "foreign/foreign.h" #include "funcapi.h" +#include "lib/bloomfilter.h" #include "lib/qunique.h" #include "miscadmin.h" #include "utils/acl.h" @@ -78,6 +79,17 @@ static Oid cached_role[] = {InvalidOid, InvalidOid, InvalidOid}; static List *cached_roles[] = {NIL, NIL, NIL}; static uint32 cached_db_hash; +/* + * If the list of roles gathered by roles_is_member_of() grows larger than the + * below threshold, a Bloom filter is created to speed up list membership + * checks. This threshold is set arbitrarily high to avoid the overhead of + * creating the Bloom filter until it seems likely to provide a net benefit. + * + * XXX: The current threshold of 1024 is little more than a wild guess and may + * need to be adjusted in the future. + */ +#define ROLES_LIST_BLOOM_THRESHOLD 1024 +static bloom_filter *roles_list_bf = NULL; static const char *getid(const char *s, char *n, Node *escontext); static void putid(char *p, const char *s); @@ -4918,6 +4930,54 @@ RoleMembershipCacheCallback(Datum arg, int cacheid, uint32 hashvalue) cached_role[ROLERECURSE_SETROLE] = InvalidOid; } +/* + * A helper function for roles_is_member_of() that provides an optimized + * implementation of list_append_unique_oid() via a Bloom filter. The caller + * (i.e., roles_is_member_of()) is responsible for freeing roles_list_bf once + * it is done using this function. + */ +static inline List * +roles_list_append(List *roles_list, Oid role) +{ + unsigned char *roleptr = (unsigned char *) &role; + + /* + * If there is a previously-created Bloom filter, use it to determine + * whether the role is missing from the list. Otherwise, do an ordinary + * linear search through the existing role list. + */ + if ((roles_list_bf && + bloom_lacks_element(roles_list_bf, roleptr, sizeof(Oid))) || + !list_member_oid(roles_list, role)) + { + /* + * If the list is large, we take on the overhead of creating and + * populating a Bloom filter to speed up future calls to this + * function. + */ + if (!roles_list_bf && + list_length(roles_list) > ROLES_LIST_BLOOM_THRESHOLD) + { + roles_list_bf = bloom_create(ROLES_LIST_BLOOM_THRESHOLD * 10, + work_mem, 0); + foreach_oid(roleid, roles_list) + bloom_add_element(roles_list_bf, + (unsigned char *) &roleid, + sizeof(Oid)); + } + + /* + * Finally, add the role to the list and the Bloom filter, if it + * exists. + */ + roles_list = lappend_oid(roles_list, role); + if (roles_list_bf) + bloom_add_element(roles_list_bf, roleptr, sizeof(Oid)); + } + + return roles_list; +} + /* * Get a list of roles that the specified roleid is a member of * @@ -5023,16 +5083,21 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type, * graph, we must test for having already seen this role. It is * legal for instance to have both A->B and A->C->B. */ - roles_list = list_append_unique_oid(roles_list, otherid); + roles_list = roles_list_append(roles_list, otherid); } ReleaseSysCacheList(memlist); /* implement pg_database_owner implicit membership */ if (memberid == dba && OidIsValid(dba)) - roles_list = list_append_unique_oid(roles_list, - ROLE_PG_DATABASE_OWNER); + roles_list = roles_list_append(roles_list, ROLE_PG_DATABASE_OWNER); } + /* + * Free the Bloom filter created by roles_list_append(), if there is one. + */ + if (roles_list_bf) + bloom_free(roles_list_bf); + /* * Copy the completed list into TopMemoryContext so it will persist. */ -- 2.25.1 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs 2024-03-26 16:59 Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]> @ 2024-03-26 18:16 ` Tom Lane <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Tom Lane @ 2024-03-26 18:16 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]> Nathan Bossart <[email protected]> writes: > I spent some time trying to get some ballpark figures but have thus far > been unsuccessful. Even if I was able to get good numbers, I'm not sure > how much they'd help us, as we'll still need to decide how much overhead we > are willing to take in comparison to the linear search. I don't think > ~1000 is an unreasonable starting point, as it seems generally more likely > that you will have many more roles to process at that point than if the > threshold was, say, 100. And if the threshold is too high (e.g., 10,000), > this optimization will only kick in for the most extreme cases, so we'd > likely be leaving a lot on the table. But, I will be the first to admit > that my reasoning here is pretty unscientific, and I'm open to suggestions > for how to make it less so. I did a little experimentation using the attached quick-hack C function, and came to the conclusion that setting up the bloom filter costs more or less as much as inserting 1000 or so OIDs the dumb way. So we definitely want a threshold that's not much less than that. For example, with ROLES_LIST_BLOOM_THRESHOLD = 100 I saw: regression=# select drive_bloom(100, 10, 100000); drive_bloom ------------- (1 row) Time: 319.931 ms regression=# select drive_bloom(101, 10, 100000); drive_bloom ------------- (1 row) Time: 319.385 ms regression=# select drive_bloom(102, 10, 100000); drive_bloom ------------- (1 row) Time: 9904.786 ms (00:09.905) That's a pretty big jump in context. With the threshold set to 1024, regression=# select drive_bloom(1024, 10, 100000); drive_bloom ------------- (1 row) Time: 14597.510 ms (00:14.598) regression=# select drive_bloom(1025, 10, 100000); drive_bloom ------------- (1 row) Time: 14589.197 ms (00:14.589) regression=# select drive_bloom(1026, 10, 100000); drive_bloom ------------- (1 row) Time: 25947.000 ms (00:25.947) regression=# select drive_bloom(1027, 10, 100000); drive_bloom ------------- (1 row) Time: 25399.718 ms (00:25.400) regression=# select drive_bloom(2048, 10, 100000); drive_bloom ------------- (1 row) Time: 33809.536 ms (00:33.810) So I'm now content with choosing a threshold of 1000 or 1024 or so. As for the bloom filter size, I see that bloom_create does bitset_bytes = Min(bloom_work_mem * UINT64CONST(1024), total_elems * 2); bitset_bytes = Max(1024 * 1024, bitset_bytes); which means that any total_elems input less than 512K is disregarded altogether. So I'm not sold on your "ROLES_LIST_BLOOM_THRESHOLD * 10" value. Maybe it doesn't matter though. I do not like, even a little bit, your use of a static variable to hold the bloom filter pointer. That code will misbehave horribly if we throw an error partway through the role-accumulation loop; the next call will try to carry on using the old filter, which would be wrong even if it still existed which it likely won't. It's not that much worse notationally to keep it as a local variable, as I did in the attached. regards, tom lane Attachments: [text/x-c] drive_bloom.c (2.8K, ../../[email protected]/2-drive_bloom.c) download | inline: /* create function drive_bloom(num_oids int, dup_freq int, count int) returns void strict volatile language c as '/path/to/drive_bloom.so'; \timing on select drive_bloom(100, 0, 100000); */ #include "postgres.h" #include "fmgr.h" #include "lib/bloomfilter.h" #include "miscadmin.h" #include "tcop/tcopprot.h" #include "utils/builtins.h" #include "utils/memutils.h" PG_MODULE_MAGIC; #define ROLES_LIST_BLOOM_THRESHOLD 1024 #define ROLES_LIST_BLOOM_SIZE (1024 * 1024) static inline List * roles_list_append(List *roles_list, Oid role, bloom_filter **roles_list_bf) { unsigned char *roleptr = (unsigned char *) &role; /* * If there is a previously-created Bloom filter, use it to determine * whether the role is missing from the list. Otherwise, do an ordinary * linear search through the existing role list. */ if ((*roles_list_bf && bloom_lacks_element(*roles_list_bf, roleptr, sizeof(Oid))) || !list_member_oid(roles_list, role)) { /* * If the list is large, we take on the overhead of creating and * populating a Bloom filter to speed up future calls to this * function. */ if (!*roles_list_bf && list_length(roles_list) > ROLES_LIST_BLOOM_THRESHOLD) { *roles_list_bf = bloom_create(ROLES_LIST_BLOOM_SIZE, work_mem, 0); foreach_oid(roleid, roles_list) bloom_add_element(*roles_list_bf, (unsigned char *) &roleid, sizeof(Oid)); } /* * Finally, add the role to the list and the Bloom filter, if it * exists. */ roles_list = lappend_oid(roles_list, role); if (*roles_list_bf) bloom_add_element(*roles_list_bf, roleptr, sizeof(Oid)); } return roles_list; } /* * drive_bloom(num_oids int, dup_freq int, count int) returns void * * num_oids: number of OIDs to de-duplicate * dup_freq: if > 0, every dup_freq'th OID is duplicated * count: overall repetition count; choose large enough to get reliable timing */ PG_FUNCTION_INFO_V1(drive_bloom); Datum drive_bloom(PG_FUNCTION_ARGS) { int32 num_oids = PG_GETARG_INT32(0); int32 dup_freq = PG_GETARG_INT32(1); int32 count = PG_GETARG_INT32(2); MemoryContext mycontext; mycontext = AllocSetContextCreate(CurrentMemoryContext, "drive_bloom work cxt", ALLOCSET_DEFAULT_SIZES); while (count-- > 0) { List *roles_list = NIL; Oid nextrole = 1; bloom_filter *roles_list_bf = NULL; MemoryContext oldcontext; oldcontext = MemoryContextSwitchTo(mycontext); for (int i = 0; i < num_oids; i++) { roles_list = roles_list_append(roles_list, nextrole, &roles_list_bf); if (dup_freq > 0 && i % dup_freq == 0) roles_list = roles_list_append(roles_list, nextrole, &roles_list_bf); nextrole++; } if (roles_list_bf) bloom_free(roles_list_bf); MemoryContextSwitchTo(oldcontext); MemoryContextReset(mycontext); CHECK_FOR_INTERRUPTS(); } PG_RETURN_VOID(); } ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-03-26 18:16 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-31 19:40 [PATCH v27 09/11] pg_ls_*/pg_stat_file to show file *type*.. Justin Pryzby <[email protected]> 2024-03-26 16:59 Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]> 2024-03-26 18:16 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Tom Lane <[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