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

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-trip testing pg_dump with column encryption...? The code coverage in pg_dump looks fairly decent, but some column encryption code 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.