Date: Tue, 1 Mar 2022 09:34:31 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Antonin Houska <ah@cybertec.at>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Temporary file access API
Message-ID: <20220301143431.GT10577@tamriel.snowman.net>
References: <4987.1644323098@antos>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="wAPz785vMyyfnE3+"
Content-Disposition: inline
In-Reply-To: <4987.1644323098@antos>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: <https://www.postgresql.org/message-id/20220301143431.GT10577%40tamriel.snowman.net>
Precedence: bulk


--wAPz785vMyyfnE3+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Antonin Houska (ah@cybertec.at) wrote:
> Here I'm starting a new thread to discuss a topic that's related to the
> Transparent Data Encryption (TDE), but could be useful even without that.=
  The
> problem has been addressed somehow in the Cybertec TDE fork, and I can po=
st
> the code here if it helps. However, after reading [1] (and the posts
> upthread), I've got another idea, so let's try to discuss it first.

Yeah, I tend to agree that it makes sense to discuss the general idea of
doing buffered I/O for the temporary files that are being created today
with things like fwrite and have that be independent of encryption.  As
mentioned on that thread, doing our own buffering and having file
accesses done the same way throughout the system is just generally a
good thing and focusing on that will certainly help with any TDE effort
that may or may not happen later.

> It makes sense to me if we first implement the buffering (i.e. writing/re=
ading
> certain amount of data at a time) and make the related functions aware of
> encryption later: as long as we use a block cipher, we also need to read/=
write
> (suitably sized) chunks rather than individual bytes (or arbitrary amount=
s of
> data). (In theory, someone might need encryption but reject buffering, bu=
t I'm
> not sure if this is a realistic use case.)

Agreed.

> For the buffering, I imagine a "file stream" object that user creates on =
the
> top of a file descriptor, such as
>=20
>     FileStream  *FileStreamCreate(File file, int buffer_size)
>=20
>     or
>=20
>     FileStream  *FileStreamCreateFD(int fd, int buffer_size)
>=20
> and uses functions like
>=20
>     int FileStreamWrite(FileStream *stream, char *buffer, int amount)
>=20
>     and
>=20
>     int FileStreamRead(FileStream *stream, char *buffer, int amount)
>=20
> to write and read data respectively.

Yeah, something along these lines was also what I had in mind, in
particular as it would mean fewer changes to places like reorderbuffer.c
(or, at least, very clear changes).

> Besides functions to close the streams explicitly (e.g. FileStreamClose()=
 /
> FileStreamFDClose()), we'd need to ensure automatic closing where that ha=
ppens
> to the file. For example, if OpenTemporaryFile() was used to obtain the f=
ile
> descriptor, the user expects that the file will be closed and deleted on
> transaction boundary, so the corresponding stream should be freed
> automatically as well.

Sure.  We do have some places that want to write using offsets and we'll
want to support that also, I'd think.

> To avoid code duplication, buffile.c should use these streams internally =
as
> well, as it also performs buffering. (Here we'd also need functions to ch=
ange
> reading/writing position.)

Yeah... though we should really go through and make sure that we
understand the use-cases for each of the places that decided to use
their own code rather than what already existed, to the extent that we
can figure that out, and make sure that we're solving those cases and
not writing a bunch of new code that won't end up getting used.  We
really want to be sure that all writes are going through these code
paths and make sure there aren't any reasons for them not to be used.

> Once we implement the encryption, we might need add an argument to the
> FileStreamCreate...() functions that helps to generate an unique IV, but =
the
> ...Read() / ...Write() functions would stay intact. And possibly one more
> argument to specify the kind of cipher, in case we support more than one.

As long as we're only needing to pass these into the Create function,
that seems reasonable to me.  I wouldn't want to go through the effort
of this and then still have to change every single place we read/write
using this system.

Thanks,

Stephen

--wAPz785vMyyfnE3+
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJiHi73AAoJEO1sijiDR2RVjAgP/RWogJ63ExKnaxcx2EP2M1Fb
OY1mmaqIazlLbTZNfvXRKLwkQfBpLOjZZJpPlFMBy08OhNUc6NyWtVRpEthWZur2
/tHXUapVOxnVmNMhZ/KSOBDi4t9nuJyTR3Yl/MB0cJyCR8LzhEAN+ggqiKqfvy51
D/XWV7cg2FVkcn7EGXCRgSgNFRh14wAt5MYKo4C42ndwwvzpXXabKcFYPB1UT1LO
kErHKTGoY39K/HvMArl5tHCr5faq+vwK5QEdGf6q3sCuH0G362w1VYwtX/1f7xBN
9sC0O7EYVnpXo7dRiTX8/PuOOwbXPtm/JHg3UEzw+5W1q55pqv91OCSbO90P6qzG
6EzkiDhvZfO5bQyIPAlD4TnWHxWF3C0/sIdaQ3F7sbmUbROrClwHNKluOGaQdxku
dEHTeqSDDV+aoksStDBKvqXmvyspT+WoZTnKZrULxQqoffn5o4ffZMImg5LDPVil
+jN1U3CSAyxrwDtKSkhpPUj5/adQ8oYFBugVajBwJbEV0fGAqo/WV1WVesNO7qD8
hL0bGgaOh2/9gb74deh/ut/d7MLzcXvajC3fBOTbdqTwU+QmdLK2E6QGF/T1KUeT
pVLbVQGngKE7GJSxD/YfJkJ+YJCmbhriJJB5D/L5i2TLlwyEHSFfXMX/DiIyNykX
K4b7hbF4zDG01/Y1btKz
=sIzV
-----END PGP SIGNATURE-----

--wAPz785vMyyfnE3+--