Re: [PATCHES] Post-special page storage TDE support - Mailing list pgsql-hackers

Hi,

- IMO the patch touches many places it shouldn't need to touch, because of
  essentially renaming a lot of existing macro names to *Limit,
  necessitating modifying a lot of users. I think instead the few places that
  care about the runtime limit should be modified.

  As-is the patch would cause a lot of fallout in extensions that just do
  things like defining an on-stack array of Datums or such - even though all
  they'd need is to change the define to the *Limit one.

  Even leaving extensions aside, it must makes reviewing (and I'm sure
  maintaining) the patch very tedious.

Hi Andres et al,

So I've been looking at alternate approaches to this issue and considering how to reduce churn, and I think we still need the *Limit variants.  Let's take a simple example:

Just looking at MaxHeapTuplesPerPage and breaking down instances in the code, loosely partitioning into whether it's used as an array index or other usage (doesn't discriminate against code vs comments, unfortunately) we get the following breakdown:

$ git grep -hoE [[]?MaxHeapTuplesPerPage | sort | uniq -c
     18 [MaxHeapTuplesPerPage
     51 MaxHeapTuplesPerPage

This would be 18 places where we would need at adjust in a fairly mechanical fashion to add the MaxHeapTuplesPerPageLimit instead of MaxHeapTuplesPerPage vs some significant fraction of non-comment--even if you assumed half were in comments, there would presumably need to be some sort of adjustments in verbage since we are going to be changing some of the interpretation.

I am working on a patch to cleanup some of the assumptions that smgr makes currently about its space usage and how the individual access methods consider it, as they should only be calculating things based on how much space is available after smgr is done with it.  That has traditionally been BLCKSZ - SizeOfPageHeaderData, but this patch (included) factors that out into a single expression that we can now use in access methods, so we can then reserve additional page space and not need to adjust the access methods furter.

Building on top of this patch, we'd define something like this to handle the #defines that need to be dynamic:

extern Size reserved_page_space;
#define PageUsableSpace (BLCKSZ - SizeOfPageHeaderData - reserved_page_space)
#define MaxHeapTuplesPerPage CalcMaxHeapTuplesPerPage(PageUsableSpace)
#define MaxHeapTuplesPerPageLimit CalcMaxHeapTuplesPerPage(BLCKSZ - SizeOfPageHeaderData)
#define CalcMaxHeapTuplesPerPage(freesize)
      ((int) ((freesize) / \
                      (MAXALIGN(SizeofHeapTupleHeader) + sizeof(ItemIdData))))

In my view, extensions that are expecting to need no changes when it comes to changing how these are interpreted are better off needing to only change the static allocation in a mechanical sense than revisit any other uses of code; this seems more likely to guarantee a correct result than if you exceed the page space and start overwriting things you weren't because you're not aware that you need to check for dynamic limits on your own.

Take another thing which would need adjusting for reserving page space, MaxHeapTupleSize:

$ git grep -ohE '[[]?MaxHeapTupleSize' | sort | uniq -c
      3 [MaxHeapTupleSize
     16 MaxHeapTupleSize

Here there are 3 static arrays which would need to be adjusted vs 16 other instances. If we kept MaxHeapTupleSize interpretation the same and didn't adjust an extension it would compile just fine, but with too large of a length compared to the smaller PageUsableSpace, so you could conceivably overwrite into the reserved space depending on what you were doing.

(since by definition the reserved_page_space >= 0, so PageUsableSpace will always be <= BLCKSZ - SizeOfPageHeaderData, so any expression based on it as a basis will be smaller).

In short, I think the approach I took originally actually will reduce errors out-of-core, and while churn is still necessary churn.

I can produce a second patch which implements this calc/limit atop this first one as well.

Thanks,

David