Some comments on documentation #436

AndersAstrand · 2025-06-17T06:23:12Z

Some comments I had during a cursory read-through of the docs.

My comments are prefixed with AÅ:

Obviously this is never meant to be merged, it's just a way to give comments 🙂

Some comments I had during a cursory read-through of the docs. My comments are prefixed with AÅ:

codecov-commenter · 2025-06-17T06:27:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.65%. Comparing base (5610639) to head (f02c72d).

❌ Your project status has failed because the head coverage (84.65%) 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     #436      +/-   ##
=====================================================
- Coverage              84.66%   84.65%   -0.01%     
=====================================================
  Files                     21       21              
  Lines                   2589     2588       -1     
  Branches                 402      401       -1     
=====================================================
- Hits                    2192     2191       -1     
  Misses                   316      316              
  Partials                  81       81

Components	Coverage Δ
access	`81.11% <ø> (ø)`
catalog	`88.22% <ø> (ø)`
common	`77.77% <ø> (ø)`
encryption	`73.45% <ø> (ø)`
keyring	`72.88% <ø> (ø)`
src	`91.44% <ø> (ø)`
smgr	`94.85% <ø> (-0.03%)`	⬇️
transam	`∅ <ø> (∅)`

🚀 New features to boost your workflow:

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

Andriciuc · 2025-06-17T07:18:42Z

contrib/pg_tde/documentation/docs/command-line-tools/pg-waldump.md

@@ -1,5 +1,7 @@
 # pg_waldump

+AÅ: We should probably include a notice about WAL encrypting being BETA and this information is subject to change
+
 [`pg_waldump` :octicons-link-external-16:](https://www.postgresql.org/docs/current/pgwaldump.html) is a tool to display a human-readable rendering of the Write-Ahead Log (WAL) of a PostgreSQL database cluster.



Added this note:

The WAL encryption feature is currently in beta and is not effective unless explicitly enabled. It is not yet production ready. Do not enable this feature in production environments.

Andriciuc · 2025-06-17T07:26:31Z

contrib/pg_tde/documentation/docs/faq.md

@@ -87,6 +87,7 @@ The initial decision on what file to encrypt is based on the table access method
 The principal key is used to encrypt the internal keys. The principal key is stored in the key management store. When you query the table, the principal key is retrieved from the key store to decrypt the table. Then the internal key for that table is used to decrypt the data.

 ### WAL encryption
+AÅ: We should mention this is a BETA feature and subject to change.



Added a note:

WAL encryption is currently in beta and is not effective unless explicitly enabled. It is not yet production ready. Do not enable this feature in production environments.

Andriciuc · 2025-06-17T07:28:52Z

contrib/pg_tde/documentation/docs/faq.md

@@ -134,6 +136,7 @@ You must restart the database in the following cases to apply the changes:

 * after you enabled the `pg_tde` extension
 * to turn on / off the WAL encryption
+AÅ: WAL encrytion is not yet production ready and should probably not be casually mentioned as something you can "turn on".



updated this to turn on/off line to:

when enabling WAL encryption, which is currently in beta. Do not enable this feature in production environments.

Andriciuc · 2025-06-17T07:53:04Z

contrib/pg_tde/documentation/docs/faq.md

@@ -150,6 +153,7 @@ In `pg_tde`, multi-tenancy is supported via a separate principal key per databas
 To control user access to the databases, you can use role-based access control (RBAC).

 WAL files are encrypted globally across the entire PostgreSQL cluster using the same encryption keys. Users don't interact with WAL files as these are used by the database management system to ensure data integrity and durability.
+AÅ: Again, WAL encryption hould probably not be mentioned as if it was a feature already.



commented the above paragraph out for now regarding WAL

Andriciuc · 2025-06-17T07:59:25Z

contrib/pg_tde/documentation/docs/faq.md

@@ -117,6 +118,7 @@ The support of other encryption mechanisms such as AES256 is planned for future
 ## Is post-quantum encryption supported?

 No, it's not yet supported. In our implementation we reply on OpenSSL libraries that don't yet support post-quantum encryption.
+AÅ: Afaik newer versions of openssl support post-quantum algorithms. Maybe we should use some better excuse or mention what version we use?



Updated the paragraph to:

No, pg_tde does not support post-quantum encryption.

Andriciuc · 2025-06-17T14:19:14Z

Just wanted to say, appreciate all the great suggestions Anders!

Andriciuc · 2025-06-17T14:36:25Z

contrib/pg_tde/documentation/docs/architecture/index.md

@@ -4,6 +4,8 @@

 Let's break down what it means.

+AÅ: We need to make it clear that this file are the ambitions for pg_tde, because a lot of this is not yet in there.
+


So, I wanted to mention that architecture is similar to design here, so I thought why not combine these two. Added the following:

This topic outlines the long-term architectural goals and intended design for pg_tde. While many of the described components are implemented, however some features are still under active development and may not yet be available in the current release.

Let's break down the key building blocks of this design.

Andriciuc · 2025-06-18T10:51:42Z

contrib/pg_tde/documentation/docs/features.md

 * Single-tenancy support via a global keyring provider
 * Multi-tenancy support
 * Table-level granularity for encryption and access control
 * Multiple Key management options
 * Logical replication of encrypted tables
+AÅ: We do not do anything for logical replication, do we?



Removed temporary tables feature to clear confusion, removed logical replication mention, removed WAL encryption as a feature.

Andriciuc · 2025-06-18T10:52:47Z

contrib/pg_tde/documentation/docs/global-key-provider-configuration/set-principal-key.md

@@ -36,6 +36,8 @@ The key material (actual cryptographic key) is auto-generated by `pg_tde` and st
 !!! note
    This process sets the **default principal key** for the server. Any database without its own key configuration will use this key.

+AÅ: This is confusing to me, but maybe makes sense in the rendered version. The Example below does _not_ set a default key, does it?
+
 ## Example



I have reworded the note and the below example to properly describe the correct param:

updated with correct code example using set_server_key_using_global parameter

updated note to reflect correct config

Andriciuc · 2025-06-18T10:57:07Z

contrib/pg_tde/documentation/docs/functions.md

@@ -10,6 +10,7 @@ However, database owners can run the “view keys” and “set principal key”

 * `GRANT EXECUTE`
 * `REVOKE EXECUTE`
+AÅ: This should probably say `GRANT EXECUTE ON FUNCTION` and similar for revoke



Fixed:

GRANT EXECUTE ON FUNCTION

REVOKE EXECUTE ON FUNCTION

Andriciuc · 2025-06-18T11:12:29Z

contrib/pg_tde/documentation/docs/functions.md

@@ -52,6 +53,8 @@ The `change` functions require the same parameters as the `add` functions. They

 Provider specific parameters differ for each implementation. Refer to the  respective subsection for details.

+AÅ: We should probably have a notice here about the modified provider settings will need to be able to provide the exact same principal keys as the original settings.
+


Added the following note for this:

!!! note
The updated provider must be able to retrieve the same principal keys as the original configuration.
If the new configuration cannot access existing keys, encrypted data and backups will become unreadable.

Andriciuc · 2025-06-18T11:13:28Z

contrib/pg_tde/documentation/docs/functions.md

@@ -231,6 +234,7 @@ These functions list the details of all key providers for the current database o
 ## Principal key management

 Use these functions to create a new principal key at a given keyprover, and to use those keys for a specific scope such as a current database, a global or default scope. You can also use them to start using a different existing key for a specific scope.
+AÅ: "keyprover"?



fixed, key provider.

Andriciuc · 2025-06-18T11:13:44Z

contrib/pg_tde/documentation/docs/functions.md

@@ -281,6 +285,7 @@ SELECT pg_tde_set_key_using_global_key_provider(
 ### pg_tde_set_server_key_using_global_key_provider

 Sets or rotates the server principal key using the specified global key provider. Use this function to set a principal key for WAL encryption.
+AÅ: Note that WAL encryption is not yet production ready



added the standard WAL warning here

Andriciuc · 2025-06-18T12:01:40Z

contrib/pg_tde/documentation/docs/how-to/multi-tenant-setup.md

@@ -82,6 +82,7 @@ You must do these steps for every database where you have created the extension.
    === "With HashiCorp Vault"

        The Vault server setup is out of scope of this document.
+        AÅ: Why doesn't KMIP have a similar comment? Is KMIP more in scope than vault? Why isn't OpenBao mentioned here?



Good point! Added the same paragraph for KMIP

Andriciuc · 2025-06-19T08:30:53Z

contrib/pg_tde/documentation/docs/how-to/multi-tenant-setup.md

@@ -144,6 +145,7 @@ You must do these steps for every database where you have created the extension.

    * `name-of-the-key` is the name of the principal key. You will use this name to identify the key.
    * `provider-name` is the name of the key provider you added before. The principal key will be associated with this provider.
+    AÅ: It will not only be "associated" with it, it is where the key will be stored and fetched from.



Updated:

provider-name is the name of the key provider you added before. The principal key is associated with this provider and it is the location where it is stored and fetched from.

Andriciuc · 2025-06-19T09:44:32Z

contrib/pg_tde/documentation/docs/index/supported-versions.md

@@ -9,6 +9,7 @@ The extension is tightly integrated with Percona Server for PostgreSQL to delive
 By using our PostgreSQL distribution, you get:

 - **Full encryption support** through the `tde_heap` access method, including tables, indexes, WAL data, and logical replication.
+AÅ: Logical replication?


Andriciuc · 2025-06-19T10:09:46Z

contrib/pg_tde/documentation/docs/how-to/uninstall.md

@@ -4,6 +4,8 @@ If you no longer wish to use TDE in your deployment, you can remove the `pg_tde`

 Here's how to do it:

+AÅ: We don't think that anyone would like to drop the extension, but keep their data? Ie. should this document mention decryption of tables before dropping rather than only how to drop the tables?
+


This is great feedback! To ensure we covered all possible scenarios I added a new step 1:

Decrypt or drop encrypted tables:

Before removing the extension, you must either decrypt or drop all encrypted tables. pg_tde does not support decrypting tables in-place yet. Therefore:

To preserve the data: manually copy it into unencrypted tables

To discard data: drop the encrypted tables

Also, added a note of warning in the intro and reworded that a bit too:

!!! warning
This process removes the extension, but does not decrypt data automatically. Only uninstall the extension after all encrypted data has been removed or decrypted.

Step 2 is also updated:

Drop the extension using the DROP EXTENSION command:

DROP EXTENSION pg_tde;

Alternatively, to remove everything at once:

DROP EXTENSION pg_tde CASCADE;

!!! note
The DROP EXTENSION command does not delete the underlying pg_tde-specific data files from disk.

Andriciuc · 2025-06-19T10:22:45Z

contrib/pg_tde/documentation/docs/index/table-access-method.md

@@ -53,6 +53,7 @@ Here is how you can set the new default table access method:
 1. Add the access method to the `default_table_access_method` parameter:

    === "via the SQL statement"
+AÅ: I think "the" shouldn't be here, should it? It's also weird that it's called a "statement" here but "command" below. Maybe "via ALTER SYSTEM" is a better heading?



Updated as such:

Add the access method to the default_table_access_method parameter:

=== "via the ALTER SYSTEM command"

Use `ALTER SYSTEM`, it requires superuser or `ALTER SYSTEM` privileges. ```sql ALTER SYSTEM SET default_table_access_method = tde_heap; ```

Andriciuc · 2025-06-19T10:44:24Z

contrib/pg_tde/documentation/docs/index/tde-limitations.md

@@ -1,5 +1,7 @@
 # Limitations of pg_tde

+AÅ: We should mention that WAL encryption isn't yet ready for use rather than mentioning pg_rewind specifically.
+


Reorganized limitations, removed rewind mention and added WAL note as text, made small changes to RC version. Added note for KMS, improved system tables text.

Andriciuc · 2025-06-19T10:49:33Z

contrib/pg_tde/documentation/docs/setup.md

@@ -12,6 +12,7 @@ The `pg_tde` extension requires additional shared memory. You need to configure
 You can configure the `shared_preload_libraries` parameter in two ways:

 * Add the following line to the `shared_preload_libraries` file:
+AÅ: Which file?



Fixed this: * Add the following line to the postgresql.conf file: ...

Also updated the last note:

!!! note
It’s recommended to use an external key provider (KMS) to manage encryption keys. For configuration instructions, see Next steps.

Andriciuc · 2025-06-19T12:04:15Z

contrib/pg_tde/documentation/docs/variables.md

@@ -3,6 +3,7 @@
 The `pg_tde` extension provides GUC variables to configure the behaviour of the extension:

 ## pg_tde.wal_encrypt
+AÅ: Note that this shouldn't be enabled for production systems



added warning note

Andriciuc · 2025-06-19T12:04:49Z

contrib/pg_tde/documentation/docs/test.md

@@ -30,6 +30,7 @@ After enabling the `pg_tde` extension for a database, you can begin encrypting d
    ```

    The function returns `t` if the table is encrypted and `f` - if not.
+AÅ: The function returns `true` or `false`. the psql client specifically renders them as `t` or `f` however.



Updated with:

The function returns true or false. The psql client specifically renders them as t or f respectively.

Andriciuc · 2025-06-19T12:23:49Z

contrib/pg_tde/documentation/docs/yum.md

@@ -9,6 +9,7 @@ Make sure you check the [list of supported platforms](install.md#__tabbed_1_1) b
 The `pg_tde` uses memory locks (mlocks) to keep internal encryption keys in RAM, both for WAL and for user data.  

 A memory lock (`mlock`) is a system call to lock a specified memory range in RAM for a process. The maximum amount of memory that can be locked differs between systems. You can check the current setting with this command:
+AÅ: This isn't necessarily true. Maybe OpenSSL does this under the hood? We don't do it in our code.



Hmm, we have two options here, just mention that you can check the system limit using the command if user uses mlock, or this:

A memory lock (mlock) is a system call that prevents specified memory ranges from being swapped to disk. Some libraries, such as OpenSSL, may use mlock internally to protect sensitive data like encryption keys. However, pg_tde does not directly invoke mlock in its own code.

You can check the current system limits for locked memory using:

ulimit -l

Thoughts?

DO NOT MERGE EVER

f02c72d

Some comments I had during a cursory read-through of the docs. My comments are prefixed with AÅ:

Andriciuc added the documentation Improvements or additions to documentation label Jun 17, 2025

Andriciuc reviewed Jun 17, 2025

View reviewed changes

Andriciuc reviewed Jun 18, 2025

View reviewed changes

Andriciuc reviewed Jun 19, 2025

View reviewed changes

		@@ -4,6 +4,8 @@

		Let's break down what it means.

		AÅ: We need to make it clear that this file are the ambitions for pg_tde, because a lot of this is not yet in there.

		@@ -52,6 +53,8 @@ The `change` functions require the same parameters as the `add` functions. They

		Provider specific parameters differ for each implementation. Refer to the respective subsection for details.

		AÅ: We should probably have a notice here about the modified provider settings will need to be able to provide the exact same principal keys as the original settings.

		@@ -4,6 +4,8 @@ If you no longer wish to use TDE in your deployment, you can remove the `pg_tde`

		Here's how to do it:

		AÅ: We don't think that anyone would like to drop the extension, but keep their data? Ie. should this document mention decryption of tables before dropping rather than only how to drop the tables?

		@@ -1,5 +1,7 @@
		# Limitations of pg_tde

		AÅ: We should mention that WAL encryption isn't yet ready for use rather than mentioning pg_rewind specifically.

Some comments on documentation #436

Are you sure you want to change the base?

Some comments on documentation #436

Uh oh!

Conversation

AndersAstrand commented Jun 17, 2025

Uh oh!

codecov-commenter commented Jun 17, 2025

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andriciuc Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andriciuc Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andriciuc commented Jun 17, 2025

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!

Andriciuc Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andriciuc Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Andriciuc Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Andriciuc Jun 17, 2025 •

edited

Loading

Andriciuc Jun 17, 2025 •

edited

Loading

Andriciuc Jun 18, 2025 •

edited

Loading

Andriciuc Jun 18, 2025 •

edited

Loading

Andriciuc Jun 19, 2025 •

edited

Loading