public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v27 08/11] pg_stat_file and pg_ls_dir_* to use lstat().. 3+ messages / 3 participants [nested] [flat]
* [PATCH v27 08/11] pg_stat_file and pg_ls_dir_* to use lstat().. @ 2020-03-30 23:59 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2020-03-30 23:59 UTC (permalink / raw) pg_ls_dir_* will now skip (no longer show) symbolic links, same as other non-regular file types, as we advertize we do since 8b6d94cf6. That seems to be the intented behavior, since irregular file types are 1) less portable; and, 2) we don't currently show a file's type except for "bool is_dir". pg_stat_file will now 1) show metadata of links themselves, rather than their target; and, 2) specifically, show links to directories with "is_dir=false"; and, 3) not error on broken symlinks. --- doc/src/sgml/func.sgml | 2 +- src/backend/utils/adt/genfile.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 31776a6296..a01971d5fe 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -27034,7 +27034,7 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8'); 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 (or a symbolic link to a directory). + indicating if it is a directory. </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 8f83ec08f6..17e05b853e 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -445,7 +445,7 @@ pg_stat_file(PG_FUNCTION_ARGS) filename = convert_and_check_filename(filename_t); - if (stat(filename, &fst) < 0) + if (lstat(filename, &fst) < 0) { if (missing_ok && errno == ENOENT) PG_RETURN_NULL(); @@ -635,7 +635,7 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags) /* Get the file info */ snprintf(path, sizeof(path), "%s/%s", dir, de->d_name); - if (stat(path, &attrib) < 0) + if (lstat(path, &attrib) < 0) { /* Ignore concurrently-deleted files, else complain */ if (errno == ENOENT) -- 2.17.0 --19uQFt6ulqmgNgg1 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v27-0009-pg_ls_-pg_stat_file-to-show-file-type.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pg_combinebackup does not detect missing files @ 2024-04-18 14:50 Robert Haas <[email protected]> 2024-04-18 23:36 ` Re: pg_combinebackup does not detect missing files David Steele <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Robert Haas @ 2024-04-18 14:50 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: pgsql-hackers On Wed, Apr 17, 2024 at 7:09 PM David Steele <[email protected]> wrote: > I think here: > > + <application>pg_basebackup</application> only attempts to verify > > you mean: > > + <application>pg_combinebackup</application> only attempts to verify > > Otherwise this looks good to me. Good catch, thanks. Committed with that change. > Fair enough. I accept that your reasoning is not random, but I'm still > not very satisfied that the user needs to run a separate and rather > expensive process to do the verification when pg_combinebackup already > has the necessary information at hand. My guess is that most users will > elect to skip verification. I think you're probably right that a lot of people will skip it; I'm just less convinced than you are that it's a bad thing. It's not a *great* thing if people skip it, but restore time is actually just about the worst time to find out that you have a problem with your backups. I think users would be better served by verifying stored backups periodically when they *don't* need to restore them. Also, saying that we have all of the information that we need to do the verification is only partially true: - we do have to parse the manifest anyway, but we don't have to compute checksums anyway, and I think that cost can be significant even for CRC-32C and much more significant for any of the SHA variants - we don't need to read all of the files in all of the backups. if there's a newer full, the corresponding file in older backups, whether full or incremental, need not be read - incremental files other than the most recent only need to be read to the extent that we need their data; if some of the same blocks have been changed again, we can economize How much you save because of these effects is pretty variable. Best case, you have a 2-backup chain with no manifest checksums, and all verification will have to do that you wouldn't otherwise need to do is walk each older directory tree in toto and cross-check which files exist against the manifest. That's probably cheap enough that nobody would be too fussed. Worst case, you have a 10-backup (or whatever) chain with SHA512 checksums and, say, a 50% turnover rate. In that case, I think having verification happen automatically could be a pretty major hit, both in terms of I/O and CPU. If your database is 1TB, it's ~5.5TB of read I/O (because one 1TB full backup and 9 0.5TB incrementals) instead of ~1TB of read I/O, plus the checksumming. Now, obviously you can still feel that it's totally worth it, or that someone in that situation shouldn't even be using incremental backups, and it's a value judgement, so fair enough. But my guess is that the efforts that this implementation makes to minimize the amount of I/O required for a restore are going to be important for a lot of people. > At least now they'll have the information they need to make an informed > choice. Right. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pg_combinebackup does not detect missing files 2024-04-18 14:50 Re: pg_combinebackup does not detect missing files Robert Haas <[email protected]> @ 2024-04-18 23:36 ` David Steele <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: David Steele @ 2024-04-18 23:36 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On 4/19/24 00:50, Robert Haas wrote: > On Wed, Apr 17, 2024 at 7:09 PM David Steele <[email protected]> wrote: > >> Fair enough. I accept that your reasoning is not random, but I'm still >> not very satisfied that the user needs to run a separate and rather >> expensive process to do the verification when pg_combinebackup already >> has the necessary information at hand. My guess is that most users will >> elect to skip verification. > > I think you're probably right that a lot of people will skip it; I'm > just less convinced than you are that it's a bad thing. It's not a > *great* thing if people skip it, but restore time is actually just > about the worst time to find out that you have a problem with your > backups. I think users would be better served by verifying stored > backups periodically when they *don't* need to restore them. Agreed, running verify regularly is a good idea, but in my experience most users are only willing to run verify once they suspect (or know) there is an issue. It's a pretty expensive process depending on how many backups you have and where they are stored. > Also, > saying that we have all of the information that we need to do the > verification is only partially true: > > - we do have to parse the manifest anyway, but we don't have to > compute checksums anyway, and I think that cost can be significant > even for CRC-32C and much more significant for any of the SHA variants > > - we don't need to read all of the files in all of the backups. if > there's a newer full, the corresponding file in older backups, whether > full or incremental, need not be read > > - incremental files other than the most recent only need to be read to > the extent that we need their data; if some of the same blocks have > been changed again, we can economize > > How much you save because of these effects is pretty variable. Best > case, you have a 2-backup chain with no manifest checksums, and all > verification will have to do that you wouldn't otherwise need to do is > walk each older directory tree in toto and cross-check which files > exist against the manifest. That's probably cheap enough that nobody > would be too fussed. Worst case, you have a 10-backup (or whatever) > chain with SHA512 checksums and, say, a 50% turnover rate. In that > case, I think having verification happen automatically could be a > pretty major hit, both in terms of I/O and CPU. If your database is > 1TB, it's ~5.5TB of read I/O (because one 1TB full backup and 9 0.5TB > incrementals) instead of ~1TB of read I/O, plus the checksumming. > > Now, obviously you can still feel that it's totally worth it, or that > someone in that situation shouldn't even be using incremental backups, > and it's a value judgement, so fair enough. But my guess is that the > efforts that this implementation makes to minimize the amount of I/O > required for a restore are going to be important for a lot of people. Sure -- pg_combinebackup would only need to verify the data that it uses. I'm not suggesting that it should do an exhaustive verify of every single backup in the chain. Though I can see how it sounded that way since with pg_verifybackup that would pretty much be your only choice. The beauty of doing verification in pg_combinebackup is that it can do a lot less than running pg_verifybackup against every backup but still get a valid result. All we care about is that the output is correct -- if there is corruption in an unused part of an earlier backup pg_combinebackup doesn't need to care about that. As far as I can see, pg_combinebackup already checks most of the boxes. The only thing I know that it can't do is detect missing files and that doesn't seem like too big a thing to handle. Regards, -David ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-04-18 23:36 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-30 23:59 [PATCH v27 08/11] pg_stat_file and pg_ls_dir_* to use lstat().. Justin Pryzby <[email protected]> 2024-04-18 14:50 Re: pg_combinebackup does not detect missing files Robert Haas <[email protected]> 2024-04-18 23:36 ` Re: pg_combinebackup does not detect missing files David Steele <[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