Re: Key management with tests

From: Andres Freund <andres(at)anarazel(dot)de>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>
Subject: Re: Key management with tests
Date: 2021-01-15 22:37:56
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On 2021-01-15 16:47:19 -0500, Bruce Momjian wrote:
> On Fri, Jan 15, 2021 at 04:23:22PM -0500, Robert Haas wrote:
> > On Fri, Jan 15, 2021 at 3:49 PM Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > I don't think that's appropriate. Several prominent community members
> > have told you that the patch, as committed the first time, needed a
> > lot more work. There hasn't been enough time between then and now for
> > you, or anyone, to do that amount of work. This patch needs detailed
> > and substantial review from senior community members, and multiple
> > rounds of feedback and improvement, before it should be considered for
> > commit.
> >
> > I am not even sure there is a consensus on the design, without which
> > any commit is always premature.
> If people want changes, I need to hear about it here. I have address
> everything people have mentioned in these threads so far.

I don't even know how anybody is supposed to realistically review the
design or the patch:

This thread started at - there's no
reference to any discussion of the design at all and the supposed links
to code are dead.

The last version of the code that I see posted ([1]), has the useless
commit message of "key squash commit" - nothing else. There's no design
documentation included in the patch either, as far as I can tell.

Manually searching for the topic brings me to
, a thread of 52 messages, which provides a bit more context, but
largely just references another thread and a wiki article. The link to
the other thread is into the middle of a 112 message thread.

The wiki page doesn't really describe a design either. It has a very
long todo, a bunch of implementation details, but no design.

Nor did 978f869b99 include much in the way of design description.

You cannot expect anybody to review a patch if developing some basic
understanding of the intended design requires reading hundreds of
messages in which the design evolved. And I don't think it's acceptable
to push it due to lack of further feedback, given this situation - the
lack of design description is a blocker in itself.

There's a few things that stand out on a very very brief scan:
- the patch badly needs to be split up into independently reviewable
- tests:
- wait, a .sh test script? No, we shouldn't add any more of those,
they're nightmare across platforms
- Do the tests actually do anything useful? It's not clear to me what
they are trying to achieve. En/Decrypting test vectors doesn't seem to
buy that much?
- the new pg_alterckey is completely untested
- the pg_upgrade paths is untested
- ..
- Without further comment BootStrapKmgr() does "copy cluster file
encryption keys from an old cluster?", but there's no explanation as
to why / when that's the case. Presumably pg_upgrade, but, uh, explain

- pg_alterckey.c
- appears to create it's own cluster lock file, using its
own routine for doing so. How does that lock file interact with the
running server?
- retrieve_cluster_keys() is missing (void).

I think this is at the very least a month away from being committable,
even if the design were completely correct (which I do not know, see


Andres Freund


In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2021-01-15 22:41:16 Re: Change default of checkpoint_completion_target
Previous Message Alvaro Herrera 2021-01-15 22:26:41 Re: Rename of triggers for partitioned tables