public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v37 03/11] Add pg_ls_dir_metadata to list a dir with file metadata.. 5+ messages / 4 participants [nested] [flat]
* [PATCH v37 03/11] Add pg_ls_dir_metadata to list a dir with file metadata.. @ 2020-03-10 03:40 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Justin Pryzby @ 2020-03-10 03:40 UTC (permalink / raw) Generalize pg_ls_dir_files and retire pg_ls_dir Need catversion bumped? --- doc/src/sgml/func.sgml | 21 ++ src/backend/catalog/system_functions.sql | 1 + src/backend/utils/adt/genfile.c | 205 ++++++++++++------- src/include/catalog/pg_proc.dat | 12 ++ src/test/regress/expected/misc_functions.out | 24 +++ src/test/regress/expected/tablespace.out | 8 + src/test/regress/sql/misc_functions.sql | 11 + src/test/regress/sql/tablespace.sql | 5 + 8 files changed, 217 insertions(+), 70 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index d958c3e74ac..a80d794a43a 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25900,6 +25900,27 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560 </para></entry> </row> + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>pg_ls_dir_metadata</primary> + </indexterm> + <function>pg_ls_dir_metadata</function> ( <parameter>dirname</parameter> <type>text</type> + <optional>, <parameter>missing_ok</parameter> <type>boolean</type>, + <parameter>include_dot_dirs</parameter> <type>boolean</type> </optional> ) + <returnvalue>setof record</returnvalue> + ( <parameter>filename</parameter> <type>text</type>, + <parameter>size</parameter> <type>bigint</type>, + <parameter>modification</parameter> <type>timestamp with time zone</type> ) + </para> + <para> + For each file in the specified directory, list the file and its + metadata. + Restricted to superusers by default, but other users can be granted + EXECUTE to run the function. + </para></entry> + </row> + <row> <entry role="func_table_entry"><para role="func_signature"> <indexterm> diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql index 30a048f6b09..b77b83017cb 100644 --- a/src/backend/catalog/system_functions.sql +++ b/src/backend/catalog/system_functions.sql @@ -704,6 +704,7 @@ REVOKE EXECUTE ON FUNCTION pg_stat_file(text,boolean) FROM public; REVOKE EXECUTE ON FUNCTION pg_ls_dir(text) FROM public; REVOKE EXECUTE ON FUNCTION pg_ls_dir(text,boolean,boolean) FROM public; +REVOKE EXECUTE ON FUNCTION pg_ls_dir_metadata(text,boolean,boolean) FROM public; REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer) FROM PUBLIC; diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index ab6f67f8747..c041c7630c8 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -37,6 +37,21 @@ #include "utils/syscache.h" #include "utils/timestamp.h" +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_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 .. */ +#define LS_DIR_SKIP_HIDDEN (1<<4) /* Do not show anything beginning with . */ +#define LS_DIR_SKIP_DIRS (1<<5) /* Do not show directories */ +#define LS_DIR_SKIP_SPECIAL (1<<6) /* Do not show special file types */ + +/* + * Shortcut for the historic behavior of the pg_ls_* functions (not including + * pg_ls_dir, which skips different files and doesn't show metadata). + */ +#define LS_DIR_HISTORIC (LS_DIR_SKIP_DIRS | LS_DIR_SKIP_HIDDEN | LS_DIR_SKIP_SPECIAL | LS_DIR_METADATA) /* * Convert a "text" filename argument to C string, and check it's allowable. @@ -516,6 +531,11 @@ pg_stat_file(PG_FUNCTION_ARGS) 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 = heap_form_tuple(tupdesc, values, isnull); @@ -543,54 +563,9 @@ pg_stat_file_1arg(PG_FUNCTION_ARGS) Datum pg_ls_dir(PG_FUNCTION_ARGS) { - ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; - char *location; - bool missing_ok = false; - bool include_dot_dirs = false; - DIR *dirdesc; - struct dirent *de; - - location = convert_and_check_filename(PG_GETARG_TEXT_PP(0)); - - /* check the optional arguments */ - if (PG_NARGS() == 3) - { - if (!PG_ARGISNULL(1)) - missing_ok = PG_GETARG_BOOL(1); - if (!PG_ARGISNULL(2)) - include_dot_dirs = PG_GETARG_BOOL(2); - } - - InitMaterializedSRF(fcinfo, MAT_SRF_USE_EXPECTED_DESC); - - dirdesc = AllocateDir(location); - if (!dirdesc) - { - /* Return empty tuplestore if appropriate */ - if (missing_ok && errno == ENOENT) - return (Datum) 0; - /* Otherwise, we can let ReadDir() throw the error */ - } - - while ((de = ReadDir(dirdesc, location)) != NULL) - { - Datum values[1]; - bool nulls[1]; - - if (!include_dot_dirs && - (strcmp(de->d_name, ".") == 0 || - strcmp(de->d_name, "..") == 0)) - continue; - - values[0] = CStringGetTextDatum(de->d_name); - nulls[0] = false; - - tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, - values, nulls); - } - - FreeDir(dirdesc); - return (Datum) 0; + text *filename_t = PG_GETARG_TEXT_PP(0); + char *filename = convert_and_check_filename(filename_t); + return pg_ls_dir_files(fcinfo, filename, LS_DIR_SKIP_DOT_DIRS); } /* @@ -603,23 +578,55 @@ pg_ls_dir(PG_FUNCTION_ARGS) Datum pg_ls_dir_1arg(PG_FUNCTION_ARGS) { - return pg_ls_dir(fcinfo); + text *filename_t = PG_GETARG_TEXT_PP(0); + char *filename = convert_and_check_filename(filename_t); + return pg_ls_dir_files(fcinfo, filename, LS_DIR_SKIP_DOT_DIRS); } /* - * Generic function to return a directory listing of files. + * Generic function to return a directory listing of files (and optionally dirs). * - * If the directory isn't there, silently return an empty set if missing_ok. + * If the directory isn't there, silently return an empty set if MISSING_OK. * Other unreadable-directory cases throw an error. */ static Datum -pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok) +pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags) { ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; DIR *dirdesc; struct dirent *de; - InitMaterializedSRF(fcinfo, 0); + /* 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)); + + /* check the optional arguments */ + if (PG_NARGS() == 3) + { + /* missing_ok */ + if (!PG_ARGISNULL(1)) + { + if (PG_GETARG_BOOL(1)) + flags |= LS_DIR_MISSING_OK; + else + flags &= ~LS_DIR_MISSING_OK; + } + + /* include_dot_dirs */ + if (!PG_ARGISNULL(2)) + { + if (PG_GETARG_BOOL(2)) + flags &= ~LS_DIR_SKIP_DOT_DIRS; + else + flags |= LS_DIR_SKIP_DOT_DIRS; + } + } + + if (flags & LS_DIR_METADATA) + InitMaterializedSRF(fcinfo, 0); + else + InitMaterializedSRF(fcinfo, MAT_SRF_USE_EXPECTED_DESC); /* * Now walk the directory. Note that we must do this within a single SRF @@ -630,20 +637,27 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok) if (!dirdesc) { /* Return empty tuplestore if appropriate */ - if (missing_ok && errno == ENOENT) + if (flags & LS_DIR_MISSING_OK && errno == ENOENT) return (Datum) 0; /* Otherwise, we can let ReadDir() throw the error */ } while ((de = ReadDir(dirdesc, dir)) != NULL) { - Datum values[3]; - bool nulls[3]; + Datum values[4]; + bool nulls[4]; char path[MAXPGPATH * 2]; struct stat attrib; - /* Skip hidden files */ - if (de->d_name[0] == '.') + /* Skip dot dirs? */ + if (flags & LS_DIR_SKIP_DOT_DIRS && + (strcmp(de->d_name, ".") == 0 || + strcmp(de->d_name, "..") == 0)) + continue; + + /* Skip hidden files? */ + if (flags & LS_DIR_SKIP_HIDDEN && + de->d_name[0] == '.') continue; /* Get the file info */ @@ -658,13 +672,35 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok) errmsg("could not stat file \"%s\": %m", path))); } - /* Ignore anything but regular files */ - if (!S_ISREG(attrib.st_mode)) - continue; + /* Skip dirs or special files? */ + if (S_ISDIR(attrib.st_mode)) + { + if (flags & LS_DIR_SKIP_DIRS) + continue; + } + else if (!S_ISREG(attrib.st_mode)) + { + if (flags & LS_DIR_SKIP_SPECIAL) + continue; + } values[0] = CStringGetTextDatum(de->d_name); - values[1] = Int64GetDatum((int64) attrib.st_size); - values[2] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_mtime)); + 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) + { +#ifdef WIN32 + /* Links should have isdir=false */ + if (pgwin32_is_junction(path)) + values[3] = BoolGetDatum(false); + else +#endif + values[3] = BoolGetDatum(S_ISDIR(attrib.st_mode)); + } + } + memset(nulls, 0, sizeof(nulls)); tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls); @@ -678,14 +714,14 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok) Datum pg_ls_logdir(PG_FUNCTION_ARGS) { - return pg_ls_dir_files(fcinfo, Log_directory, false); + return pg_ls_dir_files(fcinfo, Log_directory, LS_DIR_HISTORIC); } /* Function to return the list of files in the WAL directory */ Datum pg_ls_waldir(PG_FUNCTION_ARGS) { - return pg_ls_dir_files(fcinfo, XLOGDIR, false); + return pg_ls_dir_files(fcinfo, XLOGDIR, LS_DIR_HISTORIC); } /* @@ -703,7 +739,8 @@ pg_ls_tmpdir(FunctionCallInfo fcinfo, Oid tblspc) tblspc))); TempTablespacePath(path, tblspc); - return pg_ls_dir_files(fcinfo, path, true); + return pg_ls_dir_files(fcinfo, path, + LS_DIR_HISTORIC | LS_DIR_MISSING_OK); } /* @@ -732,7 +769,35 @@ pg_ls_tmpdir_1arg(PG_FUNCTION_ARGS) Datum pg_ls_archive_statusdir(PG_FUNCTION_ARGS) { - return pg_ls_dir_files(fcinfo, XLOGDIR "/archive_status", true); + return pg_ls_dir_files(fcinfo, XLOGDIR "/archive_status", + LS_DIR_HISTORIC | LS_DIR_MISSING_OK); +} + +/* + * Return the list of files and metadata in an arbitrary directory. + */ +Datum +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); +} + +/* + * Return the list of files and metadata in an arbitrary directory. + * note: this wrapper is necessary to pass the sanity check in opr_sanity, + * which checks that all built-in functions that share the implementing C + * function take the same number of arguments. + */ +Datum +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); } /* @@ -741,7 +806,7 @@ pg_ls_archive_statusdir(PG_FUNCTION_ARGS) Datum pg_ls_logicalsnapdir(PG_FUNCTION_ARGS) { - return pg_ls_dir_files(fcinfo, "pg_logical/snapshots", false); + return pg_ls_dir_files(fcinfo, "pg_logical/snapshots", LS_DIR_HISTORIC); } /* @@ -750,7 +815,7 @@ pg_ls_logicalsnapdir(PG_FUNCTION_ARGS) Datum pg_ls_logicalmapdir(PG_FUNCTION_ARGS) { - return pg_ls_dir_files(fcinfo, "pg_logical/mappings", false); + return pg_ls_dir_files(fcinfo, "pg_logical/mappings", LS_DIR_HISTORIC); } /* @@ -775,5 +840,5 @@ pg_ls_replslotdir(PG_FUNCTION_ARGS) slotname))); snprintf(path, sizeof(path), "pg_replslot/%s", slotname); - return pg_ls_dir_files(fcinfo, path, false); + return pg_ls_dir_files(fcinfo, path, LS_DIR_HISTORIC); } diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 20f5aa56eab..89892647808 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -11758,6 +11758,18 @@ proargmodes => '{i,o,o,o}', proargnames => '{slot_name,name,size,modification}', prosrc => 'pg_ls_replslotdir' }, +{ 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,bool}', proargmodes => '{i,i,i,o,o,o,o}', + proargnames => '{dirname,missing_ok,include_dot_dirs,filename,size,modification,isdir}', + 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,bool}', proargmodes => '{i,o,o,o,o}', + proargnames => '{dirname,filename,size,modification,isdir}', + prosrc => 'pg_ls_dir_metadata_1arg' }, # 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 77d285ecc85..a656f9ad07e 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -539,6 +539,30 @@ select * from pg_stat_file('.') limit 0; ------+--------+--------------+--------+----------+------- (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 | modification +------+------+-------------- +(0 rows) + +select filename, isdir from pg_ls_dir_metadata('.') where filename='.'; + filename | isdir +----------+------- + . | t +(1 row) + +select filename, isdir from pg_ls_dir_metadata('.', false, false) where filename='.'; -- include_dot_dirs=false + filename | isdir +----------+------- +(0 rows) + +-- Check that expected columns are present +select * from pg_ls_dir_metadata('.') limit 0; + filename | size | modification | isdir +----------+------+--------------+------- +(0 rows) + -- -- Test replication slot directory functions -- diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/expected/tablespace.out index c52cf1cfcf9..8159c9f18f1 100644 --- a/src/test/regress/expected/tablespace.out +++ b/src/test/regress/expected/tablespace.out @@ -33,6 +33,14 @@ SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\1/NNN' pg_tblspc/NNN (1 row) +-- 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. +-- 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 +------+------+-------------- +(0 rows) + -- try setting and resetting some properties for the new tablespace ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1); ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- fail diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index d299f3d8949..f808555382d 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -181,6 +181,17 @@ select * from pg_ls_tmpdir() limit 0; select * from pg_ls_waldir() limit 0; select * from pg_stat_file('.') limit 0; +-- 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'; + +select filename, isdir from pg_ls_dir_metadata('.') where filename='.'; + +select filename, isdir 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; + -- -- Test replication slot directory functions -- diff --git a/src/test/regress/sql/tablespace.sql b/src/test/regress/sql/tablespace.sql index 21db433f2a8..cf683c3bf3a 100644 --- a/src/test/regress/sql/tablespace.sql +++ b/src/test/regress/sql/tablespace.sql @@ -27,6 +27,11 @@ CREATE TABLESPACE regress_tblspace LOCATION ''; SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\1/NNN') FROM pg_tablespace WHERE spcname = 'regress_tblspace'; +-- 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. +-- 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'; + -- try setting and resetting some properties for the new tablespace ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1); ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- fail -- 2.25.1 --Pk/CTwBz1VvfPIDp Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v37-0004-pg_ls_tmpdir-to-show-directories-and-isdir-argum.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? @ 2023-03-07 04:32 Michael Paquier <[email protected]> 2023-03-07 06:14 ` Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Thomas Munro <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Michael Paquier @ 2023-03-07 04:32 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Andres Freund <[email protected]>; Alvaro Herrera <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 07, 2023 at 03:44:46PM +1300, Thomas Munro wrote: > On Tue, Mar 7, 2023 at 3:42 PM Thomas Munro <[email protected]> wrote: >> Apparently ye olde GCC 4.7 on "lapwing" doesn't like the way you >> initialised that struct. I guess it wants {{0}} instead of {0}. >> Apparently old GCC was wrong about that warning[1], but that system >> doesn't have the back-patched fixes? Not sure. 6392f2a was one such case. > Oh, you already pushed a fix. But now I'm wondering if it's useful to > have old buggy compilers set to run with -Werror. Yes, as far as I can see when investigating the issue, this is an old bug of gcc when detecting where the initialization needs to be applied. And at the same time the fix is deadly simple, so the current statu-quo does not sound that bad to me. Note that lapwing is one of the only animals testing 32b builds, and it has saved from quite few bugs over the years. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? 2023-03-07 04:32 Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Michael Paquier <[email protected]> @ 2023-03-07 06:14 ` Thomas Munro <[email protected]> 2023-03-07 06:47 ` Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Julien Rouhaud <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Thomas Munro @ 2023-03-07 06:14 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Andres Freund <[email protected]>; Alvaro Herrera <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 7, 2023 at 5:32 PM Michael Paquier <[email protected]> wrote: > On Tue, Mar 07, 2023 at 03:44:46PM +1300, Thomas Munro wrote: > > Oh, you already pushed a fix. But now I'm wondering if it's useful to > > have old buggy compilers set to run with -Werror. > > Yes, as far as I can see when investigating the issue, this is an old > bug of gcc when detecting where the initialization needs to be > applied. And at the same time the fix is deadly simple, so the > current statu-quo does not sound that bad to me. Note that lapwing is > one of the only animals testing 32b builds, and it has saved from > quite few bugs over the years. Yeah, but I'm just wondering, why not run a current release on it[1]? Debian is one of the few distributions still supporting 32 bit kernels, and it's good to test rare things, but AFAIK the primary reason we finish up with EOL'd OSes in the 'farm is because they have been forgotten (the secondary reason is because they couldn't be upgraded because the OS dropped the [micro]architecture). Unlike vintage SPARC, actual users might plausibly be running a current release on a 32 bit Intel system, I guess (maybe on a Quark microcontroller?)? BTW CI also tests 32 bit with -m32 on Debian, but with a 64 bit kernel, which probably doesn't change much at the level we care about, so maybe this doesn't matter much... just sharing an observation that we're wasting time thinking about an OS release that gave up the ghost in 2016, because it is running with -Werror. *shrug* [1] https://wiki.debian.org/DebianReleases ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? 2023-03-07 04:32 Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Michael Paquier <[email protected]> 2023-03-07 06:14 ` Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Thomas Munro <[email protected]> @ 2023-03-07 06:47 ` Julien Rouhaud <[email protected]> 2023-03-07 07:33 ` Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Thomas Munro <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Julien Rouhaud @ 2023-03-07 06:47 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Michael Paquier <[email protected]>; Bharath Rupireddy <[email protected]>; Andres Freund <[email protected]>; Alvaro Herrera <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 07, 2023 at 07:14:51PM +1300, Thomas Munro wrote: > On Tue, Mar 7, 2023 at 5:32 PM Michael Paquier <[email protected]> wrote: > > On Tue, Mar 07, 2023 at 03:44:46PM +1300, Thomas Munro wrote: > > > Oh, you already pushed a fix. But now I'm wondering if it's useful to > > > have old buggy compilers set to run with -Werror. > > > > Yes, as far as I can see when investigating the issue, this is an old > > bug of gcc when detecting where the initialization needs to be > > applied. And at the same time the fix is deadly simple, so the > > current statu-quo does not sound that bad to me. Note that lapwing is > > one of the only animals testing 32b builds, and it has saved from > > quite few bugs over the years. > > Yeah, but I'm just wondering, why not run a current release on it[1]? > Debian is one of the few distributions still supporting 32 bit > kernels, and it's good to test rare things, but AFAIK the primary > reason we finish up with EOL'd OSes in the 'farm is because they have > been forgotten (the secondary reason is because they couldn't be > upgraded because the OS dropped the [micro]architecture). I registered lapwing as a 32b Debian 7 so I thought it would be expected to keep it as-is rather than upgrading to all newer major Debian versions, especially since there were newer debian animal registered (no 32b though AFAICS). I'm not opposed to upgrading it but I think there's still value in having somewhat old packages versions being tested, especially since there isn't much 32b coverage of those. I would be happy to register a newer 32b version, or even sid, if needed but the -m32 part on the CI makes me think there isn't much value doing that now. Now about the -Werror: > BTW CI also tests 32 bit with -m32 on Debian, but with a 64 bit > kernel, which probably doesn't change much at the level we care about, > so maybe this doesn't matter much... just sharing an observation that > we're wasting time thinking about an OS release that gave up the ghost > in 2016, because it is running with -Werror. *shrug* I think this is the first time that a problem raised by -Werror on that old animal is actually a false positive, while there were many times it reported useful stuff. Now this has been up for years before we got better CI tooling, especially with -m32 support, so there might not be any value to have it anymore. As I mentioned at [1] I don't mind removing it or just work on upgrading any dependency (or removing known buggy compiler flags) to keep it without being annoying. In any case I'm usually quite fast at reacting to any problem/complaint on that animal, so you don't have to worry about the buildfarm being red too long if it came to that. [1] https://www.postgresql.org/message-id/20220921155025.wdixzbrt2uzbi6vz%40jrouhaud ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? 2023-03-07 04:32 Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Michael Paquier <[email protected]> 2023-03-07 06:14 ` Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Thomas Munro <[email protected]> 2023-03-07 06:47 ` Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Julien Rouhaud <[email protected]> @ 2023-03-07 07:33 ` Thomas Munro <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Thomas Munro @ 2023-03-07 07:33 UTC (permalink / raw) To: Julien Rouhaud <[email protected]>; +Cc: Michael Paquier <[email protected]>; Bharath Rupireddy <[email protected]>; Andres Freund <[email protected]>; Alvaro Herrera <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 7, 2023 at 7:47 PM Julien Rouhaud <[email protected]> wrote: > I registered lapwing as a 32b Debian 7 so I thought it would be expected to > keep it as-is rather than upgrading to all newer major Debian versions, > especially since there were newer debian animal registered (no 32b though > AFAICS). Animals do get upgraded: see the "w. e. f." ("with effect from") line in https://buildfarm.postgresql.org/cgi-bin/show_members.pl which comes from people running something like ./update_personality.pl --os-version "11" so that it shows up on the website. > I'm not opposed to upgrading it but I think there's still value in > having somewhat old packages versions being tested, especially since there > isn't much 32b coverage of those. I would be happy to register a newer 32b > version, or even sid, if needed but the -m32 part on the CI makes me think > there isn't much value doing that now. Totally up to you as an animal zoo keeper but in my humble opinion the interesting range of Debian releases currently is 11-13, or maybe 10 if you really want to test the LTS/old-stable release (and CI is testing 11). > I think this is the first time that a problem raised by -Werror on that old > animal is actually a false positive, while there were many times it reported > useful stuff. Now this has been up for years before we got better CI tooling, > especially with -m32 support, so there might not be any value to have it > anymore. As I mentioned at [1] I don't mind removing it or just work on > upgrading any dependency (or removing known buggy compiler flags) to keep it > without being annoying. In any case I'm usually quite fast at reacting to any > problem/complaint on that animal, so you don't have to worry about the > buildfarm being red too long if it came to that. Yeah, it's given us lots of useful data, thanks. Personally I would upgrade it so it keeps telling us useful things but I feel like I've said enough about that so I'll shut up now :-) Re: being red too long... yeah that reminds me, I really need to fix seawasp ASAP... ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-03-07 07:33 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-10 03:40 [PATCH v37 03/11] Add pg_ls_dir_metadata to list a dir with file metadata.. Justin Pryzby <[email protected]> 2023-03-07 04:32 Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Michael Paquier <[email protected]> 2023-03-07 06:14 ` Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Thomas Munro <[email protected]> 2023-03-07 06:47 ` Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Julien Rouhaud <[email protected]> 2023-03-07 07:33 ` Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? Thomas Munro <[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