Horton

Re: should frontend tools use syncfs() ?

Nathan Bossart <nathandbossart@gmail.com>

View thread View on postgr.es
From: Nathan Bossart <nathandbossart@gmail.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>, Thomas Munro <thomas.munro@gmail.com>, David Steele <david@pgmasters.net>, Fujii Masao <masao.fujii@oss.nttdata.com>, Bruce Momjian <bruce@momjian.us>, Paul Guo <guopa@vmware.com>, Tom Lane <tgl@sss.pgh.pa.us>, Michael Brown <michael.brown@discourse.org>, pgsql-hackers@postgresql.org
Date: 2023-09-01T18:08:51Z
Lists: pgsql-hackers 
Thanks for taking a look.

On Fri, Sep 01, 2023 at 12:58:10PM -0500, Justin Pryzby wrote:
>> +	if (!user_opts.sync_method)
>> +		user_opts.sync_method = pg_strdup("fsync");
> 
> why pstrdup?

I believe I was just following the precedent set by some of the other
options.

>> +parse_sync_method(const char *optarg, SyncMethod *sync_method)
>> +{
>> +	if (strcmp(optarg, "fsync") == 0)
>> +		*sync_method = SYNC_METHOD_FSYNC;
>> +#ifdef HAVE_SYNCFS
>> +	else if (strcmp(optarg, "syncfs") == 0)
>> +		*sync_method = SYNC_METHOD_SYNCFS;
>> +#endif
>> +	else
>> +	{
>> +		pg_log_error("unrecognized sync method: %s", optarg);
>> +		return false;
>> +	}
> 
> This should probably give a distinct error when syncfs is not supported
> than when it's truely recognized.

Later versions of the patch should have this.

> The patch should handle pg_dumpall, too.

It looks like pg_dumpall only ever fsyncs a single file, so I don't think
it is really needed there.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Commits

  1. Adjust documentation for syncfs().

  2. Improve the naming in wal_sync_method code.

  3. Allow using syncfs() in frontend utilities.

  4. Add support for syncfs() in frontend support functions.

  5. Make enum for sync methods available to frontend code.

  6. Move PG_TEMP_FILE* macros to file_utils.h.

  7. Change client-side fsync_fname() to report errors fatally