Date: Fri, 1 Sep 2023 11:08:51 -0700
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
Subject: Re: should frontend tools use syncfs() ?
Message-ID: <20230901180851.GC3180181@nathanxps13>
References: <20210930004340.GM831@telsasoft.com>
 <YVUz0Ay9KshniWBe@paquier.xyz>
 <20220413115412.GM26620@telsasoft.com>
 <20230729214010.GA189456@nathanxps13>
 <ZPImMi8smaPsXjGK@pryzbyj2023>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZPImMi8smaPsXjGK@pryzbyj2023>
Archived-At: 
 <https://www.postgresql.org/message-id/20230901180851.GC3180181%40nathanxps13>
Precedence: bulk

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