Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Skip .DS_Store files in server side utils

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

    Mark Guertin <markguertin@gmail.com> — 2023-04-19T15:45:51Z

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

This one is pretty self explanatory.  When running pg_basebackup on macOS it should probably be ignoring any .DS_Store files found inside data directories instead of transferring/processing them.

Steps to reproduce:
- on a macOS based pg server open a directory containing pg data files and set any kind of view options or move an icon
- this can also happen with inheritance, so even if you do not directly set these kinds of options on data folders, for example if a parent folder has a view option set when a new folder is created it can inherit the same view options, or if spotlight search sets up some metadata this .DS_Store may also be created

My workaround was to manually delete all of the .DS_Store files that the OS created and then immediately run pb_basebackup using the following method:

find <dir> -iname .DS_Store  -exec rm {} \;

Some background on .DS_Store files:

https://en.wikipedia.org/wiki/.DS_Store

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

    Tobias Bussmann <t.bussmann@gmx.net> — 2023-04-19T17:51:55Z

    > 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]: <https://www.postgresql.org/message-id/flat/20181021134206.GA14282%40paquier.xyz>

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-19T18:15:51Z

    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]: <https://www.postgresql.org/message-id/flat/20181021134206.GA14282%40paquier.xyz>

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

			regards, tom lane

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

    Michael Paquier <michael@paquier.xyz> — 2023-04-20T00:15:41Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-04-20T07:34:12Z

    > 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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-04-20T08:50:27Z

    > 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

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

    Tobias Bussmann <t.bussmann@gmx.net> — 2023-04-20T09:51:59Z

    > 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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-04-20T11:15:40Z

    > 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

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

    Michael Paquier <michael@paquier.xyz> — 2023-04-20T22:33:09Z

    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

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

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-04-21T03:02:20Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-04-27T09:08:00Z

    > 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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-04-27T14:22:08Z

    > 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

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

    Tobias Bussmann <t.bussmann@gmx.net> — 2023-04-27T15:34:59Z

    > 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

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

    Tobias Bussmann <t.bussmann@gmx.net> — 2023-04-27T15:48:26Z

    > 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

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

    Michael Paquier <michael@paquier.xyz> — 2023-04-28T05:32:52Z

    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:
https://www.postgresql.org/message-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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-04-28T12:01:47Z

    > 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:
> https://www.postgresql.org/message-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

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

    Michael Paquier <michael@paquier.xyz> — 2023-04-28T12:06:45Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-04-28T12:11:00Z

    > 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

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

    Michael Paquier <michael@paquier.xyz> — 2023-04-28T23:42:31Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-04-29T20:35:52Z

    > 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

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

    Michael Paquier <michael@paquier.xyz> — 2023-05-01T05:25:50Z

    On Sat, Apr 29, 2023 at 10:35:52PM +0200, Daniel Gustafsson wrote:
> 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.

I am fine to hear more opinions, then.  .DS_Store is created in a
directory when browsed by Finder, from what I can read, and there's
been few complaints across the years about that.
--
Michael

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

    Tobias Bussmann <t.bussmann@gmx.net> — 2023-05-01T21:23:58Z

    >> [...] is a new feature and not a bugfix [...] other opinions [...]
> I am fine to hear more opinions, then.

IMHO, while generally ignoring hidden files can be seen as a change in behaviour and a new feature, specifically skipping the '.DS_Store' files is clearly a bugfix that should be backported. With checksumming enabled, pg_basebackup is simply broken on macOS, depending on whether or not you have ever browsed parts of the data directory in the Finder.

The safe solution for the back branches would be to add '{".DS_Store", false}' to the exclude_list_item arrays in backend/backup/basebackup.c, bin/pg_checksums/pg_checksums.c and bin/pg_rewind/filemap.c, but I can't judge whether this would justify using a different codepath for the older versions or not.

--
Tobias Bussmann

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-05-02T10:34:59Z

    > On 1 May 2023, at 23:23, Tobias Bussmann <t.bussmann@gmx.net> wrote:

> The safe solution for the back branches would be to add '{".DS_Store", false}' to the exclude_list_item arrays in backend/backup/basebackup.c, bin/pg_checksums/pg_checksums.c and bin/pg_rewind/filemap.c, but I can't judge whether this would justify using a different codepath for the older versions or not.

If we deem this to be a bugfix then we should backpatch the solution from HEAD
imho.  So far there are mainly voices for considering it a bugfix, but given
where we are in the cycle I the RMT has the final say on that.

--
Daniel Gustafsson

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

    Jacob Champion <jchampion@timescale.com> — 2023-07-06T21:14:59Z

    On Tue, May 2, 2023 at 3:35 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> If we deem this to be a bugfix then we should backpatch the solution from HEAD
> imho.  So far there are mainly voices for considering it a bugfix, but given
> where we are in the cycle I the RMT has the final say on that.

(Was there an RMT decision?)

My two cents: backpatching an exception for .DS_Store seems good, but
creating an exclusion for all hidden files feels like a surprising
change for stable branches. If your postgresql.conf has something like

    include '.my.conf'

then a basebackup of that setup seems to work okay today.

--Jacob

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-07-06T23:05:20Z

    Jacob Champion <jchampion@timescale.com> writes:
> My two cents: backpatching an exception for .DS_Store seems good, but
> creating an exclusion for all hidden files feels like a surprising
> change for stable branches.

On the whole, I think I'd vote for blocking .DS_Store only, even
in HEAD.  (IIRC, I thought differently to start with, but today
I'm feeling conservative about it.)  If there are other special
file names on other platforms, we could add some more targeted
exceptions; but dropping all hidden files seems more likely to
break things than be helpful.

			regards, tom lane

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-07-07T07:23:38Z

    > On 7 Jul 2023, at 01:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Jacob Champion <jchampion@timescale.com> writes:
>> My two cents: backpatching an exception for .DS_Store seems good, but
>> creating an exclusion for all hidden files feels like a surprising
>> change for stable branches.
> 
> On the whole, I think I'd vote for blocking .DS_Store only, even
> in HEAD.  (IIRC, I thought differently to start with, but today
> I'm feeling conservative about it.)  If there are other special
> file names on other platforms, we could add some more targeted
> exceptions; but dropping all hidden files seems more likely to
> break things than be helpful.

I think the case for skipping all hidden files is that it would offer more
consistency with other serverside filesystem reads are performed.  After
.DS_Store I would think that editor swapfiles would be other likely culprit of
hidden-but-not-belonging files.

--
Daniel Gustafsson

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

    Michael Paquier <michael@paquier.xyz> — 2023-07-08T00:05:26Z

    On Fri, Jul 07, 2023 at 09:23:38AM +0200, Daniel Gustafsson wrote:
> On 7 Jul 2023, at 01:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> On the whole, I think I'd vote for blocking .DS_Store only, even
>> in HEAD.  (IIRC, I thought differently to start with, but today
>> I'm feeling conservative about it.)  If there are other special
>> file names on other platforms, we could add some more targeted
>> exceptions; but dropping all hidden files seems more likely to
>> break things than be helpful.
> 
> I think the case for skipping all hidden files is that it would offer more
> consistency with other serverside filesystem reads are performed.  After
> .DS_Store I would think that editor swapfiles would be other likely culprit of
> hidden-but-not-belonging files.

.DS_Store is not the only hidden file pattern that could be used by a
filesystem for its metadata.  And I don't quite see what we gain by
only ignoring it, letting the others be.  My take would be to just
ignore all of them, and I'm OK even if it means to do so only on
HEAD per the argument of being careful with stable branches.

I cannot get excited about the potential use case where someone
decides that it would be a good idea to include a hidden file, as
well.  It's not like it offers any kind of additional protection.
Actually, it can make the debugging of a configuration a worse
experience.
--
Michael

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-07-14T13:07:08Z

    > On 8 Jul 2023, at 02:05, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Fri, Jul 07, 2023 at 09:23:38AM +0200, Daniel Gustafsson wrote:
>> On 7 Jul 2023, at 01:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> On the whole, I think I'd vote for blocking .DS_Store only, even
>>> in HEAD.  (IIRC, I thought differently to start with, but today
>>> I'm feeling conservative about it.)  If there are other special
>>> file names on other platforms, we could add some more targeted
>>> exceptions; but dropping all hidden files seems more likely to
>>> break things than be helpful.
>> 
>> I think the case for skipping all hidden files is that it would offer more
>> consistency with other serverside filesystem reads are performed.  After
>> .DS_Store I would think that editor swapfiles would be other likely culprit of
>> hidden-but-not-belonging files.
> 
> .DS_Store is not the only hidden file pattern that could be used by a
> filesystem for its metadata.  And I don't quite see what we gain by
> only ignoring it, letting the others be.  My take would be to just
> ignore all of them, and I'm OK even if it means to do so only on
> HEAD per the argument of being careful with stable branches.

Judging by the thread there seems to be concensus on skipping .DS_Store files
in all branches, with a few +1's for skipping all hidden files in HEAD.  I'll
prepare a patch for the former and we can pick up the discussion on the latter
ones that's done.

--
Daniel Gustafsson

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

    Michael Paquier <michael@paquier.xyz> — 2023-07-17T08:29:18Z

    On Fri, Jul 14, 2023 at 03:07:08PM +0200, Daniel Gustafsson wrote:
> Judging by the thread there seems to be concensus on skipping .DS_Store files
> in all branches, with a few +1's for skipping all hidden files in HEAD.  I'll
> prepare a patch for the former and we can pick up the discussion on the latter
> ones that's done.

Yes, I am also getting that this is the consensus reached here.
--
Michael

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-07-19T23:16:45Z

    > On 17 Jul 2023, at 10:29, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Fri, Jul 14, 2023 at 03:07:08PM +0200, Daniel Gustafsson wrote:
>> Judging by the thread there seems to be concensus on skipping .DS_Store files
>> in all branches, with a few +1's for skipping all hidden files in HEAD.  I'll
>> prepare a patch for the former and we can pick up the discussion on the latter
>> ones that's done.
> 
> Yes, I am also getting that this is the consensus reached here.

Attached is a version that just skips .DS_Store files as per the discussion
here.  I opted for not testing this on macOS since I don't want to create
invalid .DS_Store files where they potentially can be seen by the Finder
causing unwanted side effects.

--
Daniel Gustafsson

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

    Michael Paquier <michael@paquier.xyz> — 2023-07-20T07:47:40Z

    On Thu, Jul 20, 2023 at 01:16:45AM +0200, Daniel Gustafsson wrote:
> Attached is a version that just skips .DS_Store files as per the discussion
> here.  I opted for not testing this on macOS since I don't want to create
> invalid .DS_Store files where they potentially can be seen by the Finder
> causing unwanted side effects.


@@ -398,6 +398,10 @@ recurse_dir(const char *datadir, const char *parentpath,
            strcmp(xlde->d_name, "..") == 0)
            continue;
 
+       /* Skip macOS system files */
+       if (strcmp(xlde->d_name, ".DS_Store") == 0)
+           continue;

Hmm.  Are you sure that this is correct for pg_rewind?  This should
work when using a local source with --source-pgdata, but under a
remote source with --source-pgdata, I get the impression that we'd
still copy a .DS_store into the target's data folder.

For example, see this little tweak in 003_extrafiles.pl that adds a
.DS_Store in the standby:
--- a/src/bin/pg_rewind/t/003_extrafiles.pl
+++ b/src/bin/pg_rewind/t/003_extrafiles.pl
@@ -53,6 +53,9 @@ sub run_test
    append_to_file
      "$test_standby_datadir/tst_standby_dir/standby_subdir/standby_file4",
      "in standby4";
+   append_to_file
+     "$test_standby_datadir/tst_standby_dir/.DS_Store",
+     "in standby4";

Note that I have not updated the list associated to is_deeply(), to
check that this file is not copied over to the primary.  This leads me
to a failure of the test for the remote case.  Then, looking from
src/bin/pg_rewind/:
$ find . -name ".DS_Store"
./tmp_check/t_003_extrafiles_primary_remote_data/pgdata/tst_standby_dir/.DS_Store
./tmp_check/t_003_extrafiles_standby_local_data/pgdata/tst_standby_dir/.DS_Store
./tmp_check/t_003_extrafiles_standby_remote_data/pgdata/tst_standby_dir/.DS_Store
--
Michael

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-07-20T10:36:52Z

    > On 20 Jul 2023, at 09:47, Michael Paquier <michael@paquier.xyz> wrote:

> Hmm.  Are you sure that this is correct for pg_rewind?  This should
> work when using a local source with --source-pgdata, but under a
> remote source with --source-pgdata, I get the impression that we'd
> still copy a .DS_store into the target's data folder.

Ah, good point.  The updated patch should work for both by moving the check
into the filemap.

--
Daniel Gustafsson

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

    Michael Paquier <michael@paquier.xyz> — 2023-07-20T23:53:02Z

    On Thu, Jul 20, 2023 at 12:36:52PM +0200, Daniel Gustafsson wrote:
> On 20 Jul 2023, at 09:47, Michael Paquier <michael@paquier.xyz> wrote:
>> Hmm.  Are you sure that this is correct for pg_rewind?  This should
>> work when using a local source with --source-pgdata, but under a
>> remote source with --source-pgdata, I get the impression that we'd
>> still copy a .DS_store into the target's data folder.
> 
> Ah, good point.  The updated patch should work for both by moving the check
> into the filemap.

Sounds OK to me to do that in the filemap.  The checksum and base
backup parts look OK as well.  Thanks for the quick update!
--
Michael

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-01-17T14:37:53Z

    > On 21 Jul 2023, at 01:53, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Thu, Jul 20, 2023 at 12:36:52PM +0200, Daniel Gustafsson wrote:
>> On 20 Jul 2023, at 09:47, Michael Paquier <michael@paquier.xyz> wrote:
>>> Hmm.  Are you sure that this is correct for pg_rewind?  This should
>>> work when using a local source with --source-pgdata, but under a
>>> remote source with --source-pgdata, I get the impression that we'd
>>> still copy a .DS_store into the target's data folder.
>> 
>> Ah, good point.  The updated patch should work for both by moving the check
>> into the filemap.
> 
> Sounds OK to me to do that in the filemap.  The checksum and base
> backup parts look OK as well.  Thanks for the quick update!

Reviving an old thread which had fallen off the end of the TODO.  The attached
is a rebase over nearby changes causing conflicts but no functional changes
from the previous which was marked ready for committer, so will go ahead with
this once it's been through a CI cycle in the cfbot.

--
Daniel Gustafsson