> On Mar 9, 2023, at 11:18 PM, Peter Eisentraut <peter.eisentraut@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
likewalsender not using column encryption, which is not terribly surprising, but it got me wondering if you had a
particularclient 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-triptesting pg_dump with column encryption...? The code coverage in pg_dump looks fairly decent, but some column
encryptioncode is not covered.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company