> On Mar 9, 2023, at 11:18 PM, Peter Eisentraut
> <peter.eisentr...@enterprisedb.com> wrote:
>
> 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.
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.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
