Re: New docs chapter on Transaction Management and related changes

On Sat, 2022-11-05 at 10:08 +0000, Simon Riggs wrote:
> Agreed; new compilation patch attached, including mine and then
> Robert's suggested rewordings.

Thanks. There is clearly a lot of usefule information in this.

Some comments:

> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -24673,7 +24673,10 @@ SELECT collation for ('foo' COLLATE "de_DE");
> <para>
> Returns the current transaction's ID. It will assign a new one if the
> current transaction does not have one already (because it has not
> - performed any database updates).
> + performed any database updates); see <xref
> + linkend="transaction-id"/> for details. If executed in a
> + subtransaction this will return the top-level xid; see <xref
> + linkend="subxacts"/> for details.
> </para></entry>
> </row>

I would use a comma after "subtransaction", and I think it would be better to write
"transaction ID" instead of "xid".

> @@ -24690,6 +24693,7 @@ SELECT collation for ('foo' COLLATE "de_DE");
> ID is assigned yet. (It's best to use this variant if the transaction
> might otherwise be read-only, to avoid unnecessary consumption of an
> XID.)
> + If executed in a subtransaction this will return the top-level xid.
> </para></entry>
> </row>

Same as above.

> @@ -24733,6 +24737,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
> <para>
> Returns a current <firstterm>snapshot</firstterm>, a data structure
> showing which transaction IDs are now in-progress.
> + Only top-level xids are included in the snapshot; subxids are not
> + shown; see <xref linkend="subxacts"/> for details.
> </para></entry>
> </row>

Again, I would avoid "xid" and "subxid", or at least use "transaction ID (xid)"
and similar.

> --- a/doc/src/sgml/ref/release_savepoint.sgml
> +++ b/doc/src/sgml/ref/release_savepoint.sgml
> @@ -34,23 +34,16 @@ RELEASE [ SAVEPOINT ] <replaceable>savepoint_name</replaceable>
> <title>Description</title>
>
> <para>
> - <command>RELEASE SAVEPOINT</command> destroys a savepoint previously defined
> - in the current transaction.
> + <command>RELEASE SAVEPOINT</command> will subcommit the subtransaction
> + established by the named savepoint, if one exists. This will release
> + any resources held by the subtransaction. If there were any
> + subtransactions of the named savepoint, these will also be subcommitted.
> </para>
>
> <para>

"Subtransactions of the named savepoint" is somewhat confusing; how about
"subtransactions of the subtransaction established by the named savepoint"?

If that is too long and explicit, perhaps "subtransactions of that subtransaction".

> @@ -78,7 +71,7 @@ RELEASE [ SAVEPOINT ] <replaceable>savepoint_name</replaceable>
>
> <para>
> It is not possible to release a savepoint when the transaction is in
> - an aborted state.
> + an aborted state, to do that use <xref linkend="sql-rollback-to"/>.
> </para>
>
> <para>

I think the following is more English:
"It is not possible ... state; to do that, use <xref .../>."

> --- a/doc/src/sgml/ref/rollback.sgml
> +++ b/doc/src/sgml/ref/rollback.sgml
> @@ -56,11 +56,14 @@ ROLLBACK [ WORK | TRANSACTION ] [ AND [ NO ] CHAIN ]
> <term><literal>AND CHAIN</literal></term>
> <listitem>
> <para>
> - If <literal>AND CHAIN</literal> is specified, a new transaction is
> + If <literal>AND CHAIN</literal> is specified, a new unaborted transaction is
> immediately started with the same transaction characteristics (see <xref
> linkend="sql-set-transaction"/>) as the just finished one. Otherwise,
> no new transaction is started.

I don't think that is an improvement. "Unaborted" is an un-word. A new transaction
is always "unaborted", isn't it?

> --- a/doc/src/sgml/wal.sgml
> +++ b/doc/src/sgml/wal.sgml
> @@ -909,4 +910,36 @@
> seem to be a problem in practice.
> </para>
> </sect1>
> +
> + <sect1 id="two-phase">
> +
> + <title>Two-Phase Transactions</title>
> +
> + <para>
> + <productname>PostgreSQL</productname> supports a two-phase commit (2PC)
[...]
> + <filename>pg_twophase</filename> directory. Currently-prepared
> + transactions can be inspected using <link
> + linkend="view-pg-prepared-xacts"><structname>pg_prepared_xacts</structname></link>.
> + </para>
> + </sect1>
> +
> </chapter>

I don't like "currently-prepared". How about:
"Transaction that are currently prepared can be inspected..."

This is clearly interesting information, but I don't think the WAL chapter is the right
place for this. "pg_twophase" is already mentioned in "storage.sgml", and details about
when exactly a prepared transaction is persisted may exceed the details level needed by
the end user.

I'd look for that information in the reference page for PREPARE TRANSACTION; perhaps
that would be a better place. Or, even better, the new "xact.sgml" chapter.

> --- /dev/null
> +++ b/doc/src/sgml/xact.sgml

+ <title>Transaction Management</title>

+ The word transaction is often abbreviated as "xact".

Should use <quote> here.

> + <title>Transactions and Identifiers</title>

> + <para>
> + Once a transaction writes to the database, it is assigned a
> + non-virtual <literal>TransactionId</literal> (or <type>xid</type>),
> + e.g., <literal>278394</literal>. Xids are assigned sequentially
> + using a global counter used by all databases within the
> + <productname>PostgreSQL</productname> cluster. This property is used by
> + the transaction system to order transactions by their first database
> + write, i.e., lower-numbered xids started writing before higher-numbered
> + xids. Of course, transactions might start in a different order.
> + </para>

"This property"? How about:
"Because transaction IDs are assigned sequentially, the transaction system can
use them to order transactions by their first database write"

I would want some additional information here: why does the transaction system have
to order transactions by their first database write?

"Of course, transactions might start in a different order."

Now that confuses me. Are you saying that BEGIN could be in a different order
than the first database write? Perhaps like this:

"Note that the order in which transactions perform their first database write
might be different from the order in which the transactions started."

> + The internal transaction ID type <type>xid</type> is 32-bits wide

There should be no hyphen in "32 bits wide", just as in "3 years old".

> + A 32-bit epoch is incremented during each
> + wrap around.

We usually call this "wraparound" without a space.

> + There is also a 64-bit type <type>xid8</type> which
> + includes this epoch and therefore does not wrap around during the
> + life of an installation and can be converted to xid by casting.

Running "and"s. Better:

"There is also ... and does not wrap ... life of an installation.
<type>xid8</type> can be converted to <type>xid</type> by casting."

> + Xids are used as the
> + basis for <productname>PostgreSQL</productname>'s <link
> + linkend="mvcc">MVCC</link> concurrency mechanism, <link
> + linkend="hot-standby">Hot Standby</link>, and Read Replica servers.

What is the difference between a hot standby and a read replica? I think
one of these terms is sufficient.

> + In addition to <literal>vxid</literal> and <type>xid</type>,
> + when a transaction is prepared for two-phase commit it
> + is also identified by a Global Transaction Identifier
> + (<acronym>GID</acronym>).

Better:

"In addition to <literal>vxid</literal> and <type>xid</type>,
prepared transactions also have a Global Transaction Identifier
(<acronym>GID</acronym>) that is assigned when the transaction is
prepared for two-phase commit."

> + <sect1 id="xact-locking">
> +
> + <title>Transactions and Locking</title>
> +
> + <para>
> + Currently-executing transactions are shown in <link
> + linkend="view-pg-locks"><structname>pg_locks</structname></link>
> + in columns <structfield>virtualxid</structfield> and
> + <structfield>transactionid</structfield>.

Better:

"The transaction IDs of currently executing transactions are shown in <link
linkend="view-pg-locks"><structname>pg_locks</structname></link>
in the columns <structfield>virtualxid</structfield> and
<structfield>transactionid</structfield>."

> + Lock waits on table-level locks are shown waiting for
> + <structfield>virtualxid</structfield>, while lock waits on row-level
> + locks are shown waiting for <structfield>transactionid</structfield>.

That's not true. Transactions waiting for table-level locks are shown
waiting for a "relation" lock in both "pg_stat_activity" and "pg_locks".

> + Row-level read and write locks are recorded directly in locked
> + rows and can be inspected using the <xref linkend="pgrowlocks"/>
> + extension. Row-level read locks might also require the assignment
> + of multixact IDs (<literal>mxid</literal>). Mxids are recorded in
> + the <filename>pg_multixact</filename> directory.

"are recorded directly in *the* locked rows"

I think the mention of multixacts should link to
<xref linkend="vacuum-for-multixact-wraparound"/>. Again, I would not
specifically mention the directory, since it is already described in
"storage.sgml", but I have no strong optinion there.

> + <sect1 id="subxacts">
> +
> + <title>Subtransactions</title>

> + The word subtransaction is often abbreviated as
> + <literal>subxact</literal>.

I'd use <quote>, not <literal>.

> + If a subtransaction is assigned a non-virtual transaction ID,
> + its transaction ID is referred to as a <literal>subxid</literal>.

Again, I would use <quote>, since we don't <literal> "subxid"
elsewhere.

+ Up to
+ 64 open subxids are cached in shared memory for each backend; after
+ that point, the overhead increases significantly since we must look
+ up subxid entries in <filename>pg_subtrans</filename>.

Comma before "since". Perhaps you should mention that this means disk I/O.

Yours,
Laurenz Albe