Document How Commit Handles Aborted Transactions

Thoughts?

On Fri, Dec 20, 2024 at 9:02 AM David G. Johnston <
david.g.johnston@gmail.com> wrote:

On Tue, Dec 31, 2024 at 12:50 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

Thoughts?

On Fri, Dec 20, 2024 at 9:02 AM David G. Johnston <david.g.johnston@gmail.com> wrote:

Hi,

The commit reference page lacks an "Outputs" section even though it is capable of outputting both "COMMIT" and "ROLLBACK".

The attached adds this section, describes when each applies, and then incorporates the same into the main description for commit as well as the transaction section of the tutorial - which presently seems to be the main discussion area for the topic (the Concurrency Control chapter lacks a section for this introductory material).

This was noted as being needed by Tom Lane back into 2006 but it never happened.
/messages/by-id/28798.1142608067@sss.pgh.pa.us

It came up again when I was answering a question on Slack regarding "commit and chain" wondering whether the "and chain" could be made conditional (i.e., could the new transaction start aborted) on whether commit outputted "commit" or "rollback".

Its left implied that this behavior of "rollback" is standard-conforming. Please feel free to suggest/add language to the Compatibility section if this is not the case.

I generally agree with the improvements proposed. I currently don't
have the infrastructure to build docs, so the following review is
without the benefit of what the build output looks like.

This line in the patch has a trailing whitespace, which should be removed.

+ <xref linkend="sql-begin"/> and

I believe this sentence can be improved slightly:

+    When a failure does occur during a transaction it is not ended but instead
+    goes into an aborted state.

as: When an error occurs in a transaction block, the transaction goes
into an aborted state.

This borrows the term 'transaction block' introduced in the previous
paragraph (the one before the note), and replaces the word 'failure'
(a very wide category) with 'error' (implying ERRORs raised by SQL
commands inside a transaction block).

I think the following intends to convey that if a transaction block is
in an aborted state due to an error, the COMMIT command will behave
identical to the ROLLBACK command. So the reference to 'changes' is
extraneous, since a transaction block may have encountered error even
without having made any changes.

+   If no changes have been made - because the transaction is in an
+   aborted state - the effect of the commit will look like a rollback,
+   including the command tag output.

So this seems like a better statement: If the transaction is in an
aborted state, say, because of an error, then the effect of the
<command>COMMIT</> will be identical to that of <command>ROLLBACK</>,
including the command tag output.

The following needs to be rephrased:

+   However, if the transaction being affected is aborted, a
<command>COMMIT</command>
+   command returns a command tag of the form

as: However, if the transaction is in an aborted state, the
<command>COMMIT</> command ...

Best regards,
Gurjeet
http://Gurje.et

On Wed, Jan 1, 2025 at 11:16 PM Gurjeet Singh <gurjeet@singh.im> wrote:

On Fri, Dec 20, 2024 at 9:02 AM David G. Johnston <

david.g.johnston@gmail.com> wrote:

The commit reference page lacks an "Outputs" section even though it is

capable of outputting both "COMMIT" and "ROLLBACK".

I generally agree with the improvements proposed. I currently don't
have the infrastructure to build docs, so the following review is
without the benefit of what the build output looks like.

Thank you for the review. Version 2 Attached.

This line in the patch has a trailing whitespace, which should be removed.

+ <xref linkend="sql-begin"/> and

Done

I believe this sentence can be improved slightly:

+    When a failure does occur during a transaction it is not ended but
instead
+    goes into an aborted state.

as: When an error occurs in a transaction block, the transaction goes
into an aborted state.

Agreed, and changed the existing usage of failure to error to match.

So this seems like a better statement: If the transaction is in an
aborted state, say, because of an error, then the effect of the
<command>COMMIT</> will be identical to that of <command>ROLLBACK</>,
including the command tag output.

I went with:

+   If the transaction is in an aborted state then no changes will be made
+   and the effect of the <command>COMMIT</command> will be identical to
that
+   of <command>ROLLBACK</command>, including the command tag output.

The following needs to be rephrased:

+   However, if the transaction being affected is aborted, a
<command>COMMIT</command>
+   command returns a command tag of the form

as: However, if the transaction is in an aborted state, the
<command>COMMIT</> command ...

I went with this, keeping the phrasing "on an" consistent between this and
the previous text.

+   However, on an aborted transaction, a <command>COMMIT</command>
+   command returns a command tag of the form

The use of "a/an" instead of "the" is supported by existing phrasing for
Insert.

"On successful completion, an INSERT command returns a command tag of the
form"

David J.

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: tested, failed
Documentation: tested, failed

Summary:
---------
The patch adds documentation to clarify how PostgreSQL handles aborted transactions during the commit process. The changes are clear and improve the existing documentation.

Testing:
--------
1. Manually applied the patch to the latest master branch (commit 4cffc93).
2. Fixed SGML structure issues in `advanced.sgml` and `commit.sgml` by wrapping `<varlistentry>` in `<variablelist>`.
3. Rebuilt the documentation using `make html`.
4. Verified the new sections are present and correctly formatted in the generated HTML.

Feedback:
---------
- The patch was manually applied due to conflicts in `advanced.sgml` and `commit.sgml`.
- Fixed invalid SGML structure by wrapping `<varlistentry>` in `<variablelist>`.
- The documentation is accurate and follows PostgreSQL’s style guidelines.
- No additional issues were found.

Recommendation:
---------------
Ready for committer. No objections.

The new status of this patch is: Ready for Committer

Ahmed,

Thank you for the review.

I'm a bit confused by the reports of apply and compile errors. I didn't
touch anything involving "varlist" and see no errors then or now in my
Meson ninja build. Nor does the CI report any.

On Fri, Feb 14, 2025 at 2:01 PM Ahmed Ashour <a8087027@gmail.com> wrote:

Feedback:
---------
- The patch was manually applied due to conflicts in `advanced.sgml` and
`commit.sgml`.
- Fixed invalid SGML structure by wrapping `<varlistentry>` in
`<variablelist>`.

If those errors were/are real this wouldn't be ready to commit. But as
they seem to be a local environment issue on your end, and you agree with
the content, I'll keep it ready to commit.

David J.

Recent discussion led me to realize we are also contrary to the SQL
Standard here. v3 updates the Commit reference page to reflect this fact.

Leaving ready-to-commit.

David J.

On Thu, 29 May 2025 at 04:32, David G. Johnston
<david.g.johnston@gmail.com> wrote:

Recent discussion led me to realize we are also contrary to the SQL Standard here. v3 updates the Commit reference page to reflect this fact.

Leaving ready-to-commit.

David J.

Hi!
I reviewed your changes and I agree with them.

The only aspect that drew my attention is in the following sentence:

+ but instead goes into an aborted state. While in this state all commands except
+    <xref linkend="sql-commit"/> and <xref linkend="sql-rollback"/> are ignored

We can also do ABORT;

Is this worth noting?

--
Best regards,
Kirill Reshke

On Friday, August 22, 2025, Kirill Reshke <reshkekirill@gmail.com> wrote:

We can also do ABORT;

Listing commands described as being “present for historical reasons” in the
documentation seems unnecessary. We don’t list abort in the “See Also”
section either.

David J.

On Fri, 22 Aug 2025 at 18:31, David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Friday, August 22, 2025, Kirill Reshke <reshkekirill@gmail.com> wrote:

We can also do ABORT;

Listing commands described as being “present for historical reasons” in the documentation seems unnecessary. We don’t list abort in the “See Also” section either.

David J.

I bumped into specific behaviour of COMMIT in an already-broken
transaction yet again recently. So, I moved CF entry to the next CF,
hope this will get eventually merged.

LGTM

--
Best regards,
Kirill Reshke

Kirill Reshke <reshkekirill@gmail.com> writes:

On Fri, 22 Aug 2025 at 18:31, David G. Johnston
<david.g.johnston@gmail.com> wrote:

Listing commands described as being “present for historical reasons” in the documentation seems unnecessary. We don’t list abort in the “See Also” section either.

I bumped into specific behaviour of COMMIT in an already-broken
transaction yet again recently. So, I moved CF entry to the next CF,
hope this will get eventually merged.
LGTM

Pushed with minor emendations.

I'm not quite sure why we describe ABORT as quasi-deprecated but not
BEGIN; neither of them are in the standard. I'm disinclined to touch
that state of affairs though.

regards, tom lane