From: Antonin Houska <ah@cybertec.at>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
cc: pgsql-hackers@lists.postgresql.org,
    Robert Haas <robertmhaas@gmail.com>,
    Stephen Frost <sfrost@snowman.net>
Subject: Re: Temporary file access API
In-reply-to: <46aa800a-dad7-5080-ee9f-371cc2797b36@enterprisedb.com>
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>
Comments: In-reply-to Peter Eisentraut <peter.eisentraut@enterprisedb.com>
   message dated "Mon, 04 Jul 2022 20:37:30 +0200."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <85650.1657027465.1@antos.home>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 05 Jul 2022 15:24:25 +0200
Message-ID: <85651.1657027465@antos.home>
Archived-At: 
 <https://www.postgresql.org/message-id/85651.1657027465%40antos.home>
Precedence: bulk

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 a=
ccessed
> >> in a non-trivial way, e.g. lseek() / fseek() or pg_pwrite() / pg_prea=
d() is
> >> called.
> > Rebased patch set is attached here, which applies to the current maste=
r.
> > (A few more opportunities for the new API implemented here.)
> =

> I don't understand what this patch set is supposed to do.  AFAICT, the t=
hread
> 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 th=
ings
> like saving on system calls and excising all OS-level file access API us=
e,
> which I found confusing, and the thread then just became a general TDE-r=
elated
> 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 ju=
st
copy & pasted from other places), 2) to have buffered I/O with configurabl=
e
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 t=
he
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=3DFCErX49bzjhNG+GoctY+=
a+XhNRWCVvDY8U74w@mail.gmail.com

which is more important for this patch than the suggestions about encrypti=
on.

> The patches at hand extend some internal file access APIs and then sprin=
kle
> them around various places in the backend code, but there is no explanat=
ion
> why or how this is better. I don't see any real structural savings one m=
ight
> 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 sub=
tle
> expectations about atomic write behavior, syncing, flushing, error recov=
ery,
> 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 rais=
e
ERROR - this needs to be fixed.

-- =

Antonin Houska
Web: https://www.cybertec-postgresql.com