pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedDo we want to avoid checksumming extra files in the datadir? [was: BUG #19647]
7+ messages / 5 participants
[nested] [flat]
* Do we want to avoid checksumming extra files in the datadir? [was: BUG #19647]
@ 2026-09-03 17:10 Jacob Champion <jacob.champion@enterprisedb.com>
0 siblings, 1 reply; 7+ messages in thread
From: Jacob Champion @ 2026-09-03 17:10 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: Edwin Polkerman <edwin.polkerman@splendiddata.com>
On Thu, Sep 3, 2026 at 9:49 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> > I will file a bug report for this issue in github for the third-party extension pgactive. Please let me know if you want me to report an enhancement request or thet you take care about that
>
> I'm happy to shepherd the conversation over to -hackers, and will do
> that shortly. (I'm probably not the right person to drive it.)
Moving to -hackers.
I can't remember the last conversation I saw on dealing with
third-party files in our data directory, sorry. Occasionally we're
fine with them, and occasionally we try to parse apart their filenames
and fail during checksum operations. I don't think that's a bug, but
an actionable complaint from the bug report is that PG17 relaxed this
requirement for pg_basebackup [1] and not pg_checksums, which is kind
of weird.
The inciting event is that an extension drops a file called
pgactive.stat into global/. We could say "don't do that", in which
case I think we should follow Robert's idea in [1] and complain about
the existence of the file itself rather than the lack of checksummable
material. Or we could say "that's fine" and adjust pg_checksums to
continue onwards if the filename isn't what we expect.
Or we could decide that the existing behavior is what we want, and
adjust the following comment in pg_checksums.c to no longer refer to a
dead implementation:
> /*
> * List of files excluded from checksum validation.
> *
> * Note: this list should be kept in sync with what basebackup.c includes.
> */
and probably adjust this code so that it doesn't always print "segment
number 0", which isn't derived from the filename:
> segmentno = atoi(segmentpath);
> if (segmentno == 0)
> pg_fatal("invalid segment number %d in file name \"%s\"",
> segmentno, fn);
Or something else entirely. Thoughts?
Thanks,
--Jacob
[1] https://postgr.es/c/025584a16
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Do we want to avoid checksumming extra files in the datadir? [was: BUG #19647]
@ 2026-09-03 17:20 Daniel Gustafsson <daniel@yesql.se>
parent: Jacob Champion <jacob.champion@enterprisedb.com>
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Gustafsson @ 2026-09-03 17:20 UTC (permalink / raw)
To: Jacob Champion <jacob.champion@enterprisedb.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Edwin Polkerman <edwin.polkerman@splendiddata.com>
> On 3 Sep 2026, at 19:10, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
>
> On Thu, Sep 3, 2026 at 9:49 AM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
>>> I will file a bug report for this issue in github for the third-party extension pgactive. Please let me know if you want me to report an enhancement request or thet you take care about that
>>
>> I'm happy to shepherd the conversation over to -hackers, and will do
>> that shortly. (I'm probably not the right person to drive it.)
>
> Moving to -hackers.
>
> I can't remember the last conversation I saw on dealing with
> third-party files in our data directory, sorry. Occasionally we're
> fine with them, and occasionally we try to parse apart their filenames
> and fail during checksum operations. I don't think that's a bug, but
> an actionable complaint from the bug report is that PG17 relaxed this
> requirement for pg_basebackup [1] and not pg_checksums, which is kind
> of weird.
>
> The inciting event is that an extension drops a file called
> pgactive.stat into global/. We could say "don't do that", in which
> case I think we should follow Robert's idea in [1] and complain about
> the existence of the file itself rather than the lack of checksummable
> material. Or we could say "that's fine" and adjust pg_checksums to
> continue onwards if the filename isn't what we expect.
I'm not sure I have a strong preference, keeping it in sync with pg_basebackup
seems like a pretty good strategy to make it easier for users.
pg_checksums won't be immune to false negatives even with this as it will still
be able to read stale leftover datafiles from crashed DROP commands.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Do we want to avoid checksumming extra files in the datadir? [was: BUG #19647]
@ 2026-09-03 17:29 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2026-09-03 17:29 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Jacob Champion <jacob.champion@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Edwin Polkerman <edwin.polkerman@splendiddata.com>
Daniel Gustafsson <daniel@yesql.se> writes:
> On 3 Sep 2026, at 19:10, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
>> The inciting event is that an extension drops a file called
>> pgactive.stat into global/. We could say "don't do that", in which
>> case I think we should follow Robert's idea in [1] and complain about
>> the existence of the file itself rather than the lack of checksummable
>> material. Or we could say "that's fine" and adjust pg_checksums to
>> continue onwards if the filename isn't what we expect.
> I'm not sure I have a strong preference, keeping it in sync with pg_basebackup
> seems like a pretty good strategy to make it easier for users.
It looks to me like there was a fair amount of consensus behind
025584a16, so I think we should conclude that pg_basebackup's new
behavior (ignore stray files) is the right thing and it was an
oversight that pg_checksums wasn't adjusted similarly.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Do we want to avoid checksumming extra files in the datadir? [was: BUG #19647]
@ 2026-09-03 18:30 Jacob Champion <jacob.champion@enterprisedb.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Jacob Champion @ 2026-09-03 18:30 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Edwin Polkerman <edwin.polkerman@splendiddata.com>
On Thu, Sep 3, 2026 at 10:29 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
> > I'm not sure I have a strong preference, keeping it in sync with pg_basebackup
> > seems like a pretty good strategy to make it easier for users.
>
> It looks to me like there was a fair amount of consensus behind
> 025584a16, so I think we should conclude that pg_basebackup's new
> behavior (ignore stray files) is the right thing and it was an
> oversight that pg_checksums wasn't adjusted similarly.
Sounds good, thanks both! Attached is the simplest thing that could
fix the reported problem (and nothing else), but I'd rather look into
moving parse_filename_for_nontemp_relation() to common/relfile.c so it
can be used directly. I probably won't have time for that today.
--Jacob
Attachments:
[application/octet-stream] 0001-WIP-pg_checksums-Ignore-more-stray-files-in-director.patch (2.5K, ../../CAOYmi+kDSmfQS-H9xE2kbpUNdXNicSgEyqMaXe5EnF1wXU-K7Q@mail.gmail.com/2-0001-WIP-pg_checksums-Ignore-more-stray-files-in-director.patch)
download | inline diff:
From 9c081ed3cff8354ad031498dfb219ab34a15315c Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Thu, 3 Sep 2026 10:42:48 -0700
Subject: [PATCH] WIP: pg_checksums: Ignore more stray files in directories
Commit 025584a16 taught pg_basebackup to ignore files that don't belong
to the cluster during checksum operations, but pg_checksums itself was
left as-is. Align them better by having pg_checksums continue past files
that don't contain a valid segment number.
TODO: To align them completely, look into parse_filename_for_nontemp_relation()
For PG20 only. The inconsistency is odd, but it doesn't seem to rise to
bug level to me.
Reported-by: Edwin Polkerman <edwin.polkerman@splendiddata.com>
Discussion: https://postgr.es/m/CAOYmi%2B%3DE0zz%2B_MgQXAoOyn5C0vW53740izzRiTv6excqtCJfRg%40mail.gmail.com
---
src/bin/pg_checksums/pg_checksums.c | 8 ++++----
src/bin/pg_checksums/t/002_actions.pl | 4 ++++
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 3b3ae23f1a6..fc3e9c5dc21 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -102,8 +102,6 @@ struct exclude_list_item
/*
* List of files excluded from checksum validation.
- *
- * Note: this list should be kept in sync with what basebackup.c includes.
*/
static const struct exclude_list_item skip[] = {
{"pg_control", false},
@@ -360,8 +358,10 @@ scan_directory(const char *basedir, const char *subdir, bool sizeonly)
*segmentpath++ = '\0';
segmentno = atoi(segmentpath);
if (segmentno == 0)
- pg_fatal("invalid segment number %d in file name \"%s\"",
- segmentno, fn);
+ {
+ /* not a valid segment */
+ continue;
+ }
}
forkpath = strchr(fnonly, '_');
diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl
index 94a11a534e9..bef9b0d2d13 100644
--- a/src/bin/pg_checksums/t/002_actions.pl
+++ b/src/bin/pg_checksums/t/002_actions.pl
@@ -124,6 +124,10 @@ append_to_file "$pgdata/global/pg_internal.init.123", "foo";
append_to_file "$pgdata/global/.DS_Store", "foo"
unless ($Config{osname} eq 'darwin');
+# This is a file that doesn't look anything like our relfile segments, so it
+# should be ignored.
+append_to_file "$pgdata/global/bar.baz", "foo";
+
# Enable checksums.
command_ok([ 'pg_checksums', '--enable', '--no-sync', '--pgdata' => $pgdata ],
"checksums successfully enabled in cluster");
--
2.34.1
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Do we want to avoid checksumming extra files in the datadir? [was: BUG #19647]
@ 2026-09-15 18:51 Jacob Champion <jacob.champion@enterprisedb.com>
parent: Jacob Champion <jacob.champion@enterprisedb.com>
0 siblings, 2 replies; 7+ messages in thread
From: Jacob Champion @ 2026-09-15 18:51 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Edwin Polkerman <edwin.polkerman@splendiddata.com>
On Thu, Sep 3, 2026 at 11:30 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Sounds good, thanks both! Attached is the simplest thing that could
> fix the reported problem (and nothing else), but I'd rather look into
> moving parse_filename_for_nontemp_relation() to common/relfile.c so it
> can be used directly. I probably won't have time for that today.
Here's a v2 to do that, which I like much better.
The --filenode argument now filters via integer equality rather than a
string comparison. I think there are two main side effects (let me
know if either is unacceptable):
1) we now prohibit `pg_checksums --filenode 0`, which IIUC isn't ever
helpful in practice, and
2) leading zeroes in the --filenode arg are now ignored, rather than
causing pg_checksums to match nothing. I.e. `--filenode 001260` will
now match relfile 1260.
Are there are any corner cases I've missed where you want pg_checksums
to check a temporary relation's relfiles? Even if a clean shutdown and
startup somehow left them around, they still wouldn't be used, right?
Thanks,
--Jacob
Attachments:
[application/octet-stream] v2-0001-Move-relfile-name-parsing-to-src-common.patch (6.9K, ../../CAOYmi+mMy4RFVcicTiHj6Yqkj=0_E6uAHR56NwgZXWc9tcDT7A@mail.gmail.com/2-v2-0001-Move-relfile-name-parsing-to-src-common.patch)
download | inline diff:
From 650d1054f61854967d1658f8f7f294a8c1e7deea Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Fri, 4 Sep 2026 13:11:53 -0700
Subject: [PATCH v2 1/3] Move relfile name parsing to src/common
This will allow pg_checksums to use the same relfile parsing logic as
the backend in a future commit.
Discussion: https://postgr.es/m/CAOYmi%2B%3DE0zz%2B_MgQXAoOyn5C0vW53740izzRiTv6excqtCJfRg%40mail.gmail.com
---
src/include/common/relpath.h | 5 ++
src/include/storage/reinit.h | 4 --
src/backend/storage/file/reinit.c | 86 -------------------------------
src/common/relpath.c | 86 +++++++++++++++++++++++++++++++
4 files changed, 91 insertions(+), 90 deletions(-)
diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
index 9772125be73..8187cc8ad6b 100644
--- a/src/include/common/relpath.h
+++ b/src/include/common/relpath.h
@@ -133,6 +133,11 @@ extern char *GetDatabasePath(Oid dbOid, Oid spcOid);
extern RelPathStr GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
int procNumber, ForkNumber forkNumber);
+extern bool parse_filename_for_nontemp_relation(const char *name,
+ RelFileNumber *relnumber,
+ ForkNumber *fork,
+ unsigned *segno);
+
/*
* Wrapper macros for GetRelationPath. Beware of multiple
* evaluation of the RelFileLocator or RelFileLocatorBackend argument!
diff --git a/src/include/storage/reinit.h b/src/include/storage/reinit.h
index a053742f695..efdb58ca77e 100644
--- a/src/include/storage/reinit.h
+++ b/src/include/storage/reinit.h
@@ -19,10 +19,6 @@
extern void ResetUnloggedRelations(int op);
-extern bool parse_filename_for_nontemp_relation(const char *name,
- RelFileNumber *relnumber,
- ForkNumber *fork,
- unsigned *segno);
#define UNLOGGED_RELATION_CLEANUP 0x0001
#define UNLOGGED_RELATION_INIT 0x0002
diff --git a/src/backend/storage/file/reinit.c b/src/backend/storage/file/reinit.c
index 25fa2151309..496b2974a78 100644
--- a/src/backend/storage/file/reinit.c
+++ b/src/backend/storage/file/reinit.c
@@ -365,89 +365,3 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
fsync_fname(dbspacedirname, true);
}
}
-
-/*
- * Basic parsing of putative relation filenames.
- *
- * This function returns true if the file appears to be in the correct format
- * for a non-temporary relation and false otherwise.
- *
- * If it returns true, it sets *relnumber, *fork, and *segno to the values
- * extracted from the filename. If it returns false, these values are set to
- * InvalidRelFileNumber, InvalidForkNumber, and 0, respectively.
- */
-bool
-parse_filename_for_nontemp_relation(const char *name, RelFileNumber *relnumber,
- ForkNumber *fork, unsigned *segno)
-{
- unsigned long n,
- s;
- ForkNumber f;
- char *endp;
-
- *relnumber = InvalidRelFileNumber;
- *fork = InvalidForkNumber;
- *segno = 0;
-
- /*
- * Relation filenames should begin with a digit that is not a zero. By
- * rejecting cases involving leading zeroes, the caller can assume that
- * there's only one possible string of characters that could have produced
- * any given value for *relnumber.
- *
- * (To be clear, we don't expect files with names like 0017.3 to exist at
- * all -- but if 0017.3 does exist, it's a non-relation file, not part of
- * the main fork for relfilenode 17.)
- */
- if (name[0] < '1' || name[0] > '9')
- return false;
-
- /*
- * Parse the leading digit string. If the value is out of range, we
- * conclude that this isn't a relation file at all.
- */
- errno = 0;
- n = strtoul(name, &endp, 10);
- if (errno || name == endp || n <= 0 || n > PG_UINT32_MAX)
- return false;
- name = endp;
-
- /* Check for a fork name. */
- if (*name != '_')
- f = MAIN_FORKNUM;
- else
- {
- int forkchar;
-
- forkchar = forkname_chars(name + 1, &f);
- if (forkchar <= 0)
- return false;
- name += forkchar + 1;
- }
-
- /* Check for a segment number. */
- if (*name != '.')
- s = 0;
- else
- {
- /* Reject leading zeroes, just like we do for RelFileNumber. */
- if (name[1] < '1' || name[1] > '9')
- return false;
-
- errno = 0;
- s = strtoul(name + 1, &endp, 10);
- if (errno || name + 1 == endp || s <= 0 || s > PG_UINT32_MAX)
- return false;
- name = endp;
- }
-
- /* Now we should be at the end. */
- if (*name != '\0')
- return false;
-
- /* Set out parameters and return. */
- *relnumber = (RelFileNumber) n;
- *fork = f;
- *segno = (unsigned) s;
- return true;
-}
diff --git a/src/common/relpath.c b/src/common/relpath.c
index 8fb3bed7873..eb0c4aaf5e1 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -220,3 +220,89 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
return rp;
}
+
+/*
+ * Basic parsing of putative relation filenames.
+ *
+ * This function returns true if the file appears to be in the correct format
+ * for a non-temporary relation and false otherwise.
+ *
+ * If it returns true, it sets *relnumber, *fork, and *segno to the values
+ * extracted from the filename. If it returns false, these values are set to
+ * InvalidRelFileNumber, InvalidForkNumber, and 0, respectively.
+ */
+bool
+parse_filename_for_nontemp_relation(const char *name, RelFileNumber *relnumber,
+ ForkNumber *fork, unsigned *segno)
+{
+ unsigned long n,
+ s;
+ ForkNumber f;
+ char *endp;
+
+ *relnumber = InvalidRelFileNumber;
+ *fork = InvalidForkNumber;
+ *segno = 0;
+
+ /*
+ * Relation filenames should begin with a digit that is not a zero. By
+ * rejecting cases involving leading zeroes, the caller can assume that
+ * there's only one possible string of characters that could have produced
+ * any given value for *relnumber.
+ *
+ * (To be clear, we don't expect files with names like 0017.3 to exist at
+ * all -- but if 0017.3 does exist, it's a non-relation file, not part of
+ * the main fork for relfilenode 17.)
+ */
+ if (name[0] < '1' || name[0] > '9')
+ return false;
+
+ /*
+ * Parse the leading digit string. If the value is out of range, we
+ * conclude that this isn't a relation file at all.
+ */
+ errno = 0;
+ n = strtoul(name, &endp, 10);
+ if (errno || name == endp || n <= 0 || n > PG_UINT32_MAX)
+ return false;
+ name = endp;
+
+ /* Check for a fork name. */
+ if (*name != '_')
+ f = MAIN_FORKNUM;
+ else
+ {
+ int forkchar;
+
+ forkchar = forkname_chars(name + 1, &f);
+ if (forkchar <= 0)
+ return false;
+ name += forkchar + 1;
+ }
+
+ /* Check for a segment number. */
+ if (*name != '.')
+ s = 0;
+ else
+ {
+ /* Reject leading zeroes, just like we do for RelFileNumber. */
+ if (name[1] < '1' || name[1] > '9')
+ return false;
+
+ errno = 0;
+ s = strtoul(name + 1, &endp, 10);
+ if (errno || name + 1 == endp || s <= 0 || s > PG_UINT32_MAX)
+ return false;
+ name = endp;
+ }
+
+ /* Now we should be at the end. */
+ if (*name != '\0')
+ return false;
+
+ /* Set out parameters and return. */
+ *relnumber = (RelFileNumber) n;
+ *fork = f;
+ *segno = (unsigned) s;
+ return true;
+}
--
2.34.1
[application/octet-stream] v2-0002-pg_checksums-Disallow-filenode-0.patch (1.6K, ../../CAOYmi+mMy4RFVcicTiHj6Yqkj=0_E6uAHR56NwgZXWc9tcDT7A@mail.gmail.com/3-v2-0002-pg_checksums-Disallow-filenode-0.patch)
download | inline diff:
From 93ee70a8e8143dba97f4242549d3ea35457d7b3e Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Fri, 4 Sep 2026 14:30:45 -0700
Subject: [PATCH v2 2/3] pg_checksums: Disallow --filenode 0
Matching a relfile with InvalidRelFileNumber doesn't make too much
sense. In anticipation of modifying pg_checksums to only validate known
relfiles, forbid that.
Discussion: https://postgr.es/m/CAOYmi%2B%3DE0zz%2B_MgQXAoOyn5C0vW53740izzRiTv6excqtCJfRg%40mail.gmail.com
---
src/bin/pg_checksums/pg_checksums.c | 2 +-
src/bin/pg_checksums/t/001_basic.pl | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 3b58c6ca608..5bb7a7bd200 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -488,7 +488,7 @@ main(int argc, char *argv[])
mode = PG_MODE_ENABLE;
break;
case 'f':
- if (!option_parse_int(optarg, "-f/--filenode", 0,
+ if (!option_parse_int(optarg, "-f/--filenode", 1,
INT_MAX,
NULL))
exit(1);
diff --git a/src/bin/pg_checksums/t/001_basic.pl b/src/bin/pg_checksums/t/001_basic.pl
index fc5d431c2b0..996670d81c7 100644
--- a/src/bin/pg_checksums/t/001_basic.pl
+++ b/src/bin/pg_checksums/t/001_basic.pl
@@ -10,4 +10,9 @@ program_help_ok('pg_checksums');
program_version_ok('pg_checksums');
program_options_handling_ok('pg_checksums');
+command_fails_like(
+ [ 'pg_checksums', '--filenode' => '0' ],
+ qr@\Qpg_checksums: error: -f/--filenode must be in range 1..2147483647\E@,
+ 'pg_checksums: --filenode must be in range');
+
done_testing();
--
2.34.1
[application/octet-stream] v2-0003-WIP-pg_checksums-Ignore-stray-files-in-directorie.patch (6.5K, ../../CAOYmi+mMy4RFVcicTiHj6Yqkj=0_E6uAHR56NwgZXWc9tcDT7A@mail.gmail.com/4-v2-0003-WIP-pg_checksums-Ignore-stray-files-in-directorie.patch)
download | inline diff:
From 58f4c263acab28a38abf67fb3194f965d723bbc7 Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Thu, 3 Sep 2026 10:42:48 -0700
Subject: [PATCH v2 3/3] WIP: pg_checksums: Ignore stray files in directories
Commit 025584a16 taught pg_basebackup to ignore files that don't belong
to the cluster during checksum operations, but pg_checksums itself was
left as-is. Align them now and get rid of some special-case code in
pg_checksums.
For PG20 only. The inconsistency in back branches is odd, but it doesn't
seem to rise to bug level, and it'd be good to put this new behavior
through a beta.
TODO: make absolutely sure we can skip temp relations here
Reported-by: Edwin Polkerman <edwin.polkerman@splendiddata.com>
Discussion: https://postgr.es/m/CAOYmi%2B%3DE0zz%2B_MgQXAoOyn5C0vW53740izzRiTv6excqtCJfRg%40mail.gmail.com
---
src/bin/pg_checksums/pg_checksums.c | 91 +++++----------------------
src/bin/pg_checksums/t/002_actions.pl | 10 ++-
2 files changed, 24 insertions(+), 77 deletions(-)
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5bb7a7bd200..ad25c7a9e08 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -40,7 +40,7 @@ static int64 blocks_written = 0;
static int64 badblocks = 0;
static ControlFileData *ControlFile;
-static char *only_filenode = NULL;
+static int only_filenode = 0;
static bool do_sync = true;
static bool verbose = false;
static bool showprogress = false;
@@ -88,34 +88,6 @@ usage(void)
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
-/*
- * Definition of one element part of an exclusion list, used for files
- * to exclude from checksum validation. "name" is the name of the file
- * or path to check for exclusion. If "match_prefix" is true, any items
- * matching the name as prefix are excluded.
- */
-struct exclude_list_item
-{
- const char *name;
- bool match_prefix;
-};
-
-/*
- * List of files excluded from checksum validation.
- *
- * Note: this list should be kept in sync with what basebackup.c includes.
- */
-static const struct exclude_list_item skip[] = {
- {"pg_control", false},
- {"pg_filenode.map", false},
- {"pg_internal.init", true},
- {"PG_VERSION", false},
-#ifdef EXEC_BACKEND
- {"config_exec_params", true},
-#endif
- {NULL, false}
-};
-
/*
* Report current progress status. Parts borrowed from
* src/bin/pg_basebackup/pg_basebackup.c.
@@ -154,26 +126,8 @@ progress_report(bool finished)
fputc((!finished && isatty(fileno(stderr))) ? '\r' : '\n', stderr);
}
-static bool
-skipfile(const char *fn)
-{
- int excludeIdx;
-
- for (excludeIdx = 0; skip[excludeIdx].name != NULL; excludeIdx++)
- {
- int cmplen = strlen(skip[excludeIdx].name);
-
- if (!skip[excludeIdx].match_prefix)
- cmplen++;
- if (strncmp(skip[excludeIdx].name, fn, cmplen) == 0)
- return true;
- }
-
- return false;
-}
-
static void
-scan_file(const char *fn, int segmentno)
+scan_file(const char *fn, unsigned segmentno)
{
PGIOAlignedBlock buf;
PageHeader header = (PageHeader) buf.data;
@@ -339,36 +293,22 @@ scan_directory(const char *basedir, const char *subdir, bool sizeonly)
pg_fatal("could not stat file \"%s\": %m", fn);
if (S_ISREG(st.st_mode))
{
- char fnonly[MAXPGPATH];
- char *forkpath,
- *segmentpath;
- int segmentno = 0;
-
- if (skipfile(de->d_name))
- continue;
+ RelFileNumber relfileno;
+ ForkNumber fork;
+ unsigned segmentno;
/*
- * Cut off at the segment boundary (".") to get the segment number
- * in order to mix it into the checksum. Then also cut off at the
- * fork boundary, to get the filenode the file belongs to for
- * filtering.
+ * If this is a relfile, get the segment number in order to mix it
+ * into the checksum, and filter on the relfile number if
+ * requested. Skip any temporary relations and anything that's not
+ * a relation at all.
*/
- strlcpy(fnonly, de->d_name, sizeof(fnonly));
- segmentpath = strchr(fnonly, '.');
- if (segmentpath != NULL)
- {
- *segmentpath++ = '\0';
- segmentno = atoi(segmentpath);
- if (segmentno == 0)
- pg_fatal("invalid segment number %d in file name \"%s\"",
- segmentno, fn);
- }
-
- forkpath = strchr(fnonly, '_');
- if (forkpath != NULL)
- *forkpath++ = '\0';
+ if (!parse_filename_for_nontemp_relation(de->d_name,
+ &relfileno, &fork,
+ &segmentno))
+ continue;
- if (only_filenode && strcmp(only_filenode, fnonly) != 0)
+ if (only_filenode && relfileno != only_filenode)
/* filenode not to be included */
continue;
@@ -490,9 +430,8 @@ main(int argc, char *argv[])
case 'f':
if (!option_parse_int(optarg, "-f/--filenode", 1,
INT_MAX,
- NULL))
+ &only_filenode))
exit(1);
- only_filenode = pstrdup(optarg);
break;
case 'N':
do_sync = false;
diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl
index 94a11a534e9..d3fea8ef3f3 100644
--- a/src/bin/pg_checksums/t/002_actions.pl
+++ b/src/bin/pg_checksums/t/002_actions.pl
@@ -110,13 +110,14 @@ append_to_file "$pgdata/global/99999_init.123", "";
append_to_file "$pgdata/global/99999_fsm.123", "";
append_to_file "$pgdata/global/99999_vm.123", "";
-# These are temporary files and folders with dummy contents, which
+# These are temporary files/folders/relations with dummy contents, which
# should be ignored by the scan.
append_to_file "$pgdata/global/pgsql_tmp_123", "foo";
mkdir "$pgdata/global/pgsql_tmp";
append_to_file "$pgdata/global/pgsql_tmp/1.1", "foo";
append_to_file "$pgdata/global/pg_internal.init", "foo";
append_to_file "$pgdata/global/pg_internal.init.123", "foo";
+append_to_file "$pgdata/global/t1_99999", "foo";
# These are non-postgres macOS files, which should be ignored by the scan.
# Only perform this test on non-macOS systems though as creating incorrect
@@ -124,6 +125,13 @@ append_to_file "$pgdata/global/pg_internal.init.123", "foo";
append_to_file "$pgdata/global/.DS_Store", "foo"
unless ($Config{osname} eq 'darwin');
+# These don't look like our relfile segments, so they should be ignored too.
+append_to_file "$pgdata/global/foo", "foo";
+append_to_file "$pgdata/global/bar.baz", "foo";
+append_to_file "$pgdata/global/001", "foo";
+append_to_file "$pgdata/global/1.001", "foo";
+append_to_file "$pgdata/global/1_foobar", "foo";
+
# Enable checksums.
command_ok([ 'pg_checksums', '--enable', '--no-sync', '--pgdata' => $pgdata ],
"checksums successfully enabled in cluster");
--
2.34.1
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Do we want to avoid checksumming extra files in the datadir? [was: BUG #19647]
@ 2026-09-30 09:43 Rui Zhao <zhaorui126@gmail.com>
parent: Jacob Champion <jacob.champion@enterprisedb.com>
1 sibling, 0 replies; 7+ messages in thread
From: Rui Zhao @ 2026-09-30 09:43 UTC (permalink / raw)
To: Jacob Champion <jacob.champion@enterprisedb.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Daniel Gustafsson <daniel@yesql.se>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Edwin Polkerman <edwin.polkerman@splendiddata.com>
Hi Jacob,
> Are there are any corner cases I've missed where you want pg_checksums
> to check a temporary relation's relfiles?
Skipping them looks right to me. A normal server restart removes
leftover temporary files, and after a backend crash they are also
removed by default. I set remove_temp_files_after_crash = off to
retain them, created temporary tables in pg_default and a separate
tablespace, then sent SIGKILL (kill -9) to the backend that created
both tables. After automatic recovery, I cleanly shut down the server.
Both temporary relation files were still present. With v2,
pg_checksums --check --verbose skipped both files. The next postmaster
startup removed them even with remove_temp_files_after_crash still off.
The --filenode changes look fine too. Zero is not a valid filenode,
so rejecting it is reasonable. Numeric comparison also makes sense:
adding leading zeroes to the argument does not identify a different
relation.
In my test, --filenode 0016385 checked the same three files as
--filenode 16385. After I deliberately corrupted a page checksum in
that relation, --filenode 0016385 reported the checksum failure.
--filenode 0 was rejected.
Could we add a leading-zero --filenode case to
check_relation_corruption() in src/bin/pg_checksums/t/002_actions.pl?
After corrupt_page_checksum(), using "00$relfilenode_corrupted" should
still report one bad checksum and exit 1. Checking only for success
would also pass with the old behavior of matching no files.
Could 0003's commit message also mention the change from string to
numeric comparison for --filenode, including the leading-zero behavior?
The core regression, pg_checksums and pg_basebackup tests passed, as
did t/014_unlogged_reinit.pl and t/022_crash_temp_files.pl in
src/test/recovery. I didn't find a functional issue with v2.
Regards,
Rui
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Do we want to avoid checksumming extra files in the datadir? [was: BUG #19647]
@ 2026-10-07 11:04 Shubhra Jain <shubhra.jain@ksolves.com>
parent: Jacob Champion <jacob.champion@enterprisedb.com>
1 sibling, 0 replies; 7+ messages in thread
From: Shubhra Jain @ 2026-10-07 11:04 UTC (permalink / raw)
To: Jacob Champion <jacob.champion@enterprisedb.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Daniel Gustafsson <daniel@yesql.se>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Edwin Polkerman <edwin.polkerman@splendiddata.com>
Hi Jacob, all,
I applied v2-0001..0003 on top of master (e40b509c08) and tested them.
- pg_checksums (77 tests) and pg_basebackup (352 tests) TAP tests pass.
- With stray files in global/ ("foreign_file" with 4 bytes and
"foreign.conf"), unpatched pg_checksums fails ("could not read block
0 ... read 4 of 8192"; the dotted name gave "invalid segment number
0"), while v2 exits 0.
- On a cluster with a table, an index and an unlogged table, patched
and unpatched report identical counts (956 files, 9362 blocks).
- "--filenode 0" is rejected with "must be in range 1..2147483647".
Two things I noticed while reviewing:
1. doc/src/sgml/ref/pg_checksums.sgml still says that when verifying
checksums "every file in the cluster is scanned". With 0003 that is
no longer accurate; it could say "every relation file" and mention
that other files are ignored.
2. Per Rui's review, the 0003 commit message could mention that
--filenode is now compared numerically, and a leading-zero
--filenode case is not yet in check_relation_corruption().
I'm happy to write the docs and test changes if you'd like.
Thanks,
Shubhra
On Wed, Oct 7, 2026 at 4:32 PM Jacob Champion <
jacob.champion@enterprisedb.com> wrote:
> On Thu, Sep 3, 2026 at 11:30 AM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
> > Sounds good, thanks both! Attached is the simplest thing that could
> > fix the reported problem (and nothing else), but I'd rather look into
> > moving parse_filename_for_nontemp_relation() to common/relfile.c so it
> > can be used directly. I probably won't have time for that today.
>
> Here's a v2 to do that, which I like much better.
>
> The --filenode argument now filters via integer equality rather than a
> string comparison. I think there are two main side effects (let me
> know if either is unacceptable):
> 1) we now prohibit `pg_checksums --filenode 0`, which IIUC isn't ever
> helpful in practice, and
> 2) leading zeroes in the --filenode arg are now ignored, rather than
> causing pg_checksums to match nothing. I.e. `--filenode 001260` will
> now match relfile 1260.
>
> Are there are any corner cases I've missed where you want pg_checksums
> to check a temporary relation's relfiles? Even if a clean shutdown and
> startup somehow left them around, they still wouldn't be used, right?
>
> Thanks,
> --Jacob
>
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-10-07 11:04 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-03 17:10 Do we want to avoid checksumming extra files in the datadir? [was: BUG #19647] Jacob Champion <jacob.champion@enterprisedb.com>
2026-09-03 17:20 ` Daniel Gustafsson <daniel@yesql.se>
2026-09-03 17:29 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-09-03 18:30 ` Jacob Champion <jacob.champion@enterprisedb.com>
2026-09-15 18:51 ` Jacob Champion <jacob.champion@enterprisedb.com>
2026-09-30 09:43 ` Rui Zhao <zhaorui126@gmail.com>
2026-10-07 11:04 ` Shubhra Jain <shubhra.jain@ksolves.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox