Thread: New docs chapter on Transaction Management and related changes
New chapter on transaction management, plus a few related changes. Markup and links are not polished yet, so please comment initially on the topics, descriptions and wording. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
Op 06-09-2022 om 17:16 schreef Simon Riggs: > New chapter on transaction management, plus a few related changes. > > Markup and links are not polished yet, so please comment initially on > the topics, descriptions and wording. > [xact_docs.v2.patch] Very clear explanations, thank you. Two typos: 'not yet yet part' should be 'not yet part' 'extenal' should be 'external' Erik
On Tue, 6 Sept 2022 at 17:19, Erik Rijkers <er@xs4all.nl> wrote: > > Op 06-09-2022 om 17:16 schreef Simon Riggs: > > New chapter on transaction management, plus a few related changes. > > > > Markup and links are not polished yet, so please comment initially on > > the topics, descriptions and wording. > > > [xact_docs.v2.patch] > > Very clear explanations, thank you. > > Two typos: > > 'not yet yet part' should be > 'not yet part' > > 'extenal' should be > 'external' Thanks, new version attached. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Tue, Sep 06, 2022 at 04:16:02PM +0100, Simon Riggs wrote: > New chapter on transaction management, plus a few related changes. > > Markup and links are not polished yet, so please comment initially on > the topics, descriptions and wording. This is useful. Nitpicks follow. + If executed in a subtransaction this will return the top-level xid. I'd prefer it with a comma after "subtransaction" +SQL Standard. PostgreSQL supports SAVEPOINTs through the implementation +defined mechanism of subtransactions, which offer a superset of the required implementation-defined +features. Prepared transactions allow PostgreSQL to implement what SQL what *the +<para> +All transactions are identified by a unique VirtualTransactionId (virtualxid or +vxid), e.g. 4/12532, which is comprised of the BackendId (in this example, 4) +and a sequentially assigned number unique to that backend, known as LocalXid sequentially-assigned +<para> +If a transaction attempts to write to the database it will be assigned a database comma +property is used by the transaction system to say that if one xid is earlier +than another xid then the earlier transaction attempted to write before the xid comma then +<para> +Currently executing transactions are shown in the pg_locks view in columns currently-executing +"virtualxid" (text) and "transactionid" (xid), if an xid has been assigned. maybe remove the "if assigned" part, since it's described next? +Read transactions will have a virtualxid but a NULL xid, while write +transactions will have both a virtualxid and an xid assigned. +</para> +<para> +Row-level read locks may require the assignment of a multixact ID (mxid), which +are recorded in the pg_multixact directory. which *is ? +top-level transaction. Subtransactions can also be started from other +subtransactions. As a result, the arrangement of transaction and subtransactions transactions (plural) ? +form a hierarchy or tree. Thus, each subtransaction has one parent transaction. +At present in PostgreSQL, only one transaction or subtransaction can be active at +one time. one time per command/query/request. +Subtransactions may end via a commit or abort without affecting their parent may end either by committing or aborting, without .. +transaction, allowing the parent transaction to continue. +also be started in other ways, such as PL/pgSQL's EXCEPTION clause. PL/Python and +PL/TCL also support explicit subtransactions. Working with C API, users may also +call BeginInternalSubTransaction(). *the C API ? +If a subtransaction is assigned an xid, we refer to this as a subxid. Read-only +subtransactions are not assigned a subxid, but when a subtransaction attempts to +write it will be assigned a subxid. We ensure that all of a subxid's parents, up write comma. Or say: "subxid is not assigned until the subtransaction attempts to write" ? +<para> +The more subtransactions each transaction uses, the greater the overhead for +transaction management. Up to 64 subxids are cached in shmem for each backend, backend semicolon +Those commands are extensions to the SQL Standard, since the SQL syntax is not yet +yet part of the standard, though the Standard does refer to encompassing yet yet +Information relating to these is stored in pg_twophase. Currently prepared s/these/two-phase commits/ +transactions can be inspected using pg_prepared_xacts view. +</para> currently-prepared ?
On Tue, 6 Sept 2022 at 21:33, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Tue, Sep 06, 2022 at 04:16:02PM +0100, Simon Riggs wrote: > > New chapter on transaction management, plus a few related changes. > > > > Markup and links are not polished yet, so please comment initially on > > the topics, descriptions and wording. > > This is useful. Nitpicks follow. Cool, thanks. > +At present in PostgreSQL, only one transaction or subtransaction can be active at > +one time. > > one time per command/query/request. Apart from that comment, all points accepted, thank you. I've also added further notes about prepared transactions. I attach a diff against the original patch, plus a new clean copy. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On 2022-Sep-06, Simon Riggs wrote: > On Tue, 6 Sept 2022 at 17:19, Erik Rijkers <er@xs4all.nl> wrote: > > > > Op 06-09-2022 om 17:16 schreef Simon Riggs: > > > New chapter on transaction management, plus a few related changes. > > > > > > Markup and links are not polished yet, so please comment initially on > > > the topics, descriptions and wording. I think the concept of XID epoch should be described also, in or near the paragraph that talks about wrapping around and switching between int32 and int64. Right now it's a bit unclear why/how that works. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Uno puede defenderse de los ataques; contra los elogios se esta indefenso"
On Thu, 8 Sept 2022 at 08:42, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2022-Sep-06, Simon Riggs wrote: > > > On Tue, 6 Sept 2022 at 17:19, Erik Rijkers <er@xs4all.nl> wrote: > > > > > > Op 06-09-2022 om 17:16 schreef Simon Riggs: > > > > New chapter on transaction management, plus a few related changes. > > > > > > > > Markup and links are not polished yet, so please comment initially on > > > > the topics, descriptions and wording. > > I think the concept of XID epoch should be described also, in or near > the paragraph that talks about wrapping around and switching between > int32 and int64. Right now it's a bit unclear why/how that works. Will do -- Simon Riggs http://www.EnterpriseDB.com/
On Wed, Sep 7, 2022 at 8:05 AM Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > On Tue, 6 Sept 2022 at 21:33, Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > On Tue, Sep 06, 2022 at 04:16:02PM +0100, Simon Riggs wrote: > > > New chapter on transaction management, plus a few related changes. > > > > > > Markup and links are not polished yet, so please comment initially on > > > the topics, descriptions and wording. > > > I've also added further notes about prepared transactions. > > I attach a diff against the original patch, plus a new clean copy. > Some feedback on the v4 patch, hopefully useful. +<para> +Transactions may be started explicitly using BEGIN and COMMIT commands, known as +a transaction block. A transaction will also be started and ended implicitly for +each request when outside of a transaction block. +</para> Transactions may be managed explicitly using BEGIN and COMMIT commands, known as a transaction block. +Committed subtransactions are recorded as committed if the main transaction +commits. The word subtransaction is often abbreviated to "subxact". +</para> Committed subtransactions are only recorded as committed if the main transaction commits, otherwise any work done in a subtransaction will be rolled back or aborted. The word subtransaction is often abbreviated as "subxact". +<para> +Subtransactions may be started explicitly by using the SAVEPOINT command, but may +also be started in other ways, such as PL/pgSQL's EXCEPTION clause. PL/Python and +PL/TCL also support explicit subtransactions. Working with the C API, users may +also call BeginInternalSubTransaction(). +</para> I think this paragraph (or something similar) should be the opening paragraph for this section, so that readers are immediately given context for what PostgreSQL considers to be a subtransaction. +prepared transactions that were prepared before the last checkpoint. In the typical +case, shorter-lived prepared transactions are stored only in shared memory and WAL. +Currently-prepared transactions can be inspected using the pg_prepared_xacts view. +</para> Transactions that are currently prepared can be inspected using the pg_prepated_xacts view. * I thought the hyphenated wording looked odd, though I understand why you used it. We don't use it elsewhere though (just `currently prepared` san hyphen) so re-worded to match the other wording. Robert Treat https://xzilla.net
On Sun, 11 Sept 2022 at 21:35, Robert Treat <rob@xzilla.net> wrote: > > On Wed, Sep 7, 2022 at 8:05 AM Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > > > On Tue, 6 Sept 2022 at 21:33, Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > > > On Tue, Sep 06, 2022 at 04:16:02PM +0100, Simon Riggs wrote: > > > > New chapter on transaction management, plus a few related changes. > > > > > > > > Markup and links are not polished yet, so please comment initially on > > > > the topics, descriptions and wording. > > > > > I've also added further notes about prepared transactions. > > > > I attach a diff against the original patch, plus a new clean copy. > > > > Some feedback on the v4 patch, hopefully useful. > > +<para> > +Transactions may be started explicitly using BEGIN and COMMIT > commands, known as > +a transaction block. A transaction will also be started and ended > implicitly for > +each request when outside of a transaction block. > +</para> > > Transactions may be managed explicitly using BEGIN and COMMIT commands, known as > a transaction block. > > > +Committed subtransactions are recorded as committed if the main transaction > +commits. The word subtransaction is often abbreviated to "subxact". > +</para> > > Committed subtransactions are only recorded as committed if the main > transaction commits, > otherwise any work done in a subtransaction will be rolled back or > aborted. The word subtransaction is > often abbreviated as "subxact". > > +<para> > +Subtransactions may be started explicitly by using the SAVEPOINT > command, but may > +also be started in other ways, such as PL/pgSQL's EXCEPTION clause. > PL/Python and > +PL/TCL also support explicit subtransactions. Working with the C API, users may > +also call BeginInternalSubTransaction(). > +</para> > > I think this paragraph (or something similar) should be the opening > paragraph for this section, so that readers are immediately given > context for what PostgreSQL considers to be a subtransaction. > > > +prepared transactions that were prepared before the last checkpoint. > In the typical > +case, shorter-lived prepared transactions are stored only in shared > memory and WAL. > +Currently-prepared transactions can be inspected using the > pg_prepared_xacts view. > +</para> > > Transactions that are currently prepared can be inspected using the > pg_prepated_xacts view. > > * I thought the hyphenated wording looked odd, though I understand why > you used it. We don't use it elsewhere though (just `currently > prepared` san hyphen) so re-worded to match the other wording. Thanks Robert. I've tried to accommodate all of your thoughts, plus Alvaro's. New v5 attached. Happy to receive further comments. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Tue, Sep 13, 2022 at 03:02:34PM +0100, Simon Riggs wrote: > Thanks Robert. I've tried to accommodate all of your thoughts, plus Alvaro's. > > New v5 attached. > > Happy to receive further comments. I liked this patch very much. It gives details on a lot of the internals we expose to users. Some of my changes were: * tightening the wording * restructuring the flow * splitting out user-visible details (prepared transactions) from internals, e.g., xid, vxid, subtransactions * adding references from places in our docs to these new sections I plan to apply this and backpatch it to all supported versions since these details apply to all versions. These docs should enable our users to much better understand and monitor Postgres. Updated patch attached. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
Op 13-10-2022 om 23:28 schreef Bruce Momjian: > On Tue, Sep 13, 2022 at 03:02:34PM +0100, Simon Riggs wrote: >> Thanks Robert. I've tried to accommodate all of your thoughts, plus Alvaro's. >> >> New v5 attached. >> >> Happy to receive further comments. > > I liked this patch very much. It gives details on a lot of the > internals we expose to users. Some of my changes were: > > * tightening the wording > * restructuring the flow > * splitting out user-visible details (prepared transactions) from > internals, e.g., xid, vxid, subtransactions > * adding references from places in our docs to these new sections > [xact.diff] I think that 'This chapter explains how the control the reliability of' should be: 'This chapter explains how to control the reliability of' And in these lines: + together in a transactional manner. The commands <command>PREPARE + TRANSACTION</command>, <command>COMMIT PREPARED</command> and + <command>ROLLBACK PREPARED</command>. Two-phase transactions 'The commands' should be 'The commands are' thanks, Erik Rijkers > I plan to apply this and backpatch it to all supported versions since > these details apply to all versions. These docs should enable our users > to much better understand and monitor Postgres. > > Updated patch attached.
On Thu, Oct 13, 2022 at 11:54:51PM +0200, Erik Rijkers wrote:
> > [xact.diff]
>
> I think that
> 'This chapter explains how the control the reliability of'
>
> should be:
> 'This chapter explains how to control the reliability of'
>
>
> And in these lines:
> + together in a transactional manner. The commands <command>PREPARE
> + TRANSACTION</command>, <command>COMMIT PREPARED</command> and
> + <command>ROLLBACK PREPARED</command>. Two-phase transactions
>
> 'The commands'
>
> should be
> 'The commands are'
Thanks, updated patch attached. You can see the output at:
https://momjian.us/tmp/pgsql/
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
On Thu, 13 Oct 2022 at 23:06, Bruce Momjian <bruce@momjian.us> wrote: > > Thanks, updated patch attached. You can see the output at: Thank you for your work to tighten and cleanup this patch, much appreciated. I had two minor typos, plus a slight rewording to avoid using the word "global" in the last section, since that is associated with distributed or 2PC transactions. For those changes, I provide a patch-on-patch so you can see clearly. In related changes, I've also done some major rewording of the RELEASE SAVEPOINT command, since it was incorrectly described as having "no other user visible behavior". A complex example is given to explain, using the terminology established in the main patch. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
+1 for this new chapter. This latest patch looks pretty good. I think that introducing the concept of "sub-commit" as in Simon's follow-up patch clarifies things, though the word itself looks very odd. Maybe it's okay. The addition of the savepoint example looks good also. On 2022-Oct-13, Bruce Momjian wrote: > + <para> > + <productname>PostgreSQL</productname> supports a two-phase commit (2PC) > + protocol that allows multiple distributed systems to work together > + in a transactional manner. The commands are <command>PREPARE > + TRANSACTION</command>, <command>COMMIT PREPARED</command> and I suggest/request that we try to avoid breaking tagged constants in DocBook; doing so makes it much easier to miss them later when grepping for them (don't laugh, it has happened to me). Also, it breaks formatting in some weird cases. I know this makes editing a bit harder because you can't just reflow with your editor like you would normal text. So this'd be: + in a transactional manner. The commands are <command>PREPARE TRANSACTION</command>, + <command>COMMIT PREPARED</command> and with whatever word wrapping you like, except breaking between PREPARE and TRANSACTION. > + <para> > + In addition to <literal>vxid</literal> and <type>xid</type>, > + when a transaction is prepared it is also identified by a Global > + Transaction Identifier (<acronym>GID</acronym>). GIDs > + are string literals up to 200 bytes long, which must be > + unique amongst other currently prepared transactions. > + The mapping of GID to xid is shown in <link > + linkend="view-pg-prepared-xacts"><structname>pg_prepared_xacts</structname></link>. > + </para> Maybe say "is prepared for two-phase commit", to make the topic of this paragraph more obvious? > + <para> > + 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>. > + 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. > + </para> Hmm, ok. > + <para> > + The parent xid of each subxid is recorded in the > + <filename>pg_subtrans</filename> directory. No entry is made for > + top-level xids since they do not have a parent, nor is an entry made > + for read-only subtransactions. > + </para> Maybe say "the immediate parent xid of each ...", or is it too obvious? -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Las cosas son buenas o malas segun las hace nuestra opinión" (Lisias)
On Fri, 14 Oct 2022 at 09:46, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > +1 for this new chapter. This latest patch looks pretty good. I think > that introducing the concept of "sub-commit" as in Simon's follow-up > patch clarifies things, though the word itself looks very odd. Maybe > it's okay. The addition of the savepoint example looks good also. > > On 2022-Oct-13, Bruce Momjian wrote: > > > + <para> > > + <productname>PostgreSQL</productname> supports a two-phase commit (2PC) > > + protocol that allows multiple distributed systems to work together > > + in a transactional manner. The commands are <command>PREPARE > > + TRANSACTION</command>, <command>COMMIT PREPARED</command> and > > I suggest/request that we try to avoid breaking tagged constants in > DocBook; doing so makes it much easier to miss them later when grepping > for them (don't laugh, it has happened to me). Also, it breaks > formatting in some weird cases. I know this makes editing a bit harder > because you can't just reflow with your editor like you would normal > text. So this'd be: > > + in a transactional manner. The commands are <command>PREPARE TRANSACTION</command>, > + <command>COMMIT PREPARED</command> and > > with whatever word wrapping you like, except breaking between PREPARE > and TRANSACTION. > > > + <para> > > + In addition to <literal>vxid</literal> and <type>xid</type>, > > + when a transaction is prepared it is also identified by a Global > > + Transaction Identifier (<acronym>GID</acronym>). GIDs > > + are string literals up to 200 bytes long, which must be > > + unique amongst other currently prepared transactions. > > + The mapping of GID to xid is shown in <link > > + linkend="view-pg-prepared-xacts"><structname>pg_prepared_xacts</structname></link>. > > + </para> > > Maybe say "is prepared for two-phase commit", to make the topic of this > paragraph more obvious? > > > + <para> > > + 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>. > > + 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. > > + </para> > > Hmm, ok. > > > + <para> > > + The parent xid of each subxid is recorded in the > > + <filename>pg_subtrans</filename> directory. No entry is made for > > + top-level xids since they do not have a parent, nor is an entry made > > + for read-only subtransactions. > > + </para> > > Maybe say "the immediate parent xid of each ...", or is it too obvious? +1 to all of those suggestions -- Simon Riggs http://www.EnterpriseDB.com/
On Fri, 14 Oct 2022 at 08:55, Simon Riggs <simon.riggs@enterprisedb.com> wrote: > In related changes, I've also done some major rewording of the RELEASE > SAVEPOINT command, since it was incorrectly described as having "no > other user visible behavior". A complex example is given to explain, > using the terminology established in the main patch. ROLLBACK TO SAVEPOINT also needs some clarification, patch attached. (Commentary: It's confusing to me that ROLLBACK TO SAVEPOINT starts a new subtransaction, whereas RELEASE SAVEPOINT does not. You might imagine they would both start a new subtransaction, but that is not the case.) -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Fri, Oct 14, 2022 at 08:55:05AM +0100, Simon Riggs wrote: > On Thu, 13 Oct 2022 at 23:06, Bruce Momjian <bruce@momjian.us> wrote: > > > > Thanks, updated patch attached. You can see the output at: > > Thank you for your work to tighten and cleanup this patch, much appreciated. > > I had two minor typos, plus a slight rewording to avoid using the word > "global" in the last section, since that is associated with > distributed or 2PC transactions. For those changes, I provide a > patch-on-patch so you can see clearly. Yes, I didn't like global either --- I like your wording. I added your other changes too, with slight rewording. Merged patch to be posted in a later email. > In related changes, I've also done some major rewording of the RELEASE > SAVEPOINT command, since it was incorrectly described as having "no > other user visible behavior". A complex example is given to explain, > using the terminology established in the main patch. Okay, added. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
On Fri, Oct 14, 2022 at 01:05:14PM +0100, Simon Riggs wrote: > On Fri, 14 Oct 2022 at 08:55, Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > > In related changes, I've also done some major rewording of the RELEASE > > SAVEPOINT command, since it was incorrectly described as having "no > > other user visible behavior". A complex example is given to explain, > > using the terminology established in the main patch. > > ROLLBACK TO SAVEPOINT also needs some clarification, patch attached. > > (Commentary: It's confusing to me that ROLLBACK TO SAVEPOINT starts a > new subtransaction, whereas RELEASE SAVEPOINT does not. You might > imagine they would both start a new subtransaction, but that is not > the case.) Agreed, added. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
On Fri, Oct 14, 2022 at 10:46:15AM +0200, Álvaro Herrera wrote: > +1 for this new chapter. This latest patch looks pretty good. I think > that introducing the concept of "sub-commit" as in Simon's follow-up > patch clarifies things, though the word itself looks very odd. Maybe > it's okay. The addition of the savepoint example looks good also. Yes, I like that term since it isn't a permament commit. > On 2022-Oct-13, Bruce Momjian wrote: > > > + <para> > > + <productname>PostgreSQL</productname> supports a two-phase commit (2PC) > > + protocol that allows multiple distributed systems to work together > > + in a transactional manner. The commands are <command>PREPARE > > + TRANSACTION</command>, <command>COMMIT PREPARED</command> and > > I suggest/request that we try to avoid breaking tagged constants in > DocBook; doing so makes it much easier to miss them later when grepping > for them (don't laugh, it has happened to me). Also, it breaks > formatting in some weird cases. I know this makes editing a bit harder > because you can't just reflow with your editor like you would normal > text. So this'd be: > > + in a transactional manner. The commands are <command>PREPARE TRANSACTION</command>, > + <command>COMMIT PREPARED</command> and > > with whatever word wrapping you like, except breaking between PREPARE > and TRANSACTION. Uh, I do a lot of word wraps and I don't think I can reaonably avoid these splits. > > > + <para> > > + In addition to <literal>vxid</literal> and <type>xid</type>, > > + when a transaction is prepared it is also identified by a Global > > + Transaction Identifier (<acronym>GID</acronym>). GIDs > > + are string literals up to 200 bytes long, which must be > > + unique amongst other currently prepared transactions. > > + The mapping of GID to xid is shown in <link > > + linkend="view-pg-prepared-xacts"><structname>pg_prepared_xacts</structname></link>. > > + </para> > > Maybe say "is prepared for two-phase commit", to make the topic of this > paragraph more obvious? Agreed. > > + <para> > > + The parent xid of each subxid is recorded in the > > + <filename>pg_subtrans</filename> directory. No entry is made for > > + top-level xids since they do not have a parent, nor is an entry made > > + for read-only subtransactions. > > + </para> > > Maybe say "the immediate parent xid of each ...", or is it too obvious? Agreed with your wording. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
On Fri, Oct 14, 2022 at 12:22:35PM +0100, Simon Riggs wrote:
> > > + <para>
> > > + The parent xid of each subxid is recorded in the
> > > + <filename>pg_subtrans</filename> directory. No entry is made for
> > > + top-level xids since they do not have a parent, nor is an entry made
> > > + for read-only subtransactions.
> > > + </para>
> >
> > Maybe say "the immediate parent xid of each ...", or is it too obvious?
>
> +1 to all of those suggestions
Attached is the merged patch from all the great comments I received. I
have also rebuilt the docs with the updated patch:
https://momjian.us/tmp/pgsql/
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
On Fri, Oct 14, 2022 at 3:51 PM Bruce Momjian <bruce@momjian.us> wrote: > Attached is the merged patch from all the great comments I received. I > have also rebuilt the docs with the updated patch: > > https://momjian.us/tmp/pgsql/ > + <command>RELEASE SAVEPOINT</command> also subcommits and destroys + all savepoints that were established after the named savepoint was + established. This means that any subtransactions of the named savepoint + will also be subcommitted and destroyed. Wonder if we should be more explicit that data changes are preserved, not destroyed... something like: "This means that any changes within subtransactions of the named savepoint will be subcommitted and those subtransactions will be destroyed." Robert Treat https://xzilla.net
On Fri, Oct 14, 2022 at 05:46:55PM -0400, Robert Treat wrote: > On Fri, Oct 14, 2022 at 3:51 PM Bruce Momjian <bruce@momjian.us> wrote: > > Attached is the merged patch from all the great comments I received. I > > have also rebuilt the docs with the updated patch: > > > > https://momjian.us/tmp/pgsql/ > > > > + <command>RELEASE SAVEPOINT</command> also subcommits and destroys > + all savepoints that were established after the named savepoint was > + established. This means that any subtransactions of the named savepoint > + will also be subcommitted and destroyed. > > Wonder if we should be more explicit that data changes are preserved, > not destroyed... something like: > "This means that any changes within subtransactions of the named > savepoint will be subcommitted and those subtransactions will be > destroyed." Good point. I reread the section and there was just too much confusion over subtransactions, partly because the behavior just doesn't map easily to subtransaction. I therefore merged all three paragraphs into one and tried to make the text saner; release_savepoint.sgml diff attached, URL content updated. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
On Sun, 16 Oct 2022 at 02:08, Bruce Momjian <bruce@momjian.us> wrote: > > On Fri, Oct 14, 2022 at 05:46:55PM -0400, Robert Treat wrote: > > On Fri, Oct 14, 2022 at 3:51 PM Bruce Momjian <bruce@momjian.us> wrote: > > > Attached is the merged patch from all the great comments I received. I > > > have also rebuilt the docs with the updated patch: > > > > > > https://momjian.us/tmp/pgsql/ > > > > > > > + <command>RELEASE SAVEPOINT</command> also subcommits and destroys > > + all savepoints that were established after the named savepoint was > > + established. This means that any subtransactions of the named savepoint > > + will also be subcommitted and destroyed. > > > > Wonder if we should be more explicit that data changes are preserved, > > not destroyed... something like: > > "This means that any changes within subtransactions of the named > > savepoint will be subcommitted and those subtransactions will be > > destroyed." > > Good point. I reread the section and there was just too much confusion > over subtransactions, partly because the behavior just doesn't map > easily to subtransaction. I therefore merged all three paragraphs into > one and tried to make the text saner; release_savepoint.sgml diff > attached, URL content updated. Just got around to reading this, thanks for changes. The rewording doesn't work for me. The use of the word "destroy" is very misleading, since the effect is to commit. So I think we must avoid use of the word destroy completely. Possible rewording: <command>RELEASE SAVEPOINT</command> will subcommit the subtransaction established by the named savepoint, releasing any resources held by it. If there were any subtransactions created by the named savepoint, these will also be subcommitted. The point is that savepoints create subtransactions, but they are not the only way to create them, so we cannot equate savepoint and subtransaction completely. -- Simon Riggs http://www.EnterpriseDB.com/
On Mon, Oct 24, 2022 at 11:02 AM Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > On Sun, 16 Oct 2022 at 02:08, Bruce Momjian <bruce@momjian.us> wrote: > > > > On Fri, Oct 14, 2022 at 05:46:55PM -0400, Robert Treat wrote: > > > On Fri, Oct 14, 2022 at 3:51 PM Bruce Momjian <bruce@momjian.us> wrote: > > > > Attached is the merged patch from all the great comments I received. I > > > > have also rebuilt the docs with the updated patch: > > > > > > > > https://momjian.us/tmp/pgsql/ > > > > > > > > > > + <command>RELEASE SAVEPOINT</command> also subcommits and destroys > > > + all savepoints that were established after the named savepoint was > > > + established. This means that any subtransactions of the named savepoint > > > + will also be subcommitted and destroyed. > > > > > > Wonder if we should be more explicit that data changes are preserved, > > > not destroyed... something like: > > > "This means that any changes within subtransactions of the named > > > savepoint will be subcommitted and those subtransactions will be > > > destroyed." > > > > Good point. I reread the section and there was just too much confusion > > over subtransactions, partly because the behavior just doesn't map > > easily to subtransaction. I therefore merged all three paragraphs into > > one and tried to make the text saner; release_savepoint.sgml diff > > attached, URL content updated. > > Just got around to reading this, thanks for changes. > > The rewording doesn't work for me. The use of the word "destroy" is > very misleading, since the effect is to commit. > > So I think we must avoid use of the word destroy completely. Possible > rewording: > > <command>RELEASE SAVEPOINT</command> will subcommit the subtransaction > established by the named savepoint, releasing any resources held by > it. If there were any subtransactions created by the named savepoint, > these will also be subcommitted. > I think it should be "If there were any subtransactions of the named savepoint, these will also be subcommitted", but otherwise I think this wording should work. Robert Treat https://xzilla.net
On Sat, 2022-10-15 at 21:08 -0400, Bruce Momjian wrote: > I therefore merged all three paragraphs into > one and tried to make the text saner; release_savepoint.sgml diff > attached, URL content updated. I wanted to have a look at this, but I am confused. The original patch was much bigger. Is this just an incremental patch? If yes, it would be nice to have a "grand total" patch, so that I can read it all in one go. Yours, Laurenz Albe
On Fri, 4 Nov 2022 at 15:17, Laurenz Albe <laurenz.albe@cybertec.at> wrote: > > On Sat, 2022-10-15 at 21:08 -0400, Bruce Momjian wrote: > > I therefore merged all three paragraphs into > > one and tried to make the text saner; release_savepoint.sgml diff > > attached, URL content updated. > > I wanted to have a look at this, but I am confused. The original patch > was much bigger. Is this just an incremental patch? If yes, it would > be nice to have a "grand total" patch, so that I can read it all > in one go. Agreed; new compilation patch attached, including mine and then Robert's suggested rewordings. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Fri, Nov 4, 2022 at 04:17:28PM +0100, Laurenz Albe wrote:
> On Sat, 2022-10-15 at 21:08 -0400, Bruce Momjian wrote:
> > I therefore merged all three paragraphs into
> > one and tried to make the text saner; release_savepoint.sgml diff
> > attached, URL content updated.
>
> I wanted to have a look at this, but I am confused. The original patch
> was much bigger. Is this just an incremental patch? If yes, it would
> be nice to have a "grand total" patch, so that I can read it all
> in one go.
Yeah, I said:
Yes, I didn't like global either --- I like your wording. I added your
other changes too, with slight rewording. Merged patch to be posted in
-------------------------
a later email.
but that was unclear. Let me post one now, and Simon posted one too.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
On Mon, 7 Nov 2022 at 07:43, Bruce Momjian <bruce@momjian.us> wrote: > > On Fri, Nov 4, 2022 at 04:17:28PM +0100, Laurenz Albe wrote: > > On Sat, 2022-10-15 at 21:08 -0400, Bruce Momjian wrote: > > > I therefore merged all three paragraphs into > > > one and tried to make the text saner; release_savepoint.sgml diff > > > attached, URL content updated. > > > > I wanted to have a look at this, but I am confused. The original patch > > was much bigger. Is this just an incremental patch? If yes, it would > > be nice to have a "grand total" patch, so that I can read it all > > in one go. > > Yeah, I said: > > Yes, I didn't like global either --- I like your wording. I added your > other changes too, with slight rewording. Merged patch to be posted in > ------------------------- > a later email. > > but that was unclear. Let me post one now, and Simon posted one too. What I've posted is the merged patch, i.e. your latest patch, plus changes to RELEASE SAVEPOINT from you on Oct 16, plus changes based on the later comments from Robert and I. -- Simon Riggs http://www.EnterpriseDB.com/
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
On Mon, 2022-11-07 at 23:04 +0100, Laurenz Albe wrote: > 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: [...] I thought some more about the patch, and I don't like the title "Transaction Management" for the new chapter. I'd expect some more from a chapter titled "Internals" / "Transaction Management". In reality, the new chapter is about transaction IDs. So perhaps the name should reflect that, so that it does not mislead the reader. Yours, Laurenz Albe
On Mon, Nov 7, 2022 at 10:58:05AM +0000, Simon Riggs wrote: > What I've posted is the merged patch, i.e. your latest patch, plus > changes to RELEASE SAVEPOINT from you on Oct 16, plus changes based on > the later comments from Robert and I. Thanks. I have two changes to your patch. First, I agree "destroy" is the wrong word for this, but I don't think "subcommit" is good, for three reasons: 1. Release merges the non-aborted changes into the previous transaction _and_ frees their resources --- "subcommit" doesn't have both meanings, which I think means if we need a single word, we should use "release" and later define what that means. 2. The "subcommit" concept doesn't closely match the user-visible behavior, even though we use subtransactions to accomplish this. Release is more of a rollup/merge into the previously-active transaction/savepoint. 3. "subcommit" is an implementation detail that I don't think we should expose to users in the manual pages. I adjusted the first paragraph of RELEASE SAVEPOINT to highlight the above issues. My original patch had similar wording. The first attachment shows my changes to your patch, and the second attachment is my full patch. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
On Tue, Nov 8, 2022 at 04:37:59AM +0100, Laurenz Albe wrote: > On Mon, 2022-11-07 at 23:04 +0100, Laurenz Albe wrote: > > 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: [...] > > I thought some more about the patch, and I don't like the title > "Transaction Management" for the new chapter. I'd expect some more > from a chapter titled "Internals" / "Transaction Management". > > In reality, the new chapter is about transaction IDs. So perhaps the > name should reflect that, so that it does not mislead the reader. I renamed it to "Transaction Processing" since we also cover locking and subtransactions. How is that? -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
On Mon, 2022-11-07 at 22:43 -0500, Bruce Momjian wrote: > > I thought some more about the patch, and I don't like the title > > "Transaction Management" for the new chapter. I'd expect some more > > from a chapter titled "Internals" / "Transaction Management". > > > > In reality, the new chapter is about transaction IDs. So perhaps the > > name should reflect that, so that it does not mislead the reader. > > I renamed it to "Transaction Processing" since we also cover locking and > subtransactions. How is that? It is better. Did you take my suggestions from [1] into account in your latest cumulative patch in [2]? Otherwise, it will be difficult to integrate both. Yours, Laurenz Albe [1]: https://postgr.es/m/3603e6e85544daa5300c7106c31bc52673711cd0.camel%40cybertec.at [2]: https://postgr.es/m/Y2nP04/3BHQOviVB%40momjian.us
On Mon, Nov 7, 2022 at 5:04 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote: > > 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: > <snip> > > --- 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". > Personally, I think these are more confusing. > > --- 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? > I thought about this as well when reviewing it, but I do think something is needed for the case where you have a transaction which has suffered an error and then you issue "rollback and chain"; if you just say "a new transaction is immediately started with the same transaction characteristics" it might imply to some the new transaction has some kind of carry over of the previous broken transaction... the use of the word unaborted makes it clear that the new transaction is 100% functional. > > --- 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 seems to align with other usage, so +1 > 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" > +1 > 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." > +1 > > + 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". > Minor aside, we should clean up glossary.sgml as well. > > + 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. > Agreed the distinction is not clear, although you could replace Hot Standby with Warm Standby. > > + 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." > +1 > > + <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. > ISTR that you only use a comma before since in cases where the preceding thought contains a negative. In any case, are you thinking something like this: " 64 open subxids are cached in shared memory for each backend; after that point the overhead increases significantly due to additional disk I/O from looking up subxid entries in <filename>pg_subtrans</filename>." Robert Treat https://xzilla.net
On Wed, 2022-11-09 at 09:16 -0500, Robert Treat wrote:
> On Mon, Nov 7, 2022 at 5:04 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > Some comments:
> >
> <snip>
> > > --- 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".
>
> Personally, I think these are more confusing.
Perhaps. I was worried because everywhere else, the wording makes a clear distinction
between a savepoint and the subtransaction created by a savepoint. But perhaps some
sloppiness is better to avoid such word cascades.
> > > --- 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?
>
> I thought about this as well when reviewing it, but I do think
> something is needed for the case where you have a transaction which
> has suffered an error and then you issue "rollback and chain"; if you
> just say "a new transaction is immediately started with the same
> transaction characteristics" it might imply to some the new
> transaction has some kind of carry over of the previous broken
> transaction... the use of the word unaborted makes it clear that the
> new transaction is 100% functional.
A new transaction is never aborted in my understanding. Being aborted is not a
characteristic of a transaction, but a state.
> >
>
>
> > > + 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".
>
> Minor aside, we should clean up glossary.sgml as well.
Right, it has this:
The numerical, unique, sequentially-assigned identifier that each
transaction receives when it first causes a database modification.
Frequently abbreviated as <firstterm>xid</firstterm>.
When stored on disk, xids are only 32-bits wide, so only
approximately four billion write transaction IDs can be generated;
to permit the system to run for longer than that,
<firstterm>epochs</firstterm> are used, also 32 bits wide.
Which reminds me that I should have suggested <firstterm> rather than
<quote> where I complained about the use of <literal>.
> >
> > + 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.
>
> ISTR that you only use a comma before since in cases where the
> preceding thought contains a negative.
Not being a native speaker, I'll leave that to those who are; I went by feeling.
> In any case, are you thinking something like this:
>
> " 64 open subxids are cached in shared memory for each backend; after
> that point the overhead increases significantly due to additional disk I/O
> from looking up subxid entries in <filename>pg_subtrans</filename>."
Yes.
Yours,
Laurenz Albe
On 2022-Nov-10, Laurenz Albe wrote: > On Wed, 2022-11-09 at 09:16 -0500, Robert Treat wrote: > > > > - 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? > > > > I thought about this as well when reviewing it, but I do think > > something is needed for the case where you have a transaction which > > has suffered an error and then you issue "rollback and chain"; if you > > just say "a new transaction is immediately started with the same > > transaction characteristics" it might imply to some the new > > transaction has some kind of carry over of the previous broken > > transaction... the use of the word unaborted makes it clear that the > > new transaction is 100% functional. > > A new transaction is never aborted in my understanding. Being aborted > is not a characteristic of a transaction, but a state. I agree, but maybe it's good to make the point explicit, because it doesn't seem obvious. Perhaps something like "If X is specified, a new transaction (never in aborted state) is immediately started with the same transaction characteristics (see X) as the just finished one. Otherwise ..." Getting the wording of that parenthical comment right is tricky, though. What I propose above is not great, but I don't know how to make it better. Other ideas that seem slightly worse but may inspire someone: ... a new transaction (which is never in aborted state) is ... ... a new transaction (not in aborted state) is ... ... a new transaction (never aborted, even if the previous is) is ... ... a new (not-aborted) transaction is ... ... a new (never aborted) transaction is ... ... a new (never aborted, even if the previous is) transaction is ... ... a new (never aborted, regardless of the status of the previous one) transaction is ... Maybe there's a way to reword the entire phrase that leads to a better formulation of the idea. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On Mon, 7 Nov 2022 at 22:04, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> 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".
Agreed.
Lock waits on table-locks are shown waiting for a lock type of
<literal>relation</literal>,
while lock waits on row-level locks are shown waiting for a lock type
of <literal>transactionid</literal>.
Table-level locks require only a virtualxid when the lock is less than an
AccessExclusiveLock; in other cases an xid must be allocated.
--
Simon Riggs http://www.EnterpriseDB.com/
On Thu, 2022-11-10 at 12:17 +0100, Alvaro Herrera wrote: > On 2022-Nov-10, Laurenz Albe wrote: > > On Wed, 2022-11-09 at 09:16 -0500, Robert Treat wrote: > > > > > - 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. > > > > A new transaction is never aborted in my understanding. Being aborted > > is not a characteristic of a transaction, but a state. > > I agree, but maybe it's good to make the point explicit, because it > doesn't seem obvious. Perhaps something like > > "If X is specified, a new transaction (never in aborted state) is > immediately started with the same transaction characteristics (see X) as > the just finished one. Otherwise ..." > > Getting the wording of that parenthical comment right is tricky, though. > What I propose above is not great, but I don't know how to make it > better. Other ideas that seem slightly worse but may inspire someone: > > ... a new transaction (which is never in aborted state) is ... > ... a new transaction (not in aborted state) is ... > ... a new transaction (never aborted, even if the previous is) is ... > ... a new (not-aborted) transaction is ... > ... a new (never aborted) transaction is ... > ... a new (never aborted, even if the previous is) transaction is ... > ... a new (never aborted, regardless of the status of the previous one) transaction is ... > > > Maybe there's a way to reword the entire phrase that leads to a better > formulation of the idea. Any of your auggestions is better than "unaborted". How about: If <literal>AND CHAIN</literal> is specified, a new transaction is immediately started with the same transaction characteristics (see <xref linkend="sql-set-transaction"/>) as the just finished one. This new transaction won't be in the <quote>aborted</quote> state, even if the old transaction was aborted. Yours, Laurenz Albe
On Tue, 8 Nov 2022 at 03:41, Bruce Momjian <bruce@momjian.us> wrote: > > On Mon, Nov 7, 2022 at 10:58:05AM +0000, Simon Riggs wrote: > > What I've posted is the merged patch, i.e. your latest patch, plus > > changes to RELEASE SAVEPOINT from you on Oct 16, plus changes based on > > the later comments from Robert and I. > > Thanks. I have two changes to your patch. First, I agree "destroy" is > the wrong word for this, but I don't think "subcommit" is good, for > three reasons: > > 1. Release merges the non-aborted changes into the previous transaction > _and_ frees their resources --- "subcommit" doesn't have both meanings, > which I think means if we need a single word, we should use "release" > and later define what that means. > > 2. The "subcommit" concept doesn't closely match the user-visible > behavior, even though we use subtransactions to accomplish this. Release > is more of a rollup/merge into the previously-active > transaction/savepoint. > > 3. "subcommit" is an implementation detail that I don't think we should > expose to users in the manual pages. I don't understand this - you seem to be presuming that "subcommit" means something different and then objecting to that difference. For me, "Subcommit" exactly matches what is happening because the code comments and details already use Subcommit in exactly this way. The main purpose of this patch is to talk about what is happening using the same language as we do in the code. The gap between the code and the docs isn't helping anyone. > I adjusted the first paragraph of RELEASE SAVEPOINT to highlight the > above issues. My original patch had similar wording. > > The first attachment shows my changes to your patch, and the second > attachment is my full patch. OK, though this makes the patch tester look like this doesn't apply. -- Simon Riggs http://www.EnterpriseDB.com/
On Mon, Nov 7, 2022 at 11:04:46PM +0100, Laurenz Albe wrote:
> 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.
Sorry again for the long delay in replying to 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".
Agreed.
> > @@ -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.
Agreed.
> > @@ -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.
Done.
> > --- 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".
This paragraph has been rewritten to:
<command>RELEASE SAVEPOINT</command> releases the named savepoint and
all active savepoints that were created after the named savepoint,
and frees their resources. All changes made since the creation of the
savepoint, excluding rolled back savepoints changes, are merged into
the transaction or savepoint that was active when the named savepoint
was created. Changes made after <command>RELEASE SAVEPOINT</command>
will also be part of this active transaction or savepoint.
> > @@ -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 .../>."
Changed to:
It is not possible to release a savepoint when the transaction is in
an aborted state; to do that, use <xref linkend="sql-rollback-to"/>.
> > --- 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?
Agreed.
> > --- 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..."
Yes, now:
Transactions that are currently prepared can be inspected using <link
linkend="view-pg-prepared-xacts"><structname>pg_prepared_xacts</structname></link>.
> 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.
Agreed, moved to xact.sgml.
> > --- /dev/null
> > +++ b/doc/src/sgml/xact.sgml
>
> + <title>Transaction Management</title>
>
> + The word transaction is often abbreviated as "xact".
>
> Should use <quote> here.
Done.
> > + <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."
I rewrote the paragraph to be:
Non-virtual <literal>TransactionId</literal> (or <type>xid</type>),
e.g., <literal>278394</literal>, are assigned sequentially to
transactions from a global counter used by all databases within
the <productname>PostgreSQL</productname> cluster. This assignment
happens when a transaction first writes to the database. This means
lower-numbered xids started writing before higher-numbered xids.
Note that the order in which transactions perform their first database
write might be different from the order in which the transactions
started, particularly if the transaction started with statements that
only performed database reads.
> > + 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".
Done.
> > + A 32-bit epoch is incremented during each
> > + wrap around.
>
> We usually call this "wraparound" without a space.
Fixed.
> > + 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."
I went with:
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; it can be converted to xid 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.
Agreed, I went with:
Xids are used as the
basis for <productname>PostgreSQL</productname>'s <link
linkend="mvcc">MVCC</link> concurrency mechanism and streaming
replication.
> > + 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."
I went with:
In addition to <literal>vxid</literal> and <type>xid</type>,
prepared transactions are also assigned Global Transaction
Identifiers (<acronym>GID</acronym>).
> > + <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>."
Done.
> > + 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".
I tested and you are right. I went with more generic wording:
Some lock types wait on <structfield>virtualxid</structfield>,
while other types wait on <structfield>transactionid</structfield>.
> > + 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"
Done.
> 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.
Done with:
Row-level read locks might also require the assignment
of multixact IDs (<literal>mxid</literal>; see <xref
linkend="vacuum-for-multixact-wraparound"/>).
> > + <sect1 id="subxacts">
> > +
> > + <title>Subtransactions</title>
>
> > + The word subtransaction is often abbreviated as
> > + <literal>subxact</literal>.
>
> I'd use <quote>, not <literal>.
Done.
> > + 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.
Done.
> + 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.
I went with:
Up to 64 open subxids are cached in shared memory for each backend; after
that point, the storage I/O overhead increases significantly, since
we must look up subxid entries in <filename>pg_subtrans</filename>.
Updated full patch attached.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
On Wed, Nov 9, 2022 at 09:16:18AM -0500, Robert Treat wrote:
> > "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".
> >
>
> Personally, I think these are more confusing.
That text is gone.
> > > --- 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?
> >
>
> I thought about this as well when reviewing it, but I do think
> something is needed for the case where you have a transaction which
> has suffered an error and then you issue "rollback and chain"; if you
> just say "a new transaction is immediately started with the same
> transaction characteristics" it might imply to some the new
> transaction has some kind of carry over of the previous broken
> transaction... the use of the word unaborted makes it clear that the
> new transaction is 100% functional.
I changed it to:
a new (unaborted) transaction is immediately started
> ISTR that you only use a comma before since in cases where the
> preceding thought contains a negative.
>
> In any case, are you thinking something like this:
>
> " 64 open subxids are cached in shared memory for each backend; after
> that point the overhead increases significantly due to additional disk I/O
> from looking up subxid entries in <filename>pg_subtrans</filename>."
I went with:
Up to 64 open subxids are cached in shared memory for
each backend; after that point, the storage I/O overhead increases
significantly due to additional lookups of subxid entries in
<filename>pg_subtrans</filename>.
New patch attached.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
On Thu, Nov 10, 2022 at 08:39:29AM +0100, Laurenz Albe wrote: > > > I don't think that is an improvement. "Unaborted" is an un-word. A new transaction > > > is always "unaborted", isn't it? > > > > I thought about this as well when reviewing it, but I do think > > something is needed for the case where you have a transaction which > > has suffered an error and then you issue "rollback and chain"; if you > > just say "a new transaction is immediately started with the same > > transaction characteristics" it might imply to some the new > > transaction has some kind of carry over of the previous broken > > transaction... the use of the word unaborted makes it clear that the > > new transaction is 100% functional. > > A new transaction is never aborted in my understanding. Being aborted is not a > characteristic of a transaction, but a state. I used "(unaborted)", which seems to be a compromise. > > > > + 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". > > > > Minor aside, we should clean up glossary.sgml as well. > > Right, it has this: > > The numerical, unique, sequentially-assigned identifier that each > transaction receives when it first causes a database modification. > Frequently abbreviated as <firstterm>xid</firstterm>. > When stored on disk, xids are only 32-bits wide, so only > approximately four billion write transaction IDs can be generated; > to permit the system to run for longer than that, > <firstterm>epochs</firstterm> are used, also 32 bits wide. > > Which reminds me that I should have suggested <firstterm> rather than > <quote> where I complained about the use of <literal>. I changed them to "firstterm". -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
On Sun, Nov 13, 2022 at 12:56:30PM +0100, Laurenz Albe wrote: > > Maybe there's a way to reword the entire phrase that leads to a better > > formulation of the idea. > > Any of your auggestions is better than "unaborted". > > How about: > > If <literal>AND CHAIN</literal> is specified, a new transaction is > immediately started with the same transaction characteristics (see <xref > linkend="sql-set-transaction"/>) as the just finished one. > This new transaction won't be in the <quote>aborted</quote> state, even > if the old transaction was aborted. I think I am going to keep "(unaborted)". -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
On Thu, Nov 10, 2022 at 11:31:25AM +0000, Simon Riggs wrote: > > 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". > > Agreed. > > Lock waits on table-locks are shown waiting for a lock type of > <literal>relation</literal>, > while lock waits on row-level locks are shown waiting for a lock type > of <literal>transactionid</literal>. > Table-level locks require only a virtualxid when the lock is less than an > AccessExclusiveLock; in other cases an xid must be allocated. Yeah, I went with more generic wording since the point seems to be that sometimes xid and sometimes vxids are waited on. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
On Tue, Nov 15, 2022 at 10:16:44AM +0000, Simon Riggs wrote: > On Tue, 8 Nov 2022 at 03:41, Bruce Momjian <bruce@momjian.us> wrote: > > > > On Mon, Nov 7, 2022 at 10:58:05AM +0000, Simon Riggs wrote: > > > What I've posted is the merged patch, i.e. your latest patch, plus > > > changes to RELEASE SAVEPOINT from you on Oct 16, plus changes based on > > > the later comments from Robert and I. > > > > Thanks. I have two changes to your patch. First, I agree "destroy" is > > the wrong word for this, but I don't think "subcommit" is good, for > > three reasons: > > > > 1. Release merges the non-aborted changes into the previous transaction > > _and_ frees their resources --- "subcommit" doesn't have both meanings, > > which I think means if we need a single word, we should use "release" > > and later define what that means. > > > > 2. The "subcommit" concept doesn't closely match the user-visible > > behavior, even though we use subtransactions to accomplish this. Release > > is more of a rollup/merge into the previously-active > > transaction/savepoint. > > > > 3. "subcommit" is an implementation detail that I don't think we should > > expose to users in the manual pages. > > I don't understand this - you seem to be presuming that "subcommit" > means something different and then objecting to that difference. > > For me, "Subcommit" exactly matches what is happening because the code > comments and details already use Subcommit in exactly this way. > > The main purpose of this patch is to talk about what is happening > using the same language as we do in the code. The gap between the code > and the docs isn't helping anyone. I didn't think that was the purpose, and certainly not in the reference/ref/man pages. I thought the purpose was to explain the behavior clearly, and in the "Internals" section, the internal API we expose to users. I didn't think matching the code was ever a goal --- I thought that is what the README files are for. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
On Fri, Nov 18, 2022 at 02:33:26PM -0500, Bruce Momjian wrote: > On Sun, Nov 13, 2022 at 12:56:30PM +0100, Laurenz Albe wrote: > > > Maybe there's a way to reword the entire phrase that leads to a better > > > formulation of the idea. > > > > Any of your auggestions is better than "unaborted". > > > > How about: > > > > If <literal>AND CHAIN</literal> is specified, a new transaction is > > immediately started with the same transaction characteristics (see <xref > > linkend="sql-set-transaction"/>) as the just finished one. > > This new transaction won't be in the <quote>aborted</quote> state, even > > if the old transaction was aborted. > > I think I am going to keep "(unaborted)". Attached is the most current version of the patch. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
On Fri, 2022-11-18 at 14:28 -0500, Bruce Momjian wrote: > New patch attached. Thanks. > --- a/doc/src/sgml/ref/release_savepoint.sgml > +++ b/doc/src/sgml/ref/release_savepoint.sgml > + <command>RELEASE SAVEPOINT</command> releases the named savepoint and > + all active savepoints that were created after the named savepoint, > + and frees their resources. All changes made since the creation of the > + savepoint, excluding rolled back savepoints changes, are merged into > + the transaction or savepoint that was active when the named savepoint > + was created. Changes made after <command>RELEASE SAVEPOINT</command> > + will also be part of this active transaction or savepoint. I am not sure if "rolled back savepoints changes" is clear enough. I understand that you are trying to avoid the term "subtransaction". How about: All changes made since the creation of the savepoint that didn't already get rolled back are merged ... > --- a/doc/src/sgml/ref/rollback.sgml > +++ b/doc/src/sgml/ref/rollback.sgml > > + If <literal>AND CHAIN</literal> is specified, a new (unaborted) *Sigh* I'll make one last plea for "not aborted". > --- /dev/null > +++ b/doc/src/sgml/xact.sgml > + <para> > + Transactions can be created explicitly using <command>BEGIN</command> > + and <command>COMMIT</command>, which creates a transaction block. > + An SQL statement outside of a transaction block automatically uses > + a single-statement transaction. > + </para> Sorry, I should have noted that the first time around. Transactions are not created using COMMIT. Perhaps we should also avoid the term "transaction block". Even without speaking of a "block", way too many people confuse PL/pgSQL's BEGIN ... END blocks with transactions. On the other hand, we use "transaction block" everywhere else in the documentation... How about: <para> Multi-statement transactions can be created explicitly using <command>BEGIN</command> or <command>START TRANSACTION</command> and are ended using <command>COMMIT</command> or <command>ROLLBACK</command>. An SQL statement outside of a transaction block automatically uses a single-statement transaction. </para> > + <sect1 id="xact-locking"> > + > + <title>Transactions and Locking</title> > + > + <para> > + The transaction IDs of 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>. Read-only transactions > + will have <structfield>virtualxid</structfield>s but NULL > + <structfield>transactionid</structfield>s, while read-write transactions > + will have both as non-NULL. > + </para> Perhaps the following will be prettier than "have both as non-NULL": ..., while both columns will be set in read-write transactions. Yours, Laurenz Albe
Agreed on not using "unaborted", per previous discussion. On 2022-Nov-21, Laurenz Albe wrote: > Perhaps we should also avoid the term "transaction block". Even without speaking > of a "block", way too many people confuse PL/pgSQL's BEGIN ... END blocks > with transactions. On the other hand, we use "transaction block" everywhere > else in the documentation... Yeah, I don't understand why we need this "transaction block" term at all. It adds nothing. We could just use the term "transaction", and little meaning would be lost. When necessary, we could just say "explicit transaction" or something to that effect. In this particular case, we could modify your proposed wording, > <para> > Multi-statement transactions can be created explicitly using > <command>BEGIN</command> or <command>START TRANSACTION</command> and > are ended using <command>COMMIT</command> or <command>ROLLBACK</command>. > An SQL statement outside of a transaction block automatically uses > a single-statement transaction. > </para> by removing the word "block": > Any SQL statement outside of an transaction automatically uses > a single-statement transaction. and perhaps add "explicit", but I don't think it's necessary: > Any SQL statement outside of an explicit transaction automatically > uses a single-statement transaction. (I also changed "An" to "Any" because it seems more natural, but I suppose it's a stylistic choice.) -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Mon, Nov 21, 2022 at 11:15:36AM +0100, Laurenz Albe wrote:
> > --- a/doc/src/sgml/ref/release_savepoint.sgml
> > +++ b/doc/src/sgml/ref/release_savepoint.sgml
>
> > + <command>RELEASE SAVEPOINT</command> releases the named savepoint and
> > + all active savepoints that were created after the named savepoint,
> > + and frees their resources. All changes made since the creation of the
> > + savepoint, excluding rolled back savepoints changes, are merged into
> > + the transaction or savepoint that was active when the named savepoint
> > + was created. Changes made after <command>RELEASE SAVEPOINT</command>
> > + will also be part of this active transaction or savepoint.
>
> I am not sure if "rolled back savepoints changes" is clear enough.
> I understand that you are trying to avoid the term "subtransaction".
> How about:
>
> All changes made since the creation of the savepoint that didn't already
> get rolled back are merged ...
Yes, I like that, changed.
> > --- a/doc/src/sgml/ref/rollback.sgml
> > +++ b/doc/src/sgml/ref/rollback.sgml
> >
> > + If <literal>AND CHAIN</literal> is specified, a new (unaborted)
>
> *Sigh* I'll make one last plea for "not aborted".
Uh, I thought you wanted "unaborted", but I now changed it to "not
aborted".
> > --- /dev/null
> > +++ b/doc/src/sgml/xact.sgml
>
> > + <para>
> > + Transactions can be created explicitly using <command>BEGIN</command>
> > + and <command>COMMIT</command>, which creates a transaction block.
> > + An SQL statement outside of a transaction block automatically uses
> > + a single-statement transaction.
> > + </para>
>
> Sorry, I should have noted that the first time around.
>
> Transactions are not created using COMMIT.
>
> Perhaps we should also avoid the term "transaction block". Even without speaking
> of a "block", way too many people confuse PL/pgSQL's BEGIN ... END blocks
> with transactions. On the other hand, we use "transaction block" everywhere
> else in the documentation...
>
> How about:
>
> <para>
> Multi-statement transactions can be created explicitly using
> <command>BEGIN</command> or <command>START TRANSACTION</command> and
> are ended using <command>COMMIT</command> or <command>ROLLBACK</command>.
> An SQL statement outside of a transaction block automatically uses
> a single-statement transaction.
> </para>
I used your wording, but technically you can use BEGIN/COMMIT with a
single statement, so multi-statement it not a requirement, so I used
your text but removed "Multi-statement":
Transactions can be created explicitly using <command>BEGIN</command> or
<command>START TRANSACTION</command> and ended using
<command>COMMIT</command> or <command>ROLLBACK</command>.
> > + <sect1 id="xact-locking">
> > +
> > + <title>Transactions and Locking</title>
> > +
> > + <para>
> > + The transaction IDs of 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>. Read-only transactions
> > + will have <structfield>virtualxid</structfield>s but NULL
> > + <structfield>transactionid</structfield>s, while read-write transactions
> > + will have both as non-NULL.
> > + </para>
>
> Perhaps the following will be prettier than "have both as non-NULL":
>
> ..., while both columns will be set in read-write transactions.
Agreed, changed. Updated patch attached.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
On Mon, Nov 21, 2022 at 11:35:09AM +0100, Álvaro Herrera wrote:
> Agreed on not using "unaborted", per previous discussion.
>
> On 2022-Nov-21, Laurenz Albe wrote:
>
> > Perhaps we should also avoid the term "transaction block". Even without speaking
> > of a "block", way too many people confuse PL/pgSQL's BEGIN ... END blocks
> > with transactions. On the other hand, we use "transaction block" everywhere
> > else in the documentation...
>
> Yeah, I don't understand why we need this "transaction block" term at
> all. It adds nothing. We could just use the term "transaction", and
> little meaning would be lost. When necessary, we could just say
> "explicit transaction" or something to that effect. In this particular
> case, we could modify your proposed wording,
Yes, I just posted that same thing:
Transactions can be created explicitly using
> > <para>
> > Multi-statement transactions can be created explicitly using
> > <command>BEGIN</command> or <command>START TRANSACTION</command> and
> > are ended using <command>COMMIT</command> or <command>ROLLBACK</command>.
> > An SQL statement outside of a transaction block automatically uses
> > a single-statement transaction.
> > </para>
>
> by removing the word "block":
>
> > Any SQL statement outside of an transaction automatically uses
> > a single-statement transaction.
>
> and perhaps add "explicit", but I don't think it's necessary:
>
> > Any SQL statement outside of an explicit transaction automatically
> > uses a single-statement transaction.
Full paragraph is now:
Transactions can be created explicitly using <command>BEGIN</command>
or <command>START TRANSACTION</command> and ended using
<command>COMMIT</command> or <command>ROLLBACK</command>. An SQL
statement outside of an explicit transaction automatically uses a
single-statement transaction.
> (I also changed "An" to "Any" because it seems more natural, but I
> suppose it's a stylistic choice.)
I think we have a plurality mismatch so I went with "SQL statements" and
didn't need "an" or "any" (even newer paragraph version):
Transactions can be created explicitly using <command>BEGIN</command>
or <command>START TRANSACTION</command> and ended using
<command>COMMIT</command> or <command>ROLLBACK</command>. SQL
statements outside of explicit transactions automatically use
single-statement transactions.
Updated patch attached.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
Op 22-11-2022 om 19:00 schreef Bruce Momjian: > On Mon, Nov 21, 2022 at 11:15:36AM +0100, Laurenz Albe wrote: >> ..., while both columns will be set in read-write transactions. > > Agreed, changed. Updated patch attached. In func.sgml: 'Only top-level transaction ID are' should be 'Only top-level transaction IDs are' 'subtransaction ID are' should be 'subtransaction IDs are' In xact.sgml: 'Non-virtual <literal>TransactionId</literal> (or <type>xid</type>)' should be 'Non-virtual <literal>TransactionId</literal>s (or <type>xid</type>s)' Erik Rijkers
On Tue, Nov 22, 2022 at 07:47:26PM +0100, Erik Rijkers wrote: > Op 22-11-2022 om 19:00 schreef Bruce Momjian: > > On Mon, Nov 21, 2022 at 11:15:36AM +0100, Laurenz Albe wrote: > > > ..., while both columns will be set in read-write transactions. > > > > Agreed, changed. Updated patch attached. > > In func.sgml: > > 'Only top-level transaction ID are' should be > 'Only top-level transaction IDs are' > > 'subtransaction ID are' should be > 'subtransaction IDs are' > > In xact.sgml: > > 'Non-virtual <literal>TransactionId</literal> (or <type>xid</type>)' should > be > 'Non-virtual <literal>TransactionId</literal>s (or <type>xid</type>s)' Agreed, updated patch attached. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
On Tue, 2022-11-22 at 13:50 -0500, Bruce Momjian wrote: > Agreed, updated patch attached. I cannot find any more problems, and I shouldn't mention the extra empty line at the end of the patch. I'd change the commitfest status to "ready for committer" now if it were not already in that status. Yours, Laurenz Albe
On Tue, Nov 22, 2022 at 01:50:36PM -0500, Bruce Momjian wrote: > + > + <para> > + A more complex example with multiple nested subtransactions: > +<programlisting> > +BEGIN; > + INSERT INTO table1 VALUES (1); > + SAVEPOINT sp1; > + INSERT INTO table1 VALUES (2); > + SAVEPOINT sp2; > + INSERT INTO table1 VALUES (3); > + RELEASE SAVEPOINT sp2; > + INSERT INTO table1 VALUES (4))); -- generates an error > +</programlisting> > + In this example, the application requests the release of the savepoint > + <literal>sp2</literal>, which inserted 3. This changes the insert's > + transaction context to <literal>sp1</literal>. When the statement > + attempting to insert value 4 generates an error, the insertion of 2 and > + 4 are lost because they are in the same, now-rolled back savepoint, > + and value 3 is in the same transaction context. The application can > + now only choose one of these two commands, since all other commands > + will be ignored with a warning: > +<programlisting> > + ROLLBACK; > + ROLLBACK TO SAVEPOINT sp1; > +</programlisting> > + Choosing <command>ROLLBACK</command> will abort everything, including > + value 1, whereas <command>ROLLBACK TO SAVEPOINT sp1</command> will retain > + value 1 and allow the transaction to continue. > + </para> This mentions a warning, but what happens is actually an error: postgres=!# select; ERROR: current transaction is aborted, commands ignored until end of transaction block
On Wed, Nov 23, 2022 at 08:57:33AM +0100, Laurenz Albe wrote: > On Tue, 2022-11-22 at 13:50 -0500, Bruce Momjian wrote: > > Agreed, updated patch attached. > > I cannot find any more problems, and I shouldn't mention the extra empty > line at the end of the patch. Fixed. ;-) > I'd change the commitfest status to "ready for committer" now if it were > not already in that status. I knew we would eventually get here. The feedback has been very helpful and I am excited about the content. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
On Wed, Nov 23, 2022 at 02:17:19AM -0600, Justin Pryzby wrote:
> On Tue, Nov 22, 2022 at 01:50:36PM -0500, Bruce Momjian wrote:
> > +
> > + <para>
> > + A more complex example with multiple nested subtransactions:
> > +<programlisting>
> > +BEGIN;
> > + INSERT INTO table1 VALUES (1);
> > + SAVEPOINT sp1;
> > + INSERT INTO table1 VALUES (2);
> > + SAVEPOINT sp2;
> > + INSERT INTO table1 VALUES (3);
> > + RELEASE SAVEPOINT sp2;
> > + INSERT INTO table1 VALUES (4))); -- generates an error
> > +</programlisting>
> > + In this example, the application requests the release of the savepoint
> > + <literal>sp2</literal>, which inserted 3. This changes the insert's
> > + transaction context to <literal>sp1</literal>. When the statement
> > + attempting to insert value 4 generates an error, the insertion of 2 and
> > + 4 are lost because they are in the same, now-rolled back savepoint,
> > + and value 3 is in the same transaction context. The application can
> > + now only choose one of these two commands, since all other commands
> > + will be ignored with a warning:
> > +<programlisting>
> > + ROLLBACK;
> > + ROLLBACK TO SAVEPOINT sp1;
> > +</programlisting>
> > + Choosing <command>ROLLBACK</command> will abort everything, including
> > + value 1, whereas <command>ROLLBACK TO SAVEPOINT sp1</command> will retain
> > + value 1 and allow the transaction to continue.
> > + </para>
>
> This mentions a warning, but what happens is actually an error:
>
> postgres=!# select;
> ERROR: current transaction is aborted, commands ignored until end of transaction block
Good point, new text:
The application can now only choose one of these two commands,
since all other commands will be ignored:
Updated patch attached.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
Attachment
On Fri, Nov 18, 2022 at 02:11:50PM -0500, Bruce Momjian wrote: > On Mon, Nov 7, 2022 at 11:04:46PM +0100, Laurenz Albe wrote: > > 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. > > Sorry again for the long delay in replying to this. Patch applied back to PG 11. Thanks to Simon for getting this important information in our docs, and for the valuable feedback from others that made this even better. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.
On 30.11.22 02:51, Bruce Momjian wrote: > On Fri, Nov 18, 2022 at 02:11:50PM -0500, Bruce Momjian wrote: >> On Mon, Nov 7, 2022 at 11:04:46PM +0100, Laurenz Albe wrote: >>> 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. >> >> Sorry again for the long delay in replying to this. > > Patch applied back to PG 11. Thanks to Simon for getting this important > information in our docs, and for the valuable feedback from others that > made this even better. I request that the backpatching of this be reverted. We don't want to have major chapter numbers in the documentation changing between minor releases. More generally, major documentation additions shouldn't be backpatched, for the same reasons we don't backpatch features.
On Wed, 30 Nov 2022 at 01:51, Bruce Momjian <bruce@momjian.us> wrote: > Thanks to Simon for getting this important > information in our docs, and for the valuable feedback from others that > made this even better. And thanks to you for pulling that all together Bruce. -- Simon Riggs http://www.EnterpriseDB.com/
On Wed, Nov 30, 2022 at 07:33:44AM +0100, Peter Eisentraut wrote: > On 30.11.22 02:51, Bruce Momjian wrote: > > Patch applied back to PG 11. Thanks to Simon for getting this important > > information in our docs, and for the valuable feedback from others that > > made this even better. > > I request that the backpatching of this be reverted. We don't want to have > major chapter numbers in the documentation changing between minor releases. Uh, how do others feel about this? I wanted to get this information to all our supported releases as soon as possible, rather than waiting for PG 16 and for people to upgrade. To me it seems the chapter renumbering is worth that benefit. I could put the new chapter inside an existing numbered section in the back branches if people prefer that, but then PG 16 would have it in a different place, which seems bad. > More generally, major documentation additions shouldn't be backpatched, for > the same reasons we don't backpatch features. I don't see how documentation additions and feature additions are comparable. Feature additions change the behavior of the system, and potentially introduce application breakage and bugs, while documentation additions, if they apply to the supported version, have no such risks. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.
On Wed, Nov 30, 2022 at 6:52 AM Bruce Momjian <bruce@momjian.us> wrote:
On Wed, Nov 30, 2022 at 07:33:44AM +0100, Peter Eisentraut wrote:
> On 30.11.22 02:51, Bruce Momjian wrote:
> > Patch applied back to PG 11. Thanks to Simon for getting this important
> > information in our docs, and for the valuable feedback from others that
> > made this even better.
>
> I request that the backpatching of this be reverted. We don't want to have
> major chapter numbers in the documentation changing between minor releases.
Uh, how do others feel about this? I wanted to get this information to
all our supported releases as soon as possible, rather than waiting for
PG 16 and for people to upgrade. To me it seems the chapter renumbering
is worth that benefit.
I'd maybe accept having it back-patched to v15 on that basis but not any further.
But I agree that our general behavior is to only apply this scope of update of the documentation to HEAD.
David J.
On Wed, Nov 30, 2022 at 07:10:35AM -0700, David G. Johnston wrote: > On Wed, Nov 30, 2022 at 6:52 AM Bruce Momjian <bruce@momjian.us> wrote: > I'd maybe accept having it back-patched to v15 on that basis but not any > further. > > But I agree that our general behavior is to only apply this scope of update of > the documentation to HEAD. If everyone agrees this new chapter is helpful, and as helpful to PG 11 users as PG 16 users, why would we not give users this information in our docs now? What is the downside? Chapter numbers? Translations? I assume this new chapter would be mentioned in the minor release notes. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.
On Wed, Nov 30, 2022 at 8:02 AM Bruce Momjian <bruce@momjian.us> wrote:
On Wed, Nov 30, 2022 at 07:10:35AM -0700, David G. Johnston wrote:
> On Wed, Nov 30, 2022 at 6:52 AM Bruce Momjian <bruce@momjian.us> wrote:
> I'd maybe accept having it back-patched to v15 on that basis but not any
> further.
>
> But I agree that our general behavior is to only apply this scope of update of
> the documentation to HEAD.
If everyone agrees this new chapter is helpful, and as helpful to PG 11
users as PG 16 users, why would we not give users this information in
our docs now? What is the downside? Chapter numbers? Translations?
Admittedly the policy is more about "we don't expend any effort to write back branch patches for this kind of material" rather than "we don't do back patching because it causes problems". But it is an existing policy, applied consistently through the years, and treating the documentation like a book, even though it is published in a non-physical medium, is a reasonable guideline to follow.
My desire to get it out in an official release early goes against this policy, and I'm fine waiting for v16 on that basis. The only reason I'm good with updating v15 is that I basically consider anything in the first 3 point releases of a major version to be a "delta" release.
One back-patchable idea to consider would be adding a note at the top of the page(s) highlighting the fact that said material has been superseded by more current documentation, with a link. But the idea of changing long-released (see my delta comment above) material doesn't sit well with me or the policy.
I assume this new chapter would be mentioned in the minor release notes.
We don't do release notes for documentation changes.
David J.
On Wed, Nov 30, 2022 at 08:25:19AM -0700, David G. Johnston wrote: > On Wed, Nov 30, 2022 at 8:02 AM Bruce Momjian <bruce@momjian.us> wrote: > On Wed, Nov 30, 2022 at 07:10:35AM -0700, David G. Johnston wrote: > If everyone agrees this new chapter is helpful, and as helpful to PG 11 > users as PG 16 users, why would we not give users this information in > our docs now? What is the downside? Chapter numbers? Translations? > > Admittedly the policy is more about "we don't expend any effort to write back > branch patches for this kind of material" rather than "we don't do back > patching because it causes problems". But it is an existing policy, applied > consistently through the years, and treating the documentation like a book, > even though it is published in a non-physical medium, is a reasonable guideline > to follow. Well, we have not backpatched cosmetic or wording improvements into back branches, usually, though unclear wording has been backpatched because the value is more significant than the disruption. I think we look at doc backpatching on an individual basis, because there are limited risks, unlike code changes. > My desire to get it out in an official release early goes against this policy, > and I'm fine waiting for v16 on that basis. The only reason I'm good with > updating v15 is that I basically consider anything in the first 3 point > releases of a major version to be a "delta" release. Well, yeah, I think the PG 15-16 is kind of an odd approach, though I can see the value of doing that since we could say anyone who cares about these details should be on the most recent major release. I think you are reinforcing my basic approach that doc changes can't have a simple blanket policy, unlike code, because of the limited risks and significan value. > One back-patchable idea to consider would be adding a note at the top of the > page(s) highlighting the fact that said material has been superseded by more > current documentation, with a link. But the idea of changing long-released > (see my delta comment above) material doesn't sit well with me or the policy. Uh, I don't share that concern, as long as it is mentioned in the minor release notes. > I assume this new chapter would be mentioned in the minor release notes. > > We don't do release notes for documentation changes. Uh, I certainly have done it for significant doc improvements in major releases, so I don't see a problem in doing it for minor releases, especially since this information has been needed in our docs for years. What I am basically saying is that "we have always done it that way" is an insufficient reason for me --- we should have some logic for _why_ we have a policy, and I am not seeing that here. This is obviously a bigger issue than this particular patch. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.
On 2022-Nov-30, Bruce Momjian wrote: > On Wed, Nov 30, 2022 at 07:33:44AM +0100, Peter Eisentraut wrote: > > On 30.11.22 02:51, Bruce Momjian wrote: > > > Patch applied back to PG 11. Thanks to Simon for getting this important > > > information in our docs, and for the valuable feedback from others that > > > made this even better. > > > > I request that the backpatching of this be reverted. We don't want to have > > major chapter numbers in the documentation changing between minor releases. > > Uh, how do others feel about this? I wanted to get this information to > all our supported releases as soon as possible, rather than waiting for > PG 16 and for people to upgrade. I find it a bit shocking to have had it backpatched, even to 15 -- a whole chapter in the documentation? I don't see why it wouldn't be treated like any other "major feature" patch, which we only consider for the development branch. Also, this is a first cut -- presumably we'll want to copy-edit it before it becomes released material. Now, keep in mind that not having it backpatched does not mean that it is invisible to users. It is definitely visible, if they use the doc URL with /devel/ in it. And this information has been missing for 20+ years, how come it is so urgent to have it everywhere now? I agree that it should be reverted from all branches other than master. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I find it a bit shocking to have had it backpatched, even to 15 -- a
> whole chapter in the documentation? I don't see why it wouldn't be
> treated like any other "major feature" patch, which we only consider for
> the development branch. Also, this is a first cut -- presumably we'll
> want to copy-edit it before it becomes released material.
I think that last point is fairly convincing. I've not read the
new material, but I didn't get further than the first line of
the new chapter file before noting a copy-and-paste error:
--- /dev/null
+++ b/doc/src/sgml/xact.sgml
@@ -0,0 +1,205 @@
+<!-- doc/src/sgml/mvcc.sgml -->
That doesn't leave me with a warm feeling that it's ready to ship.
I too vote for reverting it out of the released branches.
regards, tom lane
On Wed, Nov 30, 2022 at 12:31:55PM -0500, Tom Lane wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > I find it a bit shocking to have had it backpatched, even to 15 -- a > > whole chapter in the documentation? I don't see why it wouldn't be > > treated like any other "major feature" patch, which we only consider for > > the development branch. Also, this is a first cut -- presumably we'll > > want to copy-edit it before it becomes released material. > > I think that last point is fairly convincing. I've not read the > new material, but I didn't get further than the first line of > the new chapter file before noting a copy-and-paste error: > > --- /dev/null > +++ b/doc/src/sgml/xact.sgml > @@ -0,0 +1,205 @@ > +<!-- doc/src/sgml/mvcc.sgml --> Fixed in master. > That doesn't leave me with a warm feeling that it's ready to ship. > I too vote for reverting it out of the released branches. Patch reverted in all back branches. I was hoping to get support for more aggressive backpatches of docs, but obviously failed. I should have been clearer about my intent to backpatch, and will have to consider these issues in future doc backpatches. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.
On 2022-Dec-01, Bruce Momjian wrote: > Patch reverted in all back branches. I was hoping to get support for > more aggressive backpatches of docs, but obviously failed. I should > have been clearer about my intent to backpatch, and will have to > consider these issues in future doc backpatches. FWIW I am in favor of backpatching doc fixes (even if they're not completely trivial, such as 02d43ad6262d) and smallish additions of content (63a370938). But we've added a few terms to the glossary (606c38459, 3dddb2a82) and those weren't backpatched, which seems appropriate to me. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "The problem with the facetime model is not just that it's demoralizing, but that the people pretending to work interrupt the ones actually working." (Paul Graham)