public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v16 09/10] pg_ls_*dir to return all the metadata from pg_stat_file.. 4+ messages / 4 participants [nested] [flat]
* [PATCH v16 09/10] pg_ls_*dir to return all the metadata from pg_stat_file.. @ 2020-03-10 02:56 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2020-03-10 02:56 UTC (permalink / raw) ..but it doesn't seem worth factoring out the common bits, since stat_file doesn't return a name, so all the field numbers are off by one. NOTE, the atime is now shown where the mtime used to be. Need catversion bump --- doc/src/sgml/func.sgml | 30 ++++----- src/backend/utils/adt/genfile.c | 65 +++++++++----------- src/include/catalog/pg_proc.dat | 34 +++++----- src/test/regress/expected/misc_functions.out | 16 ++--- src/test/regress/output/tablespace.source | 8 +-- 5 files changed, 71 insertions(+), 82 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 3a2ad3eb50..1177799916 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25329,8 +25329,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); </entry> <entry><type>setof record</type></entry> <entry> - For each file in the log directory, list the file's name, size, last - modification time, and a boolean indicating if it is a directory. + For each file in the log directory, list the file and its metadata. Access is granted to members of the <literal>pg_monitor</literal> role and may be granted to other non-superuser roles. </entry> @@ -25341,8 +25340,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); </entry> <entry><type>setof record</type></entry> <entry> - For each file in the WAL directory, list the file's name, size, last - modification time, and a boolean indicating if it is a directory. + For each file in the WAL directory, list the file and its metadata. Access is granted to members of the <literal>pg_monitor</literal> role and may be granted to other non-superuser roles. </entry> @@ -25353,9 +25351,8 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); </entry> <entry><type>setof record</type></entry> <entry> - For each file in the WAL archive status directory, list the file's - name, size, last modification time, and a boolean indicating if it is a - directory. Access is granted to members of the + For each file in the WAL archive status directory, list the file and its metadata. + Access is granted to members of the <literal>pg_monitor</literal> role and may be granted to other non-superuser roles. </entry> @@ -25367,8 +25364,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); <entry><type>setof record</type></entry> <entry> For the temporary directory within <parameter>tablespace</parameter>, - list each file's name, size, last modification time, and a boolean - indicating if it is a directory. If <parameter>tablespace</parameter> + list each file and its metadata. If <parameter>tablespace</parameter> is not provided, the <literal>pg_default</literal> tablespace is used. Access is granted to members of the <literal>pg_monitor</literal> role and may be granted to other non-superuser roles. @@ -25454,8 +25450,8 @@ LATERAL pg_ls_dir_recurse(dir) AS a; </indexterm> <para> <function>pg_ls_logdir</function> lists each file in the log directory, - along with file's size, last modification time, and a boolean - indicating if the file is a directory. By default, only superusers + along with the metadata columns returned by <function>pg_stat_file</function>. + By default, only superusers and members of the <literal>pg_monitor</literal> role can use this function. Access may be granted to others using <command>GRANT</command>. Filenames beginning with a dot and special file types are not shown. @@ -25466,8 +25462,8 @@ LATERAL pg_ls_dir_recurse(dir) AS a; </indexterm> <para> <function>pg_ls_waldir</function> lists each file in the WAL directory, - along with the file's size, last modification time, and a boolean - indicating if the file is a directory. By default, only superusers + along with the metadata columns returned by <function>pg_stat_file</function>. + By default, only superusers and members of the <literal>pg_monitor</literal> role can use this function. Access may be granted to others using <command>GRANT</command>. @@ -25479,8 +25475,8 @@ LATERAL pg_ls_dir_recurse(dir) AS a; </indexterm> <para> <function>pg_ls_archive_statusdir</function> lists each file in the WAL - archive status directory, along with the file's size, last modification - time, and a boolean indicating if the file is a directory. By default, only + archive status directory, along with the metadata columns returned by + <function>pg_stat_file</function>. By default, only superusers and members of the <literal>pg_monitor</literal> role can use this function. Access may be granted to others using <command>GRANT</command>. @@ -25493,8 +25489,8 @@ LATERAL pg_ls_dir_recurse(dir) AS a; <para> <function>pg_ls_tmpdir</function> lists each file in the temporary file directory for the specified <parameter>tablespace</parameter>, along with - its size, last modified time (mtime) and a boolean indicating if the file is a - directory. Directories are used for temporary files shared by parallel + the metadata columns returned by <function>pg_stat_file</function>. + Directories are used for temporary files shared by parallel processes. If <parameter>tablespace</parameter> is not provided, the <literal>pg_default</literal> tablespace is used. By default only superusers and members of the <literal>pg_monitor</literal> diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index 91bf8c69e9..10780d3fb1 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -36,6 +36,8 @@ #include "utils/syscache.h" #include "utils/timestamp.h" +static void tuple_from_stat(struct stat *fst, const char *path, Datum *values, + bool *isnull); static Datum pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags); #define LS_DIR_ISDIR (1<<0) /* Show column: isdir */ @@ -367,6 +369,28 @@ pg_read_binary_file_all(PG_FUNCTION_ARGS) return pg_read_binary_file(fcinfo); } +/* + * Populate values and isnull from fst and path. + * Used for pg_stat_file() and pg_stat_dir_files() + * isnull is assumed to have been zeroed. + */ +static void +tuple_from_stat(struct stat *fst, const char *path, Datum *values, bool *isnull) +{ + values[0] = Int64GetDatum((int64) fst->st_size); + values[1] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_atime)); + values[2] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_mtime)); + /* Unix has file status change time, while Win32 has creation time */ +#if !defined(WIN32) && !defined(__CYGWIN__) + values[3] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_ctime)); + isnull[4] = true; +#else + isnull[3] = true; + values[4] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_ctime)); +#endif + values[5] = BoolGetDatum(S_ISDIR(fst->st_mode)); +} + /* * stat a file */ @@ -418,25 +442,7 @@ pg_stat_file(PG_FUNCTION_ARGS) BlessTupleDesc(tupdesc); memset(isnull, false, sizeof(isnull)); - - values[0] = Int64GetDatum((int64) fst.st_size); - values[1] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_atime)); - values[2] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_mtime)); - /* Unix has file status change time, while Win32 has creation time */ -#if !defined(WIN32) && !defined(__CYGWIN__) - values[3] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_ctime)); - isnull[4] = true; -#else - isnull[3] = true; - values[4] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_ctime)); -#endif - values[5] = BoolGetDatum(S_ISDIR(fst.st_mode)); -#ifdef WIN32 - /* Links should have isdir=false */ - if (pgwin32_is_junction(filename)) - values[5] = BoolGetDatum(false); -#endif - + tuple_from_stat(&fst, filename, values, isnull); tuple = heap_form_tuple(tupdesc, values, isnull); pfree(filename); @@ -578,8 +584,8 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags) while ((de = ReadDir(dirdesc, dir)) != NULL) { - Datum values[4]; - bool nulls[4]; + Datum values[7]; + bool nulls[7]; char path[MAXPGPATH * 2]; struct stat attrib; @@ -618,23 +624,10 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags) continue; } + memset(nulls, 0, sizeof(nulls)); values[0] = CStringGetTextDatum(de->d_name); if (flags & LS_DIR_METADATA) - { - values[1] = Int64GetDatum((int64) attrib.st_size); - values[2] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_mtime)); - if (flags & LS_DIR_ISDIR) - { - values[3] = BoolGetDatum(S_ISDIR(attrib.st_mode)); -#ifdef WIN32 - /* Links should have isdir=false */ - if (pgwin32_is_junction(path)) - values[3] = BoolGetDatum(false); -#endif - } - } - - memset(nulls, 0, sizeof(nulls)); + tuple_from_stat(&attrib, path, 1+values, 1+nulls); tuplestore_putvalues(tupstore, tupdesc, values, nulls); } diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index f94f403475..34ee5a8f97 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -10880,48 +10880,48 @@ { 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,bool}', proargmodes => '{o,o,o,o}', - proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_logdir' }, + 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' }, { 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,bool}', proargmodes => '{o,o,o,o}', - proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_waldir' }, + 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' }, { 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,bool}', - proargmodes => '{o,o,o,o}', proargnames => '{name,size,modification,isdir}', + 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_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,bool}', proargmodes => '{o,o,o,o}', - proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_tmpdir_noargs' }, + 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' }, { 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,bool}', proargmodes => '{i,o,o,o,o}', - proargnames => '{tablespace,name,size,modification,isdir}', + 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}', prosrc => 'pg_ls_tmpdir_1arg' }, { oid => '5032', 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,bool}', proargmodes => '{i,i,i,o,o,o,o}', - proargnames => '{dirname,missing_ok,include_dot_dirs,name,size,modification,isdir}', + 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,name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_dir_metadata' }, { oid => '5033', 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,bool}', proargmodes => '{i,o,o,o,o}', - proargnames => '{dirname,name,size,modification,isdir}', + proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}', + proargnames => '{dirname,name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_dir_metadata_1arg' }, { oid => '5034', descr => 'list all files in a directory recursively', proname => 'pg_ls_dir_recurse', prorows => '10000', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => 'text', - proallargtypes => '{text,text,int8,timestamptz,bool}', - proargnames => '{dirname,name,size,modification,isdir}', proargmodes => '{i,o,o,o,o}', - prolang => 'sql', prosrc => "with recursive ls as (select * from pg_ls_dir_metadata(dirname, true, false) union all select ls.name||'/'||a.name, a.size, a.modification, a.isdir from ls, lateral pg_ls_dir_metadata(dirname||'/'||ls.name, false, false)a where ls.isdir) select * from ls" }, + proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', + proargnames => '{dirname,name,size,access,modification,change,creation,isdir}', proargmodes => '{i,o,o,o,o,o,o,o}', + prolang => 'sql', prosrc => "with recursive ls as (select * from pg_ls_dir_metadata(dirname, true, false) union all select ls.name||'/'||a.name, a.size, a.access, a.modification, a.change, a.creation, a.isdir from ls, lateral pg_ls_dir_metadata(dirname||'/'||ls.name, false, false)a where ls.isdir) select * from ls" }, # hash partitioning constraint function { oid => '5028', descr => 'hash partition CHECK constraint', diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index 4188d684f0..cce84a60a9 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 | modification | isdir -------+------+--------------+------- + name | size | access | modification | change | creation | isdir +------+------+--------+--------------+--------+----------+------- (0 rows) select count(*) > 0 as ok from (select * from pg_ls_waldir() limit 1) ss; @@ -222,8 +222,8 @@ ERROR: could not open directory "does not exist": No such file or directory -- 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 | modification | isdir -------+------+--------------+------- + name | size | access | modification | change | creation | isdir +------+------+--------+--------------+--------+----------+------- (0 rows) select name, isdir from pg_ls_dir_metadata('.') where name='.'; @@ -239,8 +239,8 @@ select name, isdir from pg_ls_dir_metadata('.', false, false) where name='.'; -- -- Check that expected columns are present select * from pg_ls_dir_metadata('.') limit 0; - name | size | modification | isdir -------+------+--------------+------- + name | size | access | modification | change | creation | isdir +------+------+--------+--------------+--------+----------+------- (0 rows) -- Check that we at least succeed in recursing once, and that we don't show the leading dir prefix @@ -253,8 +253,8 @@ SELECT name, isdir FROM pg_ls_dir_recurse('.') WHERE isdir AND name~'^pg_wal'; -- Check that expected columns are present SELECT * FROM pg_ls_dir_recurse('.') LIMIT 0; - name | size | modification | isdir -------+------+--------------+------- + name | size | access | modification | change | creation | isdir +------+------+--------+--------------+--------+----------+------- (0 rows) -- diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source index ba9a3fe29a..1e1e02b589 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 | modification | isdir -------+------+--------------+------- + name | size | access | modification | change | creation | isdir +------+------+--------+--------------+--------+----------+------- (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 | modification | isdir -------+------+--------------+------- + name | size | access | modification | change | creation | isdir +------+------+--------+--------------+--------+----------+------- (0 rows) -- try setting and resetting some properties for the new tablespace -- 2.17.0 --2FkSFaIQeDFoAt0B Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v16-0010-pg_ls_-to-show-file-type-and-show-special-files.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible @ 2024-02-12 23:36 Andres Freund <[email protected]> 2024-02-13 01:33 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]> 2024-02-13 17:17 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Bharath Rupireddy <[email protected]> 0 siblings, 2 replies; 4+ messages in thread From: Andres Freund @ 2024-02-12 23:36 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Alvaro Herrera <[email protected]>; Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; Nathan Bossart <[email protected]>; Masahiko Sawada <[email protected]> Hi, On 2024-02-12 12:46:00 -0800, Jeff Davis wrote: > On Mon, 2024-02-12 at 12:18 -0800, Andres Freund wrote: > > + upto = Min(startptr + count, LogwrtResult.Write); > > + nbytes = upto - startptr; > > > > Shouldn't it pretty much be a bug to ever encounter this? > > In the current code it's impossible, though Bharath hinted at an > extension which could reach that path. > > What I committed was a bit of a compromise -- earlier versions of the > patch supported reading right up to the Insert pointer (which requires > a call to WaitXLogInsertionsToFinish()). I wasn't ready to commit that > code without seeing a more about how that would be used, but I thought > it was reasonable to have some simple code in there to allow reading up > to the Write pointer. I doubt there's a sane way to use WALRead() without *first* ensuring that the range of data is valid. I think we're better of moving that responsibility explicitly to the caller and adding an assertion verifying that. > It seems closer to the structure that we will ultimately need to > replicate unflushed data, right? It doesn't really seem like a necessary, or even particularly useful, part. You couldn't just call WALRead() for that, since the caller would need to know the range up to which WAL is valid but not yet flushed as well. Thus the caller would need to first use WaitXLogInsertionsToFinish() or something like it anyway - and then there's no point in doing the WALRead() anymore. Note that for replicating unflushed data, we *still* might need to fall back to reading WAL data from disk. In which case not asserting in WALRead() would just make it hard to find bugs, because not using WaitXLogInsertionsToFinish() would appear to work as long as data is in wal buffers, but as soon as we'd fall back to on-disk (but unflushed) data, we'd send bogus WAL. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible 2024-02-12 23:36 Re: Improve WALRead() to suck data directly from WAL buffers when possible Andres Freund <[email protected]> @ 2024-02-13 01:33 ` Jeff Davis <[email protected]> 1 sibling, 0 replies; 4+ messages in thread From: Jeff Davis @ 2024-02-13 01:33 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Alvaro Herrera <[email protected]>; Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; Nathan Bossart <[email protected]>; Masahiko Sawada <[email protected]> On Mon, 2024-02-12 at 15:36 -0800, Andres Freund wrote: > > It doesn't really seem like a necessary, or even particularly useful, > part. You couldn't just call WALRead() for that, since the caller > would need > to know the range up to which WAL is valid but not yet flushed as > well. Thus > the caller would need to first use WaitXLogInsertionsToFinish() or > something > like it anyway - and then there's no point in doing the WALRead() > anymore. I follow until the last part. Did you mean "and then there's no point in doing the WaitXLogInsertionsToFinish() in WALReadFromBuffers() anymore"? For now, should I assert that the requested WAL data is before the Flush pointer or assert that it's before the Write pointer? > Note that for replicating unflushed data, we *still* might need to > fall back > to reading WAL data from disk. In which case not asserting in > WALRead() would > just make it hard to find bugs, because not using > WaitXLogInsertionsToFinish() > would appear to work as long as data is in wal buffers, but as soon > as we'd > fall back to on-disk (but unflushed) data, we'd send bogus WAL. That makes me wonder whether my previous idea[1] might matter: when some buffers have been evicted, should WALReadFromBuffers() keep going through the loop and return the end portion of the requested data rather than the beginning? We can sort that out when we get closer to replicating unflushed WAL. Regards, Jeff Davis [1] https://www.postgresql.org/message-id/[email protected] ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible 2024-02-12 23:36 Re: Improve WALRead() to suck data directly from WAL buffers when possible Andres Freund <[email protected]> @ 2024-02-13 17:17 ` Bharath Rupireddy <[email protected]> 1 sibling, 0 replies; 4+ messages in thread From: Bharath Rupireddy @ 2024-02-13 17:17 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Jeff Davis <[email protected]>; Alvaro Herrera <[email protected]>; Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; Nathan Bossart <[email protected]>; Masahiko Sawada <[email protected]> On Tue, Feb 13, 2024 at 5:06 AM Andres Freund <[email protected]> wrote: > > I doubt there's a sane way to use WALRead() without *first* ensuring that the > range of data is valid. I think we're better of moving that responsibility > explicitly to the caller and adding an assertion verifying that. > > It doesn't really seem like a necessary, or even particularly useful, > part. You couldn't just call WALRead() for that, since the caller would need > to know the range up to which WAL is valid but not yet flushed as well. Thus > the caller would need to first use WaitXLogInsertionsToFinish() or something > like it anyway - and then there's no point in doing the WALRead() anymore. > > Note that for replicating unflushed data, we *still* might need to fall back > to reading WAL data from disk. In which case not asserting in WALRead() would > just make it hard to find bugs, because not using WaitXLogInsertionsToFinish() > would appear to work as long as data is in wal buffers, but as soon as we'd > fall back to on-disk (but unflushed) data, we'd send bogus WAL. Callers of WALRead() do a good amount of work to figure out what's been flushed out but they read the un-flushed and/or invalid data see the comment [1] around WALRead() call sites as well as a recent thread [2] for more details. IIUC, here's the summary of the discussion that has happened so far: a) If only replicating flushed data, then ensure all the WALRead() callers read how much ever is valid out of startptr+count. Fix provided in [2] can help do that. b) If only replicating flushed data, then ensure all the WALReadFromBuffers() callers read how much ever is valid out of startptr+count. Current and expected WALReadFromBuffers() callers will anyway determine how much of it is flushed and can validly be read. c) If planning to replicate unflushed data, then ensure all the WALRead() callers wait until startptr+count is past the current insert position with WaitXLogInsertionsToFinish(). d) If planning to replicate unflushed data, then ensure all the WALReadFromBuffers() callers wait until startptr+count is past the current insert position with WaitXLogInsertionsToFinish(). Adding an assertion or error in WALReadFromBuffers() for ensuring the callers do follow the above set of rules is easy. We can just do Assert(startptr+count <= LogwrtResult.Flush). However, adding a similar assertion or error in WALRead() gets trickier as it's being called from many places - walsenders, backends, external tools etc. even when the server is in recovery. Therefore, determining the actual valid LSN is a bit of a challenge. What I think is the best way: - Try and get the fix provided for (a) at [2]. - Implement both (c) and (d). - Have the assertion in WALReadFromBuffers() ensuring the callers wait until startptr+count is past the current insert position with WaitXLogInsertionsToFinish(). - Have a comment around WALRead() to ensure the callers are requesting the WAL that's written to the disk because it's hard to determine what's written to disk as this gets called in many scenarios - when server is in recovery, for walsummarizer etc. - In the new test module, demonstrate how one can implement reading unflushed data with WALReadFromBuffers() and/or WALRead() + WaitXLogInsertionsToFinish(). Thoughts? [1] /* * Even though we just determined how much of the page can be validly read * as 'count', read the whole page anyway. It's guaranteed to be * zero-padded up to the page boundary if it's incomplete. */ if (!WALRead(state, cur_page, targetPagePtr, XLOG_BLCKSZ, tli, &errinfo)) [2] https://www.postgresql.org/message-id/CALj2ACWBRFac2TingD3PE3w2EBHXUHY3%3DAEEZPJmqhpEOBGExg%40mail.g... -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-02-13 17:17 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-10 02:56 [PATCH v16 09/10] pg_ls_*dir to return all the metadata from pg_stat_file.. Justin Pryzby <[email protected]> 2024-02-12 23:36 Re: Improve WALRead() to suck data directly from WAL buffers when possible Andres Freund <[email protected]> 2024-02-13 01:33 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]> 2024-02-13 17:17 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Bharath Rupireddy <[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