PG-1213 Add SQL function for checking if WAL record is encrypted #514

artemgavrilov · 2025-08-08T14:55:44Z

https://perconadev.atlassian.net/browse/PG-1213

This PR bumps major version for the extension and introduces two new functions:

pg_tde_is_wal_record_encrypted - checks if record with provided LSN is encrypted. Function by default assumes that given LSN belongs to the current timeline, but allows to specify exact timeline id via optional argument.
pg_tde_get_wal_encryption_ranges - returns WAL encryption ranges. Within provided ranges all records are encrypted. I decided to not expose knowledge about "internal keys" and represent this information just as "encryption ranges". TAP tests for this function doesn't use output files but instead relies on assertion because LSN numbers not stable in different envs/test runs.

contrib/pg_tde/src/pg_tde.c

contrib/pg_tde/pg_tde--1.0--2.0.sql

codecov-commenter · 2025-08-13T19:55:00Z

Codecov Report

❌ Patch coverage is 86.95652% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.15%. Comparing base (6ddc86c) to head (9967248).

❌ Your project status has failed because the head coverage (82.15%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@                  Coverage Diff                  @@
##           TDE_REL_17_STABLE     #514      +/-   ##
=====================================================
+ Coverage              82.08%   82.15%   +0.06%     
=====================================================
  Files                     25       25              
  Lines                   3170     3216      +46     
  Branches                 505      514       +9     
=====================================================
+ Hits                    2602     2642      +40     
- Misses                   460      464       +4     
- Partials                 108      110       +2

Components	Coverage Δ
access	`83.42% <ø> (ø)`
catalog	`87.60% <ø> (ø)`
common	`77.77% <ø> (ø)`
encryption	`72.97% <ø> (ø)`
keyring	`73.21% <ø> (ø)`
src	`93.26% <86.95%> (-0.90%)`	⬇️
smgr	`95.90% <ø> (ø)`
transam	`∅ <ø> (∅)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Andriciuc

Approved from docs, well done!

nastena1606 · 2025-08-14T14:55:50Z

contrib/pg_tde/documentation/docs/functions.md

+);
+```
+
+### pg_tde_get_wal_encryption_ranges


Suggested change

### pg_tde_get_wal_encryption_ranges

### pg_tde_get_wal_encryption_ranges

dutow · 2025-08-19T12:13:23Z

contrib/pg_tde/src/pg_tde.c

+		.tli = tli,.lsn = 0
+	};
+
+	keys = pg_tde_fetch_wal_keys(loc);


We shouldn't fetch them, as they are most likely are already in the cache. This should be pg_tde_get_wal_cache_keys

Writes may produce new WAL key, but writes don't update cache as allocations are forbidden there. So WAL reads have some logic how to check if new key was produced and refresh cache entries. After discussion with @dAdAbird we decided that doing it the same way in this function will make code too complex and we can just read keys from disk for now.

postgres/contrib/pg_tde/src/access/pg_tde_xlog_smgr.c

Lines 471 to 478 in 80e0bb0

/* write has generated a new key, need to fetch it */

if (last_key != NULL && wal_location_cmp(last_key->start, write_loc) < 0)

{

pg_tde_fetch_wal_keys(write_loc);

/* in case cache was empty before */

keys = pg_tde_get_wal_cache_keys();

}

But pg_tde_fetch_wal_keys always adds the fetched keys to the cache, it doesn't check if it's already in the cache. So if we call it multiple times with the beginning, we end up spamming the cache with duplicate records.

Also, if the cache already has items, it doesn't return the first key in the cache, but the first item added to the cache in this read.

Writes may produce new WAL key, but writes don't update cache as allocations are forbidden there

This is also no longer true since #539 now the current key is also in the cache. So reading the cache should be enough.

Eah, but thinking on it, pg_tde_fetch_wal_keys() will not only fetch data from disk dub populate cache with it. In the reader, we specify the last LSN so it'll add only the absent key to the cache. Here it will pull all the keys and add them for the second/third etc, time to the backend's cache. I'm not sure what consequences it may lead to... Unfortunately, we don't have an existing function to read only from the disk and don't flood the cache...

Ahh damn, you're right. I didn't check how cached was populated 🤦

contrib/pg_tde/src/pg_tde.c

dAdAbird · 2025-08-19T16:00:34Z

contrib/pg_tde/t/wal_encryption_ranges.pl

+$node->append_conf('postgresql.conf', "shared_preload_libraries = 'pg_tde'");
+$node->append_conf('postgresql.conf', "wal_level = 'logical'");
+# We don't test that it can't start: the test framework doesn't have an easy way to do this
+#$node->append_conf('postgresql.conf', "pg_tde.wal_encrypt = 1");


dAdAbird · 2025-08-19T16:04:20Z

contrib/pg_tde/t/wal_encryption_ranges.pl

+	stdout => \$psql_out);
+is($psql_out, 't', "Check that WAL record is still encrypted");
+
+# Check that WAL records that we recoreded before match the encryption ranges. We doint this


a typo in "recorded"

dAdAbird · 2025-08-19T16:06:32Z

contrib/pg_tde/t/wal_encryption_ranges.pl

+is($psql_out, 't', "Check that WAL record is still encrypted");
+
+# Check that WAL records that we recoreded before match the encryption ranges. We doint this
+# that way because LSN numbers are not stable across different runs of the test.


"We doint this that way..." needs rephrasing

dAdAbird · 2025-08-19T16:14:54Z

contrib/pg_tde/src/pg_tde.c

+		.tli = tli,.lsn = 0
+	};
+
+	keys = pg_tde_fetch_wal_keys(loc);


Eah, but thinking on it, pg_tde_fetch_wal_keys() will not only fetch data from disk dub populate cache with it. In the reader, we specify the last LSN so it'll add only the absent key to the cache. Here it will pull all the keys and add them for the second/third etc, time to the backend's cache. I'm not sure what consequences it may lead to... Unfortunately, we don't have an existing function to read only from the disk and don't flood the cache...

Add user function for checking if WAL record with given LSN is encrypted. Function assumes that provided LSN belongs to current timeline, however it allows to specify timeline ID as second argument.

Add user function that shows LSN ranges for encrypted records in WAL.

dAdAbird reviewed Aug 8, 2025

View reviewed changes

contrib/pg_tde/src/pg_tde.c Outdated Show resolved Hide resolved

jeltz reviewed Aug 9, 2025

View reviewed changes

contrib/pg_tde/pg_tde--1.0--2.0.sql Outdated Show resolved Hide resolved

artemgavrilov force-pushed the PG-1213-check-wal-enc-func branch 7 times, most recently from d411530 to f23f140 Compare August 13, 2025 19:49

artemgavrilov force-pushed the PG-1213-check-wal-enc-func branch 4 times, most recently from 80db6f8 to 6d0da57 Compare August 14, 2025 14:14

artemgavrilov marked this pull request as ready for review August 14, 2025 14:30

artemgavrilov requested review from jeltz and dAdAbird August 14, 2025 14:30

artemgavrilov force-pushed the PG-1213-check-wal-enc-func branch from 6d0da57 to 9967248 Compare August 14, 2025 14:40

artemgavrilov requested review from nastena1606 and Andriciuc as code owners August 14, 2025 14:40

nastena1606 deployed to PG-1213-check-wal-enc-func - INTERNAL-pg_tde docs PR #514 August 14, 2025 14:41 — with Render View deployment

Andriciuc approved these changes Aug 14, 2025

View reviewed changes

nastena1606 reviewed Aug 14, 2025

View reviewed changes

dutow reviewed Aug 19, 2025

View reviewed changes

dAdAbird reviewed Aug 19, 2025

View reviewed changes

artemgavrilov added 2 commits August 20, 2025 16:46

PG-1213 Add function for checking if WAL record is encrypted

097235c

Add user function for checking if WAL record with given LSN is encrypted. Function assumes that provided LSN belongs to current timeline, however it allows to specify timeline ID as second argument.

PG-1213 Add function that returns WAL encryption ragnes

c8e5310

Add user function that shows LSN ranges for encrypted records in WAL.

artemgavrilov force-pushed the PG-1213-check-wal-enc-func branch from 9967248 to c8e5310 Compare August 21, 2025 13:29

nastena1606 deployed to PG-1213-check-wal-enc-func - INTERNAL-pg_tde docs PR #514 August 21, 2025 13:29 — with Render View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

PG-1213 Add SQL function for checking if WAL record is encrypted #514

PG-1213 Add SQL function for checking if WAL record is encrypted #514

Uh oh!

artemgavrilov commented Aug 8, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Aug 13, 2025 •

edited

Loading

Uh oh!

Andriciuc left a comment

Uh oh!

nastena1606 Aug 14, 2025

Uh oh!

dutow Aug 19, 2025

Uh oh!

artemgavrilov Aug 19, 2025

Uh oh!

dutow Aug 19, 2025

Uh oh!

dutow Aug 19, 2025

Uh oh!

dAdAbird Aug 19, 2025

Uh oh!

artemgavrilov Aug 19, 2025

Uh oh!

Uh oh!

dAdAbird Aug 19, 2025

Uh oh!

dAdAbird Aug 19, 2025

Uh oh!

dAdAbird Aug 19, 2025

Uh oh!

dAdAbird Aug 19, 2025

Uh oh!

Uh oh!

	### pg_tde_get_wal_encryption_ranges
	### pg_tde_get_wal_encryption_ranges

	/* write has generated a new key, need to fetch it */
	if (last_key != NULL && wal_location_cmp(last_key->start, write_loc) < 0)
	{
	pg_tde_fetch_wal_keys(write_loc);

	/* in case cache was empty before */
	keys = pg_tde_get_wal_cache_keys();
	}

PG-1213 Add SQL function for checking if WAL record is encrypted #514

Are you sure you want to change the base?

PG-1213 Add SQL function for checking if WAL record is encrypted #514

Uh oh!

Conversation

artemgavrilov commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Andriciuc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artemgavrilov commented Aug 8, 2025 •

edited

Loading

codecov-commenter commented Aug 13, 2025 •

edited

Loading