Feedback-ID: i131946ab:Fastmail
Message-ID: <46aa800a-dad7-5080-ee9f-371cc2797b36@enterprisedb.com>
Date: Mon, 4 Jul 2022 20:37:30 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.11.0
Subject: Re: Temporary file access API
Content-Language: en-US
To: Antonin Houska <ah@cybertec.at>, pgsql-hackers@lists.postgresql.org
Cc: Robert Haas <robertmhaas@gmail.com>, Stephen Frost <sfrost@snowman.net>
References: <4987.1644323098@antos>
 <20220301143431.GT10577@tamriel.snowman.net> <27013.1646738099@antos>
 <CA+TgmobnojZw6mZi0T_61RtUDgsxroYhp+DJU5Yhz6caxUrQgw@mail.gmail.com>
 <3146.1649408068@antos>
 <CA+TgmobQM4Yym3944tFpcaJX4EfYs92Vv4NCY3V0R5BE4RU2rg@mail.gmail.com>
 <2132.1649664364@antos>
 <CA+TgmoaJ74opLkguCmm+Du9T1mAPtPYThCQhF_bbhZW5i4VAkg@mail.gmail.com>
 <3690.1649755839@antos>
 <CA+TgmobFVyBNxhsYOteJ-KdvkYyTGU4WUZv=y2VwmmahNyMUsA@mail.gmail.com>
 <24759.1650466826@antos> <60402.1656952546@antos.home>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <60402.1656952546@antos.home>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/46aa800a-dad7-5080-ee9f-371cc2797b36%40enterprisedb.com>
Precedence: bulk

On 04.07.22 18:35, Antonin Houska wrote:
>> Attached is a new version of the patch, to evaluate what the API use in the
>> backend could look like. I haven't touched places where the file is accessed
>> in a non-trivial way, e.g. lseek() / fseek() or pg_pwrite() / pg_pread() is
>> called.
> Rebased patch set is attached here, which applies to the current master.
> 
> (A few more opportunities for the new API implemented here.)

I don't understand what this patch set is supposed to do.  AFAICT, the 
thread originally forked off from a TDE discussion and, considering the 
thread subject, was possibly once an attempt to refactor temporary file 
access to make integrating encryption easier?  The discussion then 
talked about things like saving on system calls and excising all 
OS-level file access API use, which I found confusing, and the thread 
then just became a general TDE-related mini-discussion.

The patches at hand extend some internal file access APIs and then 
sprinkle them around various places in the backend code, but there is no 
explanation why or how this is better.  I don't see any real structural 
savings one might expect from a refactoring patch.  No information has 
been presented how this might help encryption.

I also suspect that changing around the use of various file access APIs 
needs to be carefully evaluated in each individual case.  Various code 
has subtle expectations about atomic write behavior, syncing, flushing, 
error recovery, etc.  I don't know if this has been considered here.