Re: storing an explicit nonce

Hi,

On 2021-05-25 12:46:45 -0400, Robert Haas wrote:
> This approach has a few disadvantages. For example, right now, we only
> need to WAL log hints for the first write to each page after a
> checkpoint, but in this approach, if the same page is written multiple
> times per checkpoint cycle, we'd need to log hints every time. In some
> workloads that could be quite expensive, especially if we log an FPI
> every time.

Yes. I think it'd likely be prohibitively expensive in some situations.

> So I would like to propose an alternative: store the nonce in the
> page. Now the next question is where to put it. I think that putting
> it into the page header would be far too invasive, so I propose that
> we instead store it at the end of the page, as part of the special
> space. That makes an awful lot of code not really notice that anything
> is different, because it always thought that the usable space on the
> page ended where the special space begins, and it doesn't really care
> where that is exactly. The code that knows about the special space
> might care a little bit, but whatever private data it's storing is
> going to be at the beginning of the special space, and the nonce would
> be stored - in this proposal - at the end of the special space. So it
> turns out that it doesn't really care that much either.

The obvious concerns are issues around binary upgrades for cases that
already use the special space? Are you planning to address that by not
having that path? Or by storing the nonce at the "start" of the special
space (i.e. [normal data][nonce][existing special])?

Is there an argument for generalizing the nonce approach for to replace
fake LSNs for unlogged relations?

Why is using pd_special better than finding space for a flag bit in the
header indicating whether it has a nonce? Using pd_special will burden
all code using special space, and maybe even some that does not (think
empty pages now occasionally having a non-zero pd_special), whereas
implementing it on the page level wouldn't quite have the same concerns.

> One thing that happens is that a bunch of values that used to be
> constant - like TOAST_INDEX_TARGET and GinDataPageMaxDataSize - become
> non-constant. I suggested to Bharath that he handle this by changing
> those macros to take the nonce size as an argument, which is what the
> patch does, although it missed pushing that idea down all the way in
> some obscure case (e.g. SIGLEN_MAX). That has the down side that we
> will now have more computation to do at runtime vs. compile-time. I am
> unclear whether there would be enough impact to get exercised about,
> but I'm hopeful that the answer is "no".
>
> As written, the patch makes initdb take a --tde-nonce-size argument,
> but that's really just for demonstration purposes. I assume that, if
> we decide to go this way, we'd have an initdb option that selects
> whether to use encryption, or perhaps the specific encryption
> algorithm to be used, and then the nonce size would be computed based
> on that, or else set to 0 if encryption is not in use.

I do suspect having only the "no nonce" or "nonce is a compile time
constant" cases would be good performance wise. Stuff like

> +#define MaxHeapTupleSizeLimit (BLCKSZ - MAXALIGN(SizeOfPageHeaderData + \
> + sizeof(ItemIdData)))
> +#define MaxHeapTupleSize(tdeNonceSize) (BLCKSZ - MAXALIGN(SizeOfPageHeaderData + \
> + sizeof(ItemIdData)) - MAXALIGN(tdeNonceSize))

won't be free.

Greetings,

Andres Freund