pg_basebackup: errors on macOS on directories with ".DS_Store" files

pg_basebackup: error: backup failed: ERROR: invalid segment number 0 in file ".DS_Store"

This seem to be the same problem as already discussed in [1]</messages/by-id/20181021134206.GA14282@paquier.xyz&gt;: Tools like pg_basebackup and pg_checksums can get upset if there are random extra files in $PGDATA/base, /global or /pg_tblspc. Unfortunately, macOS has the habit to create such files quite easily. If I read the outcome correctly, we are back at a blacklist approach, so adding ".DS_Store" may be something to consider.

[1]: </messages/by-id/20181021134206.GA14282@paquier.xyz&gt;

Tobias Bussmann <t.bussmann@gmx.net> writes:

pg_basebackup: error: backup failed: ERROR: invalid segment number 0 in file ".DS_Store"

This seem to be the same problem as already discussed in [1]: Tools like pg_basebackup and pg_checksums can get upset if there are random extra files in $PGDATA/base, /global or /pg_tblspc. Unfortunately, macOS has the habit to create such files quite easily. If I read the outcome correctly, we are back at a blacklist approach, so adding ".DS_Store" may be something to consider.
[1]: </messages/by-id/20181021134206.GA14282@paquier.xyz&gt;

Yeah. I wonder if we ought to do something more general, and
ignore all files whose names start with ".".

regards, tom lane

On Wed, Apr 19, 2023 at 02:15:51PM -0400, Tom Lane wrote:

Yeah. I wonder if we ought to do something more general, and
ignore all files whose names start with ".".

Indeed. I don't see any point is adding hidden files for the
checksum, rewind and base backup lists, so we could just make it a
hardcoded rule. That's currently what we do for configuration files
when using include_dir for postgresql.conf (as well as hba and ident
files in 16~).
--
Michael

On 20 Apr 2023, at 02:15, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Apr 19, 2023 at 02:15:51PM -0400, Tom Lane wrote:

Yeah. I wonder if we ought to do something more general, and
ignore all files whose names start with ".".

Indeed. I don't see any point is adding hidden files for the
checksum, rewind and base backup lists, so we could just make it a
hardcoded rule. That's currently what we do for configuration files
when using include_dir for postgresql.conf (as well as hba and ident
files in 16~).

+1, it will keep editor leftovers away as well.

--
Daniel Gustafsson

On 20 Apr 2023, at 02:15, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Apr 19, 2023 at 02:15:51PM -0400, Tom Lane wrote:

Yeah. I wonder if we ought to do something more general, and
ignore all files whose names start with ".".

Indeed. I don't see any point is adding hidden files for the
checksum, rewind and base backup lists, so we could just make it a
hardcoded rule. That's currently what we do for configuration files
when using include_dir for postgresql.conf (as well as hba and ident
files in 16~).

The attached trivial diff skips hidden files for pg_basebackup and pg_checksums
as part of the checks already performed by these tools for skipping other
files. Skimming other callsites of ReadDir and readdir I didn't see any others
which could benefit from skipping hidden files.

--
Daniel Gustafsson

Am 20.04.2023 um 10:50 schrieb Daniel Gustafsson <daniel@yesql.se>:

Skimming other callsites of ReadDir and readdir I didn't see any others
which could benefit from skipping hidden files.

Could /src/bin/pg_rewind/filemap.c be another candidate for skipping hidden files? The 'copy all other files'-behaviour seems to be similar to what pg_basebackup does and it uses the same exclude_list_item blacklist mechanism.

--
Tobias Bussmann

On 20 Apr 2023, at 11:51, Tobias Bussmann <t.bussmann@gmx.net> wrote:

Am 20.04.2023 um 10:50 schrieb Daniel Gustafsson <daniel@yesql.se>:

Skimming other callsites of ReadDir and readdir I didn't see any others
which could benefit from skipping hidden files.

Could /src/bin/pg_rewind/filemap.c be another candidate for skipping hidden files? The 'copy all other files'-behaviour seems to be similar to what pg_basebackup does and it uses the same exclude_list_item blacklist mechanism.

Maybe. I'm a bit hesitant to add too many smarts to pg_rewind. It's a tool
for when something has gone wrong with a cluster (albeit probably not at the
filesystem level), and at that point I feel it's better to put the user fully
in charge. Perhaps I'm overly cautious, curious to hear from others.

--
Daniel Gustafsson

On Thu, Apr 20, 2023 at 01:15:40PM +0200, Daniel Gustafsson wrote:

Maybe. I'm a bit hesitant to add too many smarts to pg_rewind. It's a tool
for when something has gone wrong with a cluster (albeit probably not at the
filesystem level), and at that point I feel it's better to put the user fully
in charge. Perhaps I'm overly cautious, curious to hear from others.

Hmm. pg_rewind is mostly a differential block-level backup tool, so
applying the same rules everywhere across the board would be sensible
here. See that exclude_list_item is able to handle prefixes, and we
may want to extend the same logic for the directory list, as well..

By the way, the patch ought to add some tests? For pg_basebackup,
this would be around "These files should not be copied" in
010_pg_basebackup.pl. pg_checksums has also its own checks in
002_actions.pl.
--
Michael

At Fri, 21 Apr 2023 07:33:09 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Thu, Apr 20, 2023 at 01:15:40PM +0200, Daniel Gustafsson wrote:

Maybe. I'm a bit hesitant to add too many smarts to pg_rewind. It's a tool
for when something has gone wrong with a cluster (albeit probably not at the
filesystem level), and at that point I feel it's better to put the user fully
in charge. Perhaps I'm overly cautious, curious to hear from others.

Hmm. pg_rewind is mostly a differential block-level backup tool, so
applying the same rules everywhere across the board would be sensible
here. See that exclude_list_item is able to handle prefixes, and we
may want to extend the same logic for the directory list, as well..

After applying the change, sendDir excludes some files in the the
following steps.

1. Excludes "." and ".." quietly.

2. Excludes files starting with "pgsql_tmp", quietly.

3. (new) Excludes files begins with '.', quietly.

(checks signal)

4. Excludes everything suggested by excludeFiles then reports the
excluded files using DEBUG1.

5. Excludes files based on more complex conditions.

These steps seem a bit arbitrary. Is there any reason we keep step 1
seprate from step 3, and step 2 seprate from step 4?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 21 Apr 2023, at 00:33, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Apr 20, 2023 at 01:15:40PM +0200, Daniel Gustafsson wrote:

Maybe. I'm a bit hesitant to add too many smarts to pg_rewind. It's a tool
for when something has gone wrong with a cluster (albeit probably not at the
filesystem level), and at that point I feel it's better to put the user fully
in charge. Perhaps I'm overly cautious, curious to hear from others.

Hmm. pg_rewind is mostly a differential block-level backup tool, so
applying the same rules everywhere across the board would be sensible
here. See that exclude_list_item is able to handle prefixes, and we
may want to extend the same logic for the directory list, as well..

By the way, the patch ought to add some tests? For pg_basebackup,
this would be around "These files should not be copied" in
010_pg_basebackup.pl. pg_checksums has also its own checks in
002_actions.pl.

Skipping hidden files in pg_rewind added as well as tests for all three
utilities and mentions of this in the docs. I'll park this in the next
commitfest for now.

--
Daniel Gustafsson

On 21 Apr 2023, at 05:02, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

After applying the change, sendDir excludes some files in the the
following steps.

1. Excludes "." and ".." quietly.

2. Excludes files starting with "pgsql_tmp", quietly.

3. (new) Excludes files begins with '.', quietly.

(checks signal)

4. Excludes everything suggested by excludeFiles then reports the
excluded files using DEBUG1.

5. Excludes files based on more complex conditions.

These steps seem a bit arbitrary. Is there any reason we keep step 1
seprate from step 3,

I'm not sure I follow. While "." and ".." and hidden files share a leading '.'
character they are vastly different and combining them in the same check would
trade lines of code for readability. I'm not sure that's a net win. If you
mean that the checks should be reordered such that hidden files are checked for
before "pgsql_tmp", then that seems equally arbitrary no? There is no reason
to believe that hidden files would be more frequently found than "pgsql_tmp
files. I don't have strong opinions on the order, I'm just not sure either is
better than the other.

and step 2 seprate from step 4?

The commit message for 6ad8ac60262 doesn't explain why pgsql_tmp was left out
of the excludeFiles table, but my guess is that it's either an optimization or
a deliberate choice to not DEBUG1 log skipping temporary files. I didn't go
digging in the archives to find the corresponding thread but there might be
clues to be had there.

--
Daniel Gustafsson

Skipping hidden files in pg_rewind added as well as tests for all three
utilities and mentions of this in the docs. I'll park this in the next
commitfest for now.

thanks for the update!

I gave the v2 patch a quick review:
* it applies and builds cleanly
* the new TAP tests pass with the changes and fail without
* in a real world test on macOS, the issue with the .DS_Store files on macOS is no longer showing with the patch and reproducible without
* the documentation changes apply and are understandable. One thing seems to be missing, thus: in protocol-replication.html the change should be mentioned as well:

Various temporary files and directories created during the operation of the PostgreSQL server, such as any file or directory beginning with pgsql_tmp and temporary relations.

In the attached v3 I propose the following change:

- with <filename>pgsql_tmp</filename> and temporary relations.
+ with <filename>pgsql_tmp</filename>, hidden files and temporary relations.

So beside the documentation detail, +1 from my side.

Best,
Tobias

Various temporary files and directories created during the operation of the PostgreSQL server, such as any file or directory beginning with pgsql_tmp and temporary relations.

On a second read, I realised this is likely not the proper paragraph to stuff it in, as these files are not 'created during the operation of PostgreSQL'.

Attached v4 where I propose to modify the last paragraph instead:

- Files other than regular files and directories, such as symbolic
+ Hidden files and files other than regular files and directories, such as symbolic

Tobias

On Thu, Apr 27, 2023 at 04:22:08PM +0200, Daniel Gustafsson wrote:

I'm not sure I follow. While "." and ".." and hidden files share a leading '.'
character they are vastly different and combining them in the same check would
trade lines of code for readability. I'm not sure that's a net win. If you
mean that the checks should be reordered such that hidden files are checked for
before "pgsql_tmp", then that seems equally arbitrary no? There is no reason
to believe that hidden files would be more frequently found than "pgsql_tmp
files. I don't have strong opinions on the order, I'm just not sure either is
better than the other.

FWIW, I don't have an issue with the order of the checks as presented
in v2.

The commit message for 6ad8ac60262 doesn't explain why pgsql_tmp was left out
of the excludeFiles table, but my guess is that it's either an optimization or
a deliberate choice to not DEBUG1 log skipping temporary files. I didn't go
digging in the archives to find the corresponding thread but there might be
clues to be had there.

FWIW, here is the thread:
/messages/by-id/CAB7nPqSNFm2Lz6jASj1RGvAdod1W8ZmHfvML3M7gDnBQ3x6QMw@mail.gmail.com

I think that you're right, the idea is to avoid the random noise
caused by these temp files and their names. This elog has been useful
for debugging in the past for the fixed entries, at least for me.

While on it, it strikes me that we should have a check on
PG_TEMP_FILES_DIR in basebackup.c's sendDir()? Okay, that's the same
as PG_TEMP_FILE_PREFIX, but pg_checksums and pg_rewind check for
*both* patterns so that feels inconsistent to me. This should not be
in excludeDirContents, because we don't want an empty folder in this
case.
--
Michael

On 28 Apr 2023, at 07:32, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Apr 27, 2023 at 04:22:08PM +0200, Daniel Gustafsson wrote:

The commit message for 6ad8ac60262 doesn't explain why pgsql_tmp was left out
of the excludeFiles table, but my guess is that it's either an optimization or
a deliberate choice to not DEBUG1 log skipping temporary files. I didn't go
digging in the archives to find the corresponding thread but there might be
clues to be had there.

FWIW, here is the thread:
/messages/by-id/CAB7nPqSNFm2Lz6jASj1RGvAdod1W8ZmHfvML3M7gDnBQ3x6QMw@mail.gmail.com

I think that you're right, the idea is to avoid the random noise
caused by these temp files and their names. This elog has been useful
for debugging in the past for the fixed entries, at least for me.

Aha, thanks for the digging!

While on it, it strikes me that we should have a check on
PG_TEMP_FILES_DIR in basebackup.c's sendDir()? Okay, that's the same
as PG_TEMP_FILE_PREFIX, but pg_checksums and pg_rewind check for
*both* patterns so that feels inconsistent to me. This should not be
in excludeDirContents, because we don't want an empty folder in this
case.

That makes sense, even though it's a bit duplicative as it stands in core
today. Given that these *can* be different at some point in the future (or in
a fork), we should check both of course. Attached v5 does that as well as
incorporates a version of the doc change proposed in v4 upthread.

--
Daniel Gustafsson

On Fri, Apr 28, 2023 at 02:01:47PM +0200, Daniel Gustafsson wrote:

That makes sense, even though it's a bit duplicative as it stands in core
today. Given that these *can* be different at some point in the future (or in
a fork), we should check both of course.

Yes, who knows what the future is made of.

Attached v5 does that as well as incorporates a version of the doc
change proposed in v4 upthread.

Looks rather OK on a quick pass.
--
Michael

On 28 Apr 2023, at 14:06, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Apr 28, 2023 at 02:01:47PM +0200, Daniel Gustafsson wrote:

Attached v5 does that as well as incorporates a version of the doc
change proposed in v4 upthread.

Looks rather OK on a quick pass.

Thanks! I've parked it in the July commitfest to keep it from being forgotten
about when the tree re-opens.

--
Daniel Gustafsson

On Fri, Apr 28, 2023 at 02:11:00PM +0200, Daniel Gustafsson wrote:

Thanks! I've parked it in the July commitfest to keep it from being forgotten
about when the tree re-opens.

I'm OK to hold on this patch until v17 opens for business, but isn't
that something we'd better backpatch at some point? The behavior of
the stable branches is kind of annoying, in my opinion.
--
Michael

On 29 Apr 2023, at 01:42, Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Apr 28, 2023 at 02:11:00PM +0200, Daniel Gustafsson wrote:

Thanks! I've parked it in the July commitfest to keep it from being forgotten
about when the tree re-opens.

I'm OK to hold on this patch until v17 opens for business, but isn't
that something we'd better backpatch at some point? The behavior of
the stable branches is kind of annoying, in my opinion.

My thinking is that skipping non-postgres files which really have no business
being there in the first place is a new feature and not a bugfix, but I can see
merit in your argument too. If the RMT and -hackers have other opinions then
I'm not going to object.

--
Daniel Gustafsson