Feedback-ID: id4a34324:Fastmail
Date: Thu, 12 Jan 2023 18:43:54 -0800
From: Andres Freund <andres@anarazel.de>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Thomas Munro <thomas.munro@gmail.com>, vignesh C <vignesh21@gmail.com>,
	pgsql-hackers@lists.postgresql.org,
	Andrew Dunstan <andrew@dunslane.net>, Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Cygwin cleanup
Message-ID: <20230113024354.3q4aigbf4fyh2igg@awork3.anarazel.de>
References: <20220804033828.GK19644@telsasoft.com>
 <CA+hUKGJ=jTuW=i4Yjfr6RRczepWthBA+jRRLP9pt4jXWeAnOeg@mail.gmail.com>
 <20221021034040.GT16921@telsasoft.com>
 <20221109010437.GK3775@telsasoft.com>
 <CALDaNm1QADWDvb3bUBWq2YGX=bN3jj1hVorjNR9Y07-h0M-a1g@mail.gmail.com>
 <20230103142537.GM1153@telsasoft.com>
 <CA+hUKG+Q6DU4Ov9LrvUyDcF3oHS4KMRVSKmVGaeePq-kOyG9gA@mail.gmail.com>
 <CA+hUKGJ6nGVhsGCECtWQMmPMqaAJUJcdp+cDrBjd9=0Mv8xdTw@mail.gmail.com>
 <CA+hUKGL1kpPwt8DzU7cm_VMOi5mpSKSwCrdJAPDQfr_tg0E=NA@mail.gmail.com>
 <20230112043949.GQ9837@telsasoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230112043949.GQ9837@telsasoft.com>
Archived-At: <https://www.postgresql.org/message-id/20230113024354.3q4aigbf4fyh2igg%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2023-01-11 22:39:49 -0600, Justin Pryzby wrote:
> Thomas raised a good question, which was how the tests were passing when
> SnapBuildSerialize() was raising an error, which is what it would've
> been doing when I used data_sync_retry=no.

Presumably some test not checking for failures in a part of the test.


> So .. why is wal_sync_method being used to control fsync for things
> other than WAL?

Historical raisins, I think. The problem is that macOS lies about fsync, and
one needs special magic to make it behave like a real fsync. Somebody thought
that instead of inventing a separate GUC to control whether the "real" fsync
is used for other subsystems, it'd be better to reuse wal_sync_method.

Note that this isn't the function that is actually used for WAL (that's
issue_xlog_fsync()), and that pg_fsync() only uses the GUC to know whether to
use pg_fsync_writethrough() or pg_fsync_no_writethrough(fd).


> See 6dc7760ac (c. 2005) which added wal_fsync_writethrough, at which
> point (since 9b178555f, c. 2004) wal_sync_method was already being used
> for SLOG.
>
> Now, it's also being used for logical decoding (since b89e1510 and
> 858ec1185, c. 2014) in rewriteheap.c/snapbuild.c.  And pidfiles (since
> ee0e525bf, 2010).  And the control file (8b938d36f7, 2019).  Note that
> data_sync_retry wasn't added until 9ccdd7f66 (c.  2018)
>
> It looks like logical decoding may be the "most wrong" place that
> wal_sync_method is being used, so maybe my change is reasonable to
> consider, and not just a workaround.

I don't follow. What does using fsync_fname() vs fsync_fname_ext() have to do
with pg_fsync() using wal_sync_method? fsync_fname() is just a wrapper around
fsync_fname_ext(). Both end up in pg_fsync().

Are you actually proposing that we don't PANIC after an fsync for the category
of files that you list here, even with data_sync_retry set?


Greetings,

Andres Freund