|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
https://postgr.es/m/20210101045047.GB30966%40momjian.us - 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 (), 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
- 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
- appears to create it's own cluster lock file, using its
own routine for doing so. How does that lock file interact with the
- 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
|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|