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

Hi

On Mon, 24 Oct 2022, 19:56 David Christensen, <david.christensen@crunchydata.com> wrote:
>
> Discussion is welcome and encouraged!

Did you read the related thread with related discussion from last June, "Re: better page-level checksums" [0]? In that I argued that space at the end of a page is already allocated for the AM, and that reserving variable space at the end of the page for non-AM usage is wasting the AM's performance potential.

Apart from that: Is this variable-sized 'metadata' associated with smgr infrastructure only, or is it also available for AM features? If not; then this is a strong -1. The amount of tasks smgr needs to do on a page is generally much less than the amount of tasks an AM needs to do; so in my view the AM has priority in prime page real estate, not smgr or related infrastructure.

re: PageFeatures
I'm not sure I understand the goal, nor the reasoning. Shouldn't this be part of the storage manager (smgr) implementation / can't this be part of the smgr of the relation?

re: use of pd_checksum
I mentioned this in the above-mentioned thread too, in [1], that we could use pd_checksum as an extra area marker for this storage-specific data, which would be located between pd_upper and pd_special.

Re: patch contents

0001:
>+ specialSize = MAXALIGN(specialSize) + reserved_page_size;

This needs to be aligned, so MAXALIGN(specialSize + reserved_page_size), or an assertion that reserved_page_size is MAXALIGNED, would be better.

>  PageValidateSpecialPointer(Page page)
>  {
>      Assert(page);
> -    Assert(((PageHeader) page)->pd_special <= BLCKSZ);
> +    Assert((((PageHeader) page)->pd_special - reserved_page_size) <= BLCKSZ);

This check is incorrect. With your code it would allow pd_special past the end of the block. If you want to put the reserved_space_size effectively inside the special area, this check should instead be:

+    Assert(((PageHeader) page)->pd_special <= (BLCKSZ - reserved_page_size));

Or, equally valid

+    Assert((((PageHeader) page)->pd_special + reserved_page_size) <= BLCKSZ);

> + * +-------------+-----+------------+-----------------+
> + * | ... tuple2 tuple1 | "special space" | "reserved" |
> + * +-------------------+------------+-----------------+

Could you fix the table display if / when you revise the patchset? It seems to me that the corners don't line up with the column borders.

0002:
> Make the output of "select_views" test stable
> Changing the reserved_page_size has resulted in non-stable results for this test.

This makes sense, what kind of instability are we talking about? Are there different results for runs with the same binary, or is this across compilations?

0003 and up were not yet reviewed in depth.


Kind regards,

Matthias van de Meent


[0] https://www.postgresql.org/message-id/flat/CA%2BTgmoaCeQ2b-BVgVfF8go8zFoceDjJq9w4AFQX7u6Acfdn2uA%40mail.gmail.com#90badc63e568a89a76f51fc95f07ffaf
[1] https://postgr.es/m/CAEze2Wi5wYinU7nYxyKe_C0DRc6uWYa8ivn5%3Dzg63nKtHBnn8A%40mail.gmail.com