On Tue, May 25, 2021 at 03:09:03PM -0400, Robert Haas wrote: > On Tue, May 25, 2021 at 2:45 PM Bruce Momjian <br...@momjian.us> wrote: > > Well, if we create a separate nonce counter, we still need to make sure > > it doesn't go backwards during a crash, so we have to WAL log it > > I think we don't really need a global counter, do we? We could simply > increment the nonce every time we write the page. If we want to avoid > using the same IV for different pages, then 8 bytes of the nonce could > store a value that's different for every page, and the other 8 bytes > could store a counter. Presumably we won't manage to write the same > page more than 2^64 times, since LSNs are limited to be <2^64, and > those are consumed more than 1 byte at a time for every change to any > page anywhere.
The issue we had here is what do you use as a special value for each relation? Where do you store it if it is not computed? You can use a global counter for the per-page nonce that doesn't change when the page is updated, but that would still need to be a global counter. Also, when you change hint bits, either you don't change the nonce/LSN, and don't recrypt the page (and the hint bit changes are visible), or you change the nonce and reencrypt the page, and you are then WAL logging the page. I don't see how having a nonce different from the LSN helps here. > > The buffer encryption overhead is 2-4%, and WAL encryption is going to > > add to that, so I thought hint bit logging overhead would be minimal > > in comparison. > > I think it depends. If buffer evictions are rare, then it won't matter > much. But if they are common, then using the LSN as the nonce will add > a lot of overhead. Well, see above. A separate nonce somewhere else doesn't help much, as I see it. > > Have you looked at the code, specifically EncryptPage(): > > > > > > https://github.com/postgres/postgres/compare/bmomjian:cfe-11-gist..bmomjian:_cfe-12-rel.patch > > > > + if (!relation_is_permanent && !is_gist_page_or_similar) > > + PageSetLSN(page, LSNForEncryption(relation_is_permanent)); > > > > > > It assigns an LSN to unlogged pages. As far as the buffer manager > > seeing fake LSNs that already happens for GiST indexes, so I just built > > on that --- seemed to work fine. > > I had not, but I don't see why this issue is specific to GiST rather > than common to every kind of unlogged and temporary relation. > > > I have to ask why we should consider adding it to the special space, > > since my current version seems fine, and has minimal code impact, and > > has some advantages over using the special space. Is it because of the > > WAL hint overhead, or for a cleaner API, or something else? > > My concern is about the overhead, and also the code complexity. I > think that making sure that the LSN gets changed in all cases may be > fairly tricky. Please look over the patch to see if I missed anything --- for me, it seemed quite clear, and I am not an expert in that area of the code. -- Bruce Momjian <br...@momjian.us> https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.