Re: Transparent column encryption

On 11.03.23 19:08, Mark Dilger wrote:
> 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".

fixed

> 
> 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?
 

Added a comment.

> BTW, get_cmkalg_jwa_name() has no test coverage.

Ok, I'll look into it.

> 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?
 

AFAICT, the relationship between printsimple.c and the replicaton 
protocol is not actually firmly defined anywhere, it just happens that 
they are used together.  So I feel the column encryption mode needs to 
be supported, technically, even if nothing is using it right now.

> 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.
 

I have added test coverage for pg_encrypted_in() (via a COPY round-trip 
test in under src/test/column_encryption), as well as additional 
coverage in pg_dump and some DDL commands.  I didn't find any obvious 
gaps in test coverage elsewhere.