Re: post-freeze damage control

[Tom Kincaid]

>
> > Yeah, that's an excellent practive, but is why I'm less worried for
> > this feature.  The docs at [1] caution about "not to remove earlier
> > backups if they might be needed when restoring later incremental
> > backups".  Like Alvaro said, should we insist a bit more about the WAL
> > retention part in this section of the docs, down to the last full
> > backup?
>
> I think that would make sense in general. But if we are doing it because
> we lack confidence in the incremental backup feature maybe that's a sign
> that the feature should be released as experimental (or not released at
> all).
>
>
The extensive Beta process we have can be used to build confidence we need
in a feature that has extensive review and currently has no known issues or
outstanding objections.



> Regards,
> -David
>
>
>

-- 
Thomas John Kincaid

Re: Key management with tests

[Tom Kincaid]

Thanks Stephen, Bruce and Masahiko,


> > discussions so far and the point behind the design so that everyone
> > can understand why this feature is designed in that way. To do that,
> > it might be a good start to sort the wiki page since it has data
> > encryption part, KMS, and ToDo mixed.
>
> I hope it's pretty clear that I'm also very much in support of both this
> effort with the KMS and of TDE in general- TDE is specifically,
> repeatedly, called out as a capability whose lack is blocking PG from
> being able to be used for certain use-cases that it would otherwise be
> well suited for, and that's really unfortunate.
>

It is clear you are supportive.

As you know, I share your point of view that PG adoption is suffering for
certain use cases because it does not have TDE.

I appreciate the recent discussion and reviews of the KMS in particular,
> and of the patches which have been sent enabling TDE based on the KMS
> patches.  Having them be relatively independent seems to be an ongoing
> concern and perhaps we should figure out a way to more clearly put them
> together.  That is- the KMS patches have been posted on one thread, and
> TDE PoC patches which use the KMS patches have been on another thread,
> leading some to not realize that there's already been TDE PoC work done
> based on the KMS patches.  Seems like it might make sense to get one
> patch set which goes all the way from the KMS and includes the TDE PoC,
> even if they don't all go in at once.
>

Sounds good, thanks Masahiko, let's see if we can get consensus on the
approach for moving this forward see below.


>
> together, as a few on this thread have voiced, but there's no doubt that
> this is a large project and it's hard to see how we could possibly
> commit all of it at once.
>

I propose that we meet to discuss what approach we want to use to move TDE
forward.  We then start a new thread with a proposal on the approach
and finalize it via community consensus. I will invite Bruce, Stephen and
Masahiko to this meeting. If anybody else would like to participate in this
discussion and subsequently in the effort to get TDE in PG1x, please let me
know. Assuming Bruce, Stephen and Masahiko are down for this, I (or a
volunteer from this meeting) will post the proposal for how we move this
patch forward in another thread. Hopefully, we can get consensus on that
and subsequently restart the execution of delivering this feature.





> Thanks!
>
> Stephen
>


-- 
Thomas John Kincaid

Re: Key management with tests

[Tom Kincaid]

Hello,


> > I don't think it makes sense to think about committing this to v14. I
> > believe it only makes sense if we have a TDE patch that is relatively
> > close to committable that can be used with it. I also don't think that
> > this patch is in good enough shape to commit yet in terms of where
> > it's at in terms of quality; I think it needs more review first,
> > hopefully including review from people who can comment intelligently
> > specifically on the cryptography aspects of it. However, the
> > challenges don't seem insurmountable. There's also still some question
> > in my mind about whether the design choices here (one KEK, 2 DEKs, one
> > for data and one for WAL) have enough consensus. I don't have a
> > considered opinion on that, partly because I'm not quite sure what the
> > reasonable alternatives are, but it seems that other people had some
> > questions about it, IIUC.
>
> While I am willing to make requested adjustments to the patch, I don't
> plan to work on this feaure any further, assuming your analysis above is
> correct.  If after years we are still not sure this is the right
> direction, I don't see any point in moving forward with the later
> pieces, which are even more complicated.  I will join the group of
> people that feel there will never be consensus on implementing this
> feature in the community, so it is not worth trying.
>
> I would also like to add a "not wanted" entry for this feature on the
> TODO list, baaed on the feature's limited usefulness, but I already
> asked about that and no one seems to feel we don't want it.
>

I want to avoid seeing this happen. As a result of a lot of customer and
user discussions, around their criteria for choosing a database, I believe
TDE is an important feature and having it appear with a "not-wanted" tag
will keep the version of PostgreSQL released by the community out of
certain (and possibly growing) number of deployment scenarios which I don't
think anybody wants to see.

I think the current situation to be as follows (if I missed something
please let me know):

1) We need to get the current patch for Key Management reviewed and tested
further.

I spoke to Bruce just now he will see if can get somebody to do this.


2) We need to start working on the actual TDE implementation and get it
pretty close to final before we start committing smaller portions of the
feature.

Unfortunately, on this front, the only things, I think I can offer are:

a) Ask for volunteers to work on the TDE implementation.
b) Facilitate the work between volunteers.
c) Prod folks along and cheer as we go.

So I will start with (a), do we have any volunteers who feel they can
contribute regularly for a while and would like to be part of a team that
moves this forward?



I now better understand why the OpenSSL project has had such serious
> problems in the past.
>
> Updated patch attached as seven attachments.
>
> --
>   Bruce Momjian  https://momjian.us
>   EDB  https://enterprisedb.com
>
>   The usefulness of a cup is in its emptiness, Bruce Lee
>
>

-- 
Thomas John Kincaid

Re: Key management with tests

[Tom Kincaid]

 I met with Bruce and Stephen this afternoon to discuss the feedback
we received so far (prior to Robert's note which I haven't fully
digested yet)
on this patch.

Here is what we plan to do:

1) Bruce is going to gather all the details from the Wiki and build a
README for the TDE Key Management patch. In addition, it will include
details about the implementation, the data structures involved and the
locks that are taken and general technical implementation approach.

2) Stephen is going to write up the overall design of TDE.

Between these two patches, we hope to cover what Andres is asking for
and what Robert is asking for in his reply on this thread which I
haven't fully digested yet.


Stephen's documentation patch will also make reference to Neil Chen's
TDE prototype for making use of this Key Management patch to encrypt
and
decrypt heap pages as well as index pages.

https://www.postgresql.org/message-id/CAA3qoJ=qto5jcsbjqfdbt9ikux9xkmc5bxcrd7ryse+xsme...@mail.gmail.com

3) Tom will work to find somebody who will sign up as a reviewer upon
the next submission of this patch. (Somebody who is not an author).

Could we get feedback if this feels like enough to get this patch
(which will include just the Key Management portion of TDE) to a state
where it can be reviewed and assuming the review issues are resolved
with consensus be committed?

On Mon, Jan 18, 2021 at 2:00 PM Andres Freund  wrote:
>
> On 2021-01-18 13:58:20 -0500, Bruce Momjian wrote:
> > On Mon, Jan 18, 2021 at 09:42:54AM -0800, Andres Freund wrote:
> > > Personally, but I admit that there's legitimate reasons to differ on
> > > that note, I don't think it's reasonable for a feature this invasive to
> > > commit preliminary patches without the major subsequent patches being in
> > > a shape that allows reviewing the whole picture.
> >
> > OK, if that is a requirement, I can't help anymore since there are
> > already complaints that the patch is too large to review, even if broken
> > into pieces.  Please let me know what the community decides.
>
> Those aren't conflicting demands. Having later patches around to
> validate the design of earlier patches doesn't necessitates that the
> later patches need to be reviewed at the same time.



-- 
Thomas John Kincaid

Re: Key management with tests

[Tom Kincaid]

> > I have to admit I was kind of baffled that the wiki page wasn't
> > sufficient, because it is one of the longest Postgres feature
> > explanations I have seen, but I now think the missing part is tying
> > the wiki contents to the code implementation.  If that is it, please
> > confirm.  If it is something else, also explain.
>
> I don't think the wiki right now covers what's needed. The "Overview",
> "Threat model" and "Scope of TDE" are a start, but beyond that it's
> missing a bunch of things. And it's not in the source tree (we'll soon
> have multiple versions of postgres with increasing levels of TDE
> features, the wiki doesn't help with that)
>

Thanks, the versioning issue makes sense for the design document
needing to be part of the the source tree.


As I was reading the README for the patch Amit referenced and as I am
going through this patch, I feel the desire to incorporate diagrams.
Are design diagrams ever incorporated in the source tree as a part of
the design description of a feature? If not, any concerns about doing
that? I think that is likely where I can contribute the most.


> Missing:
> - talks about cluster wide encyrption being simpler, without mentioning
>   what it's being compared to, and what makes it simpler
> - no differentiation from file system / block level encryption
> - there's no explanation of which/why specific crypto primitives were
>   chosen, what the tradeoffs are
> - no explanation which keys exists, stored where
> - the key management patch introduces new files, not documented
> - there's new types of lock files, possibility of interrupted
>   operations, ... - no documentation of what that means
> - there's no documentation what "key wrapping" actually precisely is,
>   what the danger of the two-tier model is, ...
> - are there dangers in not encrypting zero pages etc?
> - ...
>

Some of the missing things you mention above are about the design of
TDE  feature in general. However, this patch is about Key Management
which is going part of the larger TDE feature. So it feels as though
there is the need for a general design document about the overall
vision / approach for TDE and a specific design doc. for Key
Management. Is it appropriate to include both of those in the same
patch?

Something along the lines here is the overall design of TDE and here
is how the Key Management portion is designed and implemented. I guess
in that case, follow on patches for TDE could refer to the overall
design described in this patch.




>
>
> Personally, but I admit that there's legitimate reasons to differ on
> that note, I don't think it's reasonable for a feature this invasive to
> commit preliminary patches without the major subsequent patches being in
> a shape that allows reviewing the whole picture.
>
> Greetings,
>
> Andres Freund



-- 
Thomas John Kincaid

Re: Key management with tests

[Tom Kincaid]

> > > I think that's not at all acceptable. I don't mind hashing out details
> > > on calls / off-list, but the design needs to be public, documented, and
> > > reviewable.  And if it's something the community can't understand, then
> > > it can't get in. We're going to have to maintain this going forward.
> >
> > OK, so we don't want it.  That's fine with me.
>
> That's not what I said...
>


 I think the majority of us believe that it is important we take this
first step towards a solid TDE implementation in PostgreSQL that is
built around the community processes which involves general consensus.

Before this feature falls into the “we will never do it because we
will never build consensus" category and community PostgreSQL
potentially gets locked out of more deployment scenarios that require
this feature I would like to see if I can help with this current
attempt at it. I will share that I am concerned that if the people who
have been involved in this to date can’t get this in, it will never
happen.

Admittedly I am a novice on this topic, and the majority of the
PostgreSQL source code, however I am hopeful enough (those of you who
know me understand that I suffer from eternal optimism) that I am
going to attempt to help.

Is there a design document for a Postgres feature of this size and
scope that people feel would serve as a good example? Alternatively,
is there a design document template that has been successfully used in
the past? I could guess based on things I have observed reading this
list for many years. However, if there is something that those who are
deeply involved in the development effort feel would suffice as an
example of a "good design document" or a "good design template"
sharing it would be greatly appreciated.

Re: pgsql: New files for MERGE

[Tom Kincaid]

On Thu, Apr 5, 2018 at 10:54 PM, Michael Paquier  wrote:
> On Thu, Apr 05, 2018 at 04:02:20PM -0400, Bruce Momjian wrote:
>> Simon, you have three committers in this thread suggesting this patch be
>> reverted.  Are you just going to barrel ahead with the fixes without
>> addressing their emails?
>
> If my opinion counts, please count me in this bucket as well.  I have
> seen also Peter G. commenting about the design of the patch in a very
> advanced way and emit doubts, this is enough to convince me that
> something wrong is going on here.  I have to admit that I did not look
> at the patch in details but the design issues for the executor and
> parser mentioned show that some low-level considerations have not been
> taken into account, so this is worrying.

Apologies for butting in here as it is not my place. Just a rather
timid introduction, I have met most of you and am a huge fan all of
you. I have been reading hackers for many years. I will follow up with
a response in a vein similar to Michael's,  if my opinion counts,
which it probably does not, since this is my first post to hackers
ever:

 I have read the thread from the start and I don't think this is a
fair characterization of Peter's feedback. I would say initially yes
that would be a fair statement. However, in past 4-6 weeks I
interpreted his feedback as supportive. FWIW, I haven't read the patch
either and it would be of little value if I did :-).

Pavan did respond to all Peter's issues and implement all Peter's
requested changes and at one point spent a lot of time looking at and
reporting back on how another database handle certain situations with
MERGE so he could incorporate the proper behavior into Postgres and
properly respond to Peter's concerns. The community at large
requirements that MERGE support RLS and Partitioning were implemented
and Steven Frost reviewed the RLS implementation. The sqlsmith team
did extensive testing of the patch.

So given all this, I am not sure why people feel this patch was rushed
through or has a flawed design.  The comments from Andres while I am
sure they have merit came before the commit but technically after the
time when Simon said he was going to commit the patch (which he gave
with 5 days notice). The patch was developed and reviewed in the
community for many months. Pavan and Simon continue to respond on
these comments and implementing changes people are requesting.







> --
> Michael



-- 
Thomas John Kincaid