public inbox for [email protected]  

help / color / mirror / Atom feed

From: Mark Dilger <[email protected]>
To: Peter Eisentraut <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Transparent column encryption
Date: Sat, 11 Mar 2023 10:08:32 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CALDaNm393nCjM1GenCem=9kOfZxbSadsTSn48WDVomwPnfnCFw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>



> On Mar 9, 2023, at 11:18 PM, Peter Eisentraut <[email protected]> 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

view thread (8+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: Transparent column encryption
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox