From: Antonin Houska <ah@cybertec.at>
To: pgsql-hackers@lists.postgresql.org
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Stephen Frost <sfrost@snowman.net>
Subject: Re: Temporary file access API
Date: Fri, 29 Jul 2022 17:37:33 +0200
Message-ID: <56024.1659109053@antos.home> (raw)
In-Reply-To: <85651.1657027465@antos.home>
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>
<46aa800a-dad7-5080-ee9f-371cc2797b36@enterprisedb.com>
<85651.1657027465@antos.home>
Antonin Houska <ah@cybertec.at> wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> > 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.
>
> Yes, it's an attempt to make the encryption less invasive, but there are a few
> other objectives, at least: 1) to make the read / write operations "less
> low-level" (there are common things like error handling which are often just
> copy & pasted from other places), 2) to have buffered I/O with configurable
> buffer size (the current patch version still has fixed buffer size though)
>
> It's true that the discussion ends up to be encryption-specific, however the
> scope of the patch is broader. The first meassage of the thread references a
> related discussion
>
> https://www.postgresql.org/message-id/CA+TgmoYGjN_f=FCErX49bzjhNG+GoctY+a+XhNRWCVvDY8U74w@mail.gmail...
>
> which is more important for this patch than the suggestions about encryption.
>
> > 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.
>
> At this stage I expected feedback from the developers who have already
> contributed to the discussion, because I'm not sure myself if this version
> fits their ideas - that's why I didn't elaborate the documentation yet. I'll
> try to summarize my understanding in the next version, but I'd appreciate if I
> got some feedback for the current version first.
>
> > 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.
>
> I considered that, but could have messed up at some places. Right now I'm
> aware of one problem: pgstat.c does not expect the file access API to raise
> ERROR - this needs to be fixed.
Attached is a new version. It allows the user to set "elevel" (i.e. ERROR is
not necessarily thrown on I/O failure, if the user prefers to check the number
of bytes read/written) and to specify the buffer size. Also, 0015 adds a
function to copy data from one file descriptor to another.
--
Antonin Houska
Web: https://www.cybertec-postgresql.comAttachments:
temp_file_api_v4.tgz (application/gzip, 33.2K) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ah@cybertec.at, pgsql-hackers@lists.postgresql.org, peter.eisentraut@enterprisedb.com, robertmhaas@gmail.com, sfrost@snowman.net
Subject: Re: Temporary file access API
In-Reply-To: <56024.1659109053@antos.home>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox