Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\))
Subject: Re: Transparent column encryption
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: <7a430d89-233b-5cd7-6892-9fed06695d39@enterprisedb.com>
Date: Sat, 11 Mar 2023 10:08:32 -0800
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <8B88C587-8E7C-47AC-9984-DC55CB80E581@enterprisedb.com>
References: <89157929-c2b6-817b-6025-8e4b2d89d88f@enterprisedb.com>
 <79f08a39-a7da-5157-cef4-378fb60c18f8@enterprisedb.com>
 <258c5064-437e-f41e-7537-5e8c343c33cc@enterprisedb.com>
 <ddb316e8-a978-ee69-1edd-0ee82ecc6d8b@enterprisedb.com>
 <edb5eb1f-5818-5521-537d-20cf7f470742@enterprisedb.com>
 <6bd99fea-3298-854d-d37f-554151342f36@enterprisedb.com>
 <963aa100-7e78-3463-0645-700eaaa325f2@enterprisedb.com>
 <06830254-6d87-86b2-0280-bf2eee7736a5@enterprisedb.com>
 <a86c8d36-e9c5-cc5b-2c50-cc563d818419@enterprisedb.com>
 <75f394fa-f539-1875-079c-c654deceed41@enterprisedb.com>
 <8bcedb49-1496-0755-bd24-1cbabf30ec84@enterprisedb.com>
 <d9dd3634-6154-b0b7-f3a9-0d86dad7b8ff@enterprisedb.com>
 <bdb6433c-2d4e-8ae3-3bda-913f0b53fbb8@enterprisedb.com>
 <CALDaNm393nCjM1GenCem=9kOfZxbSadsTSn48WDVomwPnfnCFw@mail.gmail.com>
 <1034b6f1-ccab-8a22-c843-71104ebedf01@enterprisedb.com>
 <40c43d0d-ae4b-fe34-2667-771b3718384e@enterprisedb.com>
 <47e17f03-6595-6c09-8978-7a1581cad6f2@enterprisedb.com>
 <7a430d89-233b-5cd7-6892-9fed06695d39@enterprisedb.com>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Archived-At: <https://www.postgresql.org/message-id/8B88C587-8E7C-47AC-9984-DC55CB80E581%40enterprisedb.com>
Precedence: bulk


> On Mar 9, 2023, at 11:18 PM, Peter Eisentraut =
<peter.eisentraut@enterprisedb.com> wrote:
>=20
> Here is an updated patch.

Thanks, Peter.  The patch appears to be in pretty good shape, but I have =
a few comments and concerns.

CEKIsVisible() and CMKIsVisible() are obviously copied from =
TSParserIsVisible(), and the code comments weren't fully updated.  =
Specifically, the phrase "hidden by another parser of the same name" =
should be updated to not mention "parser".

Why does get_cmkalg_name() return the string "unspecified" for =
PG_CMK_UNSPECIFIED, but the next function get_cmkalg_jwa_name() returns =
NULL for PG_CMK_UNSPECIFIED?  It seems they would both return NULL, or =
both return "unspecified".  If there's a reason for the divergence, =
could you add a code comment to clarify?  BTW, get_cmkalg_jwa_name() has =
no test coverage.
=20
Looking further at code coverage, the new conditional in =
printsimple_startup() is never tested with =
(MyProcPort->column_encryption_enabled), so the block is never entered.  =
This would seem to be a consequence of backends like walsender not using =
column encryption, which is not terribly surprising, but it got me =
wondering if you had a particular client use case in mind when you added =
this block?

The new function pg_encrypted_in() appears totally untested, but I have =
to wonder if that's because we're not round-trip testing pg_dump with =
column encryption...?  The code coverage in pg_dump looks fairly decent, =
but some column encryption code is not covered.

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company