public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v24 09/11] pg_ls_*/pg_stat_file to show file *type*..
7+ messages / 6 participants
[nested] [flat]
* [PATCH v24 09/11] pg_ls_*/pg_stat_file to show file *type*..
@ 2020-03-31 19:40 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 7+ 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 e3d7df2dd7..55e9f5a8b2 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25747,13 +25747,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
@@ -25777,7 +25777,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 specified directory, list the file and its
@@ -25800,13 +25800,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
@@ -25828,14 +25828,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
@@ -25858,7 +25858,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
@@ -25868,7 +25868,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
@@ -25942,13 +25942,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 9ff06f4158..6e8898029f 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 tuple_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.
@@ -399,6 +400,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()
@@ -418,7 +456,7 @@ tuple_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));
}
/*
@@ -468,7 +506,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));
@@ -537,10 +575,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)
@@ -738,7 +776,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);
}
/*
@@ -753,5 +791,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 7fea405068..12a7391e73 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6202,16 +6202,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',
@@ -10951,41 +10951,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 => '9979', 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 => '9980', 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 1f4106cd9c..5b87ab4844 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -157,8 +157,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;
@@ -221,32 +221,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 1e1e02b589..025c9709a1 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 64d9d70874..0961cdc058 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -72,9 +72,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
--mPTHnM80CEnHQ2WJ
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v24-0010-Preserve-pg_stat_file-isdir.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid.
@ 2024-06-14 07:45 Anton A. Melnikov <[email protected]>
2024-06-18 06:57 ` Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Anton A. Melnikov <[email protected]>
2024-06-19 17:21 ` Re: Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid. Alvaro Herrera <[email protected]>
0 siblings, 2 replies; 7+ messages in thread
From: Anton A. Melnikov @ 2024-06-14 07:45 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
Hello!
The src/backend/access/heap/README.tuplock says about HEAP_XMAX_INVALID bit
that "Any tuple with this bit set does not have a valid value stored in XMAX."
Found that FreezeMultiXactId() tries to process such an invalid multi xmax
and may looks for an update xid in the pg_multixact for it.
Maybe not do this work in FreezeMultiXactId() and exit immediately if the
bit HEAP_XMAX_INVALID was already set?
For instance, like that:
master
@@ -6215,6 +6215,15 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
/* We should only be called in Multis */
Assert(t_infomask & HEAP_XMAX_IS_MULTI);
+ /* Xmax is already marked as invalid */
+ if (MultiXactIdIsValid(multi) &&
+ (t_infomask & HEAP_XMAX_INVALID))
+ {
+ *flags |= FRM_INVALIDATE_XMAX;
+ pagefrz->freeze_required = true;
+ return InvalidTransactionId;
+ }
+
if (!MultiXactIdIsValid(multi) ||
HEAP_LOCKED_UPGRADED(t_infomask))
With the best regards,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
^ permalink raw reply [nested|flat] 7+ messages in thread
* Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid?
2024-06-14 07:45 Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid. Anton A. Melnikov <[email protected]>
@ 2024-06-18 06:57 ` Anton A. Melnikov <[email protected]>
2024-06-18 14:29 ` Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Maxim Orlov <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Anton A. Melnikov @ 2024-06-18 06:57 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
On 14.06.2024 10:45, Anton A. Melnikov wrote:
> The src/backend/access/heap/README.tuplock says about HEAP_XMAX_INVALID bit
> that "Any tuple with this bit set does not have a valid value stored in XMAX."
>
> Found that FreezeMultiXactId() tries to process such an invalid multi xmax
> and may looks for an update xid in the pg_multixact for it.
>
> Maybe not do this work in FreezeMultiXactId() and exit immediately if the
> bit HEAP_XMAX_INVALID was already set?
>
Seems it is important to save the check that multi xmax is not behind relminmxid.
So saved it and correct README.tuplock accordingly.
Would be glad if someone take a look at the patch attached.
With the best regards,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
[text/x-patch] v1-0001-Dont-process-multi-xmax-in-the-FreezeMultiXactId.patch (1.7K, ../../[email protected]/2-v1-0001-Dont-process-multi-xmax-in-the-FreezeMultiXactId.patch)
download | inline diff:
From 7608078bddf35904427cb1c04497ab0c98e9d111 Mon Sep 17 00:00:00 2001
From: "Anton A. Melnikov" <[email protected]>
Date: Fri, 14 Jun 2024 10:42:55 +0300
Subject: [PATCH] Don't process multi xmax in the FreezeMultiXactId() if the
HEAP_XMAX_INVALID bit was already set.
---
src/backend/access/heap/README.tuplock | 1 +
src/backend/access/heap/heapam.c | 8 ++++++++
2 files changed, 9 insertions(+)
diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index 6441e8baf0..e198942647 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -124,6 +124,7 @@ The following infomask bits are applicable:
- HEAP_XMAX_INVALID
Any tuple with this bit set does not have a valid value stored in XMAX.
+ Although if it's a multi xmax it must follow relminmxid.
- HEAP_XMAX_IS_MULTI
This bit is set if the tuple's Xmax is a MultiXactId (as opposed to a
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 82bb9cb33b..d1cba6970d 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6227,6 +6227,14 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
(errcode(ERRCODE_DATA_CORRUPTED),
errmsg_internal("found multixact %u from before relminmxid %u",
multi, cutoffs->relminmxid)));
+ else if (MultiXactIdIsValid(multi) &&
+ (t_infomask & HEAP_XMAX_INVALID))
+ {
+ /* Xmax is already marked as invalid */
+ *flags |= FRM_INVALIDATE_XMAX;
+ pagefrz->freeze_required = true;
+ return InvalidTransactionId;
+ }
else if (MultiXactIdPrecedes(multi, cutoffs->OldestMxact))
{
TransactionId update_xact;
--
2.45.2
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid?
2024-06-14 07:45 Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid. Anton A. Melnikov <[email protected]>
2024-06-18 06:57 ` Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Anton A. Melnikov <[email protected]>
@ 2024-06-18 14:29 ` Maxim Orlov <[email protected]>
2024-06-18 15:47 ` Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Maxim Orlov @ 2024-06-18 14:29 UTC (permalink / raw)
To: Anton A. Melnikov <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Hi!
Maybe, I'm too bold, but looks like a kinda bug to me. At least, I don't
understand why we do not check the HEAP_XMAX_INVALID flag.
My guess is nobody noticed, that MultiXactIdIsValid call does not check the
mentioned flag in the "first" condition, but it's all my speculation.
Does anyone know if there are reasons to deliberately ignore the HEAP_XMAX
INVALID flag? Or this is just an unfortunate oversight.
PFA, my approach on this issue.
--
Best regards,
Maxim Orlov.
Attachments:
[application/octet-stream] v2-0001-Invalidate-xmax-if-HEAP_XMAX_INVALID-is-set.patch (772B, ../../CACG=ezbB2q2wYZseBA5mcNKXZwamPRb2dzzqrRtJ37E=K7do2w@mail.gmail.com/3-v2-0001-Invalidate-xmax-if-HEAP_XMAX_INVALID-is-set.patch)
download | inline diff:
From 90867d80a081de1509dc57938e39ed02649f88ab Mon Sep 17 00:00:00 2001
From: Maxim Orlov <[email protected]>
Date: Tue, 18 Jun 2024 17:17:06 +0300
Subject: [PATCH v2] Invalidate xmax if HEAP_XMAX_INVALID is set.
---
src/backend/access/heap/heapam.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 82bb9cb33b6..0c4db5b225f 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6216,6 +6216,7 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
Assert(t_infomask & HEAP_XMAX_IS_MULTI);
if (!MultiXactIdIsValid(multi) ||
+ (t_infomask & HEAP_XMAX_INVALID) ||
HEAP_LOCKED_UPGRADED(t_infomask))
{
*flags |= FRM_INVALIDATE_XMAX;
--
2.45.2
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid?
2024-06-14 07:45 Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid. Anton A. Melnikov <[email protected]>
2024-06-18 06:57 ` Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Anton A. Melnikov <[email protected]>
2024-06-18 14:29 ` Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Maxim Orlov <[email protected]>
@ 2024-06-18 15:47 ` Peter Geoghegan <[email protected]>
2024-06-19 17:00 ` Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Yura Sokolov <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Geoghegan @ 2024-06-18 15:47 UTC (permalink / raw)
To: Maxim Orlov <[email protected]>; +Cc: Anton A. Melnikov <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Jun 18, 2024 at 10:29 AM Maxim Orlov <[email protected]> wrote:
> Maybe, I'm too bold, but looks like a kinda bug to me. At least, I don't understand why we do not check the HEAP_XMAX_INVALID flag.
> My guess is nobody noticed, that MultiXactIdIsValid call does not check the mentioned flag in the "first" condition, but it's all my speculation.
A related code path was changed in commit 02d647bbf0. That change made
the similar xmax handling that covers XIDs (not MXIDs) *stop* doing
what you're now proposing to do in the Multi path.
Why do you think this is a bug?
> Does anyone know if there are reasons to deliberately ignore the HEAP_XMAX INVALID flag? Or this is just an unfortunate oversight.
HEAP_XMAX_INVALID is just a hint.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid?
2024-06-14 07:45 Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid. Anton A. Melnikov <[email protected]>
2024-06-18 06:57 ` Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Anton A. Melnikov <[email protected]>
2024-06-18 14:29 ` Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Maxim Orlov <[email protected]>
2024-06-18 15:47 ` Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Peter Geoghegan <[email protected]>
@ 2024-06-19 17:00 ` Yura Sokolov <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Yura Sokolov @ 2024-06-19 17:00 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; +Cc: Anton A. Melnikov <[email protected]>; PostgreSQL Hackers <[email protected]>
18.06.2024 18:47, Peter Geoghegan пишет:
> On Tue, Jun 18, 2024 at 10:29 AM Maxim Orlov <[email protected]> wrote:
>> Maybe, I'm too bold, but looks like a kinda bug to me. At least, I don't understand why we do not check the HEAP_XMAX_INVALID flag.
>> My guess is nobody noticed, that MultiXactIdIsValid call does not check the mentioned flag in the "first" condition, but it's all my speculation.
>
> A related code path was changed in commit 02d647bbf0. That change made
> the similar xmax handling that covers XIDs (not MXIDs) *stop* doing
> what you're now proposing to do in the Multi path.
I don't agree commit 02d647bbf0 is similar to suggested change.
Commit 02d647bbf0 fixed decision to set
freeze_xmax = false;
xmax_already_frozen = true;
Suggested change is for decision to set
*flags |= FRM_INVALIDATE_XMAX;
pagefrz->freeze_required = true;
Which leads to
freeze_xmax = true;
So it is quite different code paths, and one could not be used
to decline or justify other.
More over, certainly test on HEAP_XMAX_INVALID could be used
there in heap_prepare_freeze_tuple to set
freeze_xmax = true;
Why didn't you do it?
>
> Why do you think this is a bug?
It is not a bug per se.
But:
- it is missed opportunity for optimization,
- it is inconsistency in data handling.
Inconsistency leads to bugs when people attempt to modify code.
Yes, we changed completely different place mistakenly relying on
consistent reaction on this "hint", and that leads to bug in our
patch.
>> Does anyone know if there are reasons to deliberately ignore the HEAP_XMAX INVALID flag? Or this is just an unfortunate oversight.
>
> HEAP_XMAX_INVALID is just a hint.
>
WTF is "just a hint"?
I thought, hint is "yep, you can ignore it. But we already did some job
and stored its result as this hint. And if you don't ignore this hint,
then you can skip doing the job we did already".
So every time we ignore hint, we miss opportunity for optimization.
Why the hell we shouldn't optimize when we safely can?
If we couldn't rely on hint, then hint is completely meaningless.
--------
have a nice day
Yura Sokolov
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid.
2024-06-14 07:45 Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid. Anton A. Melnikov <[email protected]>
@ 2024-06-19 17:21 ` Alvaro Herrera <[email protected]>
1 sibling, 0 replies; 7+ messages in thread
From: Alvaro Herrera @ 2024-06-19 17:21 UTC (permalink / raw)
To: Anton A. Melnikov <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 2024-Jun-14, Anton A. Melnikov wrote:
> Hello!
>
> The src/backend/access/heap/README.tuplock says about HEAP_XMAX_INVALID bit
> that "Any tuple with this bit set does not have a valid value stored in XMAX."
>
> Found that FreezeMultiXactId() tries to process such an invalid multi xmax
> and may looks for an update xid in the pg_multixact for it.
>
> Maybe not do this work in FreezeMultiXactId() and exit immediately if the
> bit HEAP_XMAX_INVALID was already set?
>
> For instance, like that:
>
> master
> @@ -6215,6 +6215,15 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
> /* We should only be called in Multis */
> Assert(t_infomask & HEAP_XMAX_IS_MULTI);
> + /* Xmax is already marked as invalid */
> + if (MultiXactIdIsValid(multi) &&
> + (t_infomask & HEAP_XMAX_INVALID))
Hmm, but why are we calling FreezeMultiXactId at all if the
HEAP_XMAX_INVALID bit is set? We shouldn't do that. I think the fix
should appear in heap_prepare_freeze_tuple() to skip work completely if
HEAP_XMAX_INVALID is set. Then in FreezeMultiXactId you could simply
Assert() that the given tuple does not have HEAP_XMAX_INVALID set.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"It takes less than 2 seconds to get to 78% complete; that's a good sign.
A few seconds later it's at 90%, but it seems to have stuck there. Did
somebody make percentages logarithmic while I wasn't looking?"
http://smylers.hates-software.com/2005/09/08/1995c749.html
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-06-19 17:21 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-31 19:40 [PATCH v24 09/11] pg_ls_*/pg_stat_file to show file *type*.. Justin Pryzby <[email protected]>
2024-06-14 07:45 Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid. Anton A. Melnikov <[email protected]>
2024-06-18 06:57 ` Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Anton A. Melnikov <[email protected]>
2024-06-18 14:29 ` Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Maxim Orlov <[email protected]>
2024-06-18 15:47 ` Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Peter Geoghegan <[email protected]>
2024-06-19 17:00 ` Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid? Yura Sokolov <[email protected]>
2024-06-19 17:21 ` Re: Don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid. Alvaro Herrera <[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