Re: [PATCH] Docs: Make notes on sequences and rollback more obvious

Hi all

I'm seeing enough questions on pgsql-general and stack overflow to
suggest that the docs for how sequences interact with transaction
rollback. Take the most recent post on -general, where the person read
at least the tutorial, but had no idea about the exemption.

The attached patch:

- Moves the note about nextval() from the footer to be inside the
nextval description

- Adds an xref from the advanced-transactions tutorial where the poster
noted their point of confusion, noting the exemption and pointing to the
docs on nextval.

- A pointer from the docs on SERIAL types to the nextval notes on tx
rollback.

Comments would be appreciated.

--
Craig Ringer

Craig Ringer <ringerc(at)ringerc(dot)id(dot)au> wrote:

> I'm seeing enough questions on pgsql-general and stack overflow
> to suggest that the docs for how sequences interact with
> transaction rollback.

Yeah, I've noticed a surprising number of people who are being
surprised by the non-transactional nature of sequences (and serial
columns) in spite of current caveats in the docs; so I agree that we
should punch that up in the docs a bit.

> The attached patch:
>
> - Moves the note about nextval() from the footer to be inside the
> nextval description
>
> - Adds an xref from the advanced-transactions tutorial where the
> poster noted their point of confusion, noting the exemption and
> pointing to the docs on nextval.
>
> - A pointer from the docs on SERIAL types to the nextval notes on
> tx rollback.
>
> Comments would be appreciated.

I haven't reviewed it in detail but noticed an apparent editing
error: "which are used the counters" should probably have an "as"
thrown in there. Or something.

-Kevin

On 08/04/2012 04:12 AM, Kevin Grittner wrote:

> I haven't reviewed it in detail but noticed an apparent editing error:
> "which are used the counters" should probably have an "as" thrown in
> there. Or something.

Thanks. Editing fail. I revised that spot repeatedly to try to keep it
short and simple without in any way implying that SEQUENCEs are *only*
used for SERIAL columns.

Fixed attached.

On Sat, Aug 4, 2012 at 12:56 AM, Craig Ringer <ringerc(at)ringerc(dot)id(dot)au> wrote:
> On 08/04/2012 04:12 AM, Kevin Grittner wrote:
>> I haven't reviewed it in detail but noticed an apparent editing error:
>> "which are used the counters" should probably have an "as" thrown in there.
>> Or something.
>
> Thanks. Editing fail. I revised that spot repeatedly to try to keep it short
> and simple without in any way implying that SEQUENCEs are *only* used for
> SERIAL columns.
>
> Fixed attached.

In datatype.sgml, I think that adding that <important> block in the
middle of the existing paragraph is too choppy. I moved it down a
bit, changed it to a note, expanded it a little, and fixed some typos
and markup.

In func.sgml, I chose to keep the <important> at the end, instead of
switching the order of the paragraphs as you did, but I moved it up
under nextval instead of having it at the end, as you had it. I kept
your note in setval() but cleaned it up a bit.

I did not commit the advanced.sgml changes. I am not sure I believe
the assertion that any function or type with special transactional
behavior will include a documentation mention. It doesn't seem like a
terribly future-proof assertion at any rate. With respect to the
mention of autocommit, I think it would be good to add something
there, but maybe it should cross-reference our existing documentation
mentions of autocommit. Also, it's a bit ambiguous the way it's
worded whether you get the automatic BEGIN/COMMIT with autocommit=on
or with autocommit=off; somehow we should try to clarify what we mean
a little more there.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 08/07/2012 02:27 AM, Robert Haas wrote:
> I did not commit the advanced.sgml changes.

That's arguably the most important point to raise this. The most recent
question came from someone who actually bothered to RTFM and believed
based on the advanced-transactions page that rollback rolls *everything*
back.

Some kind of hint that there are execptions is IMO very important. I'm
not sure what the best form for it to take is.

> I am not sure I believe
> the assertion that any function or type with special transactional
> behavior will include a documentation mention.

It absolutely should, but I guess that doesn't mean it's guaranteed to.

> It doesn't seem like a
> terribly future-proof assertion at any rate. With respect to the
> mention of autocommit, I think it would be good to add something
> there, but maybe it should cross-reference our existing documentation
> mentions of autocommit. Also, it's a bit ambiguous the way it's
> worded whether you get the automatic BEGIN/COMMIT with autocommit=on
> or with autocommit=off; somehow we should try to clarify what we mean
> a little more there.

Yeah. I should've kept that separate, as it was something I noticed in
passing, rather than central to the changes.

--
Craig Ringer

On Tue, Aug 7, 2012 at 3:59 AM, Craig Ringer <ringerc(at)ringerc(dot)id(dot)au> wrote:
> On 08/07/2012 02:27 AM, Robert Haas wrote:
>>
>> I did not commit the advanced.sgml changes.
>
> That's arguably the most important point to raise this. The most recent
> question came from someone who actually bothered to RTFM and believed based
> on the advanced-transactions page that rollback rolls *everything* back.
>
> Some kind of hint that there are execptions is IMO very important. I'm not
> sure what the best form for it to take is.

I'm not sure, either. Maybe we should avoid blanket statements and
just say something like:

Note: Some operations on sequences are non-transactional and will not
be rolled back on transaction abort. See <xref>.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Craig Ringer <ringerc(at)ringerc(dot)id(dot)au> wrote:
>> On 08/07/2012 02:27 AM, Robert Haas wrote:
>>>
>>> I did not commit the advanced.sgml changes.
>>
>> That's arguably the most important point to raise this. The most
>> recent question came from someone who actually bothered to RTFM
>> and believed based on the advanced-transactions page that
>> rollback rolls *everything* back.
>>
>> Some kind of hint that there are execptions is IMO very
>> important. I'm not sure what the best form for it to take is.
>
> I'm not sure, either. Maybe we should avoid blanket statements
> and just say something like:
>
> Note: Some operations on sequences are non-transactional and will
> not be rolled back on transaction abort. See <xref>.

I also think it's a problem that one can get through the entire
"Concurrency Control" chapter (mvcc.sgml) without a clue that
sequences aren't transactional. I think maybe a mention in the
Introduction section of that chapter with a <ref> would be
appropriate.

-Kevin

On Tuesday, August 07, 2012 09:45:35 AM Kevin Grittner wrote:
[...snipped...]
> I also think it's a problem that one can get through the entire
> "Concurrency Control" chapter (mvcc.sgml) without a clue that
> sequences aren't transactional. I think maybe a mention in the
> Introduction section of that chapter with a <ref> would be
> appropriate.
>

+1

> -Kevin

On Tue, Aug 07, 2012 at 03:59:42PM +0800, Craig Ringer wrote:
> On 08/07/2012 02:27 AM, Robert Haas wrote:
> >I did not commit the advanced.sgml changes.
>
> That's arguably the most important point to raise this. The most
> recent question came from someone who actually bothered to RTFM and
> believed based on the advanced-transactions page that rollback rolls
> *everything* back.

Perhaps we should see about correcting that misapprehension. When
PostgreSQL does any irreversible process
<http://en.wikipedia.org/wiki/Irreversible_process> such as
incrementing a sequence, writing a file, sending an email, etc., it
can't be rolled back. Might it be useful to find those irreversible
operations we document and mark same?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 08/07/2012 09:45 PM, Kevin Grittner wrote:
> I also think it's a problem that one can get through the entire
> "Concurrency Control" chapter (mvcc.sgml) without a clue that
> sequences aren't transactional. I think maybe a mention in the
> Introduction section of that chapter with a <ref> would be
> appropriate.

How about this? Is it accurate to suggest that sequences behave as if
they were always in "dirty read" isolation? Or would you instead say
that "changes made to a sequence are immediately visible to all other
transactions" ?

[PATCH] Make sure you can't read through mvcc.sgml without realising
that SEQUENCEs and SERIAL don't follow the rules.

---
doc/src/sgml/mvcc.sgml | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
new file mode 100644
index 8f88582..eed1f85
*** a/doc/src/sgml/mvcc.sgml
--- b/doc/src/sgml/mvcc.sgml
***************
*** 260,265 ****
--- 260,276 ----
command <xref linkend="sql-set-transaction">.
</para>

+ <important>
+ <para>
+ Some PostgreSQL data types and functions have special transactional
+ behaviour. Notably, SEQUENCEs
+ behave as if the isolation level is Dirty Read, irrespective
+ of the current isolation level, and are exempt from transaction
+ rollback. See <xref linkend="functions-sequence">.
+ SEQUENCEs are used by the SERIAL data types. See <xref
linkend="datatype-serial">.
+ </para>
+ </important>
+
<sect2 id="xact-read-committed">
<title>Read Committed Isolation Level</title>

--
1.7.11.2

--
Craig Ringer

On 08/07/2012 09:45 PM, Kevin Grittner wrote:

> I also think it's a problem that one can get through the entire
> "Concurrency Control" chapter (mvcc.sgml) without a clue that
> sequences aren't transactional.

I'm also wondering about adding something like the following summary of
features with odd transactional behaviour. I'm sure there's more than
I've listed, but nothing is jumping out at me.

<sect1 id="mvcc-exceptions">
<title>Exceptions to normal transactional rules</title>

<para>
Some PostgreSQL features, functions and data types differ from the
usual transactional behaviour described in this chapter. Differences
are generally mentioned in the documentation sections for the
features they affect. Such exceptions are collected here for
easy reference.
</para>

<itemizedlist>
<listitem>
<para>
Serial pseudo-types <xref linkend="datatype-serial">
</para>
</listitem>
<listitem>
<para>
<literal>SEQUENCE</literal>s - <xref linkend="functions-sequence">
</para>
</listitem>
<listitem>
<para>
Advisory locks - <xref linkend="advisory-locks">
</para>
</listitem>
<listitem>
<para>
Disk I/O to files outside the database, as performed by
<literal>COPY ... TO</literal>, adminpack functions, and other
add-ons. See <xref linkend="sql-copy">,
<xref linkend="adminpack">.
</para>
</listitem>
</itemizedlist>

</sect1>

2012/8/7 Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov>:

> I also think it's a problem that one can get through the entire
> "Concurrency Control" chapter (mvcc.sgml) without a clue that
> sequences aren't transactional.

It is possible to say that they *are* transactional when considering
the following definition: nextval() doesn’t always give you “the” next
value, but “some” next value that is higher than the one gotten by any
preceding transactions.

I personally like it better to introduce this minor complexity in the
definition of sequences, rather than messing with the definition of
transactionality.

Nicolas

--
A. Because it breaks the logical sequence of discussion.
Q. Why is top posting bad?

On 08/18/2012 05:19 PM, Nicolas Barbier wrote:
> 2012/8/7 Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov>:
>
>> I also think it's a problem that one can get through the entire
>> "Concurrency Control" chapter (mvcc.sgml) without a clue that
>> sequences aren't transactional.
>
> It is possible to say that they *are* transactional when considering
> the following definition: nextval() doesn’t always give you “the” next
> value, but “some” next value that is higher than the one gotten by any
> preceding transactions.
>
> I personally like it better to introduce this minor complexity in the
> definition of sequences, rather than messing with the definition of
> transactionality.

I guess they're semi-transactional. You don't get dirty reads unless you
actually `SELECT ... FROM some_transaction` which isn't really official
API. OTOH, one transaction affects another, and they aren't subject to
rollbacks.

--
Craig Ringer

On Sat, Aug 18, 2012 at 1:34 AM, Craig Ringer <ringerc(at)ringerc(dot)id(dot)au> wrote:
> On 08/07/2012 09:45 PM, Kevin Grittner wrote:
>>
>> I also think it's a problem that one can get through the entire
>> "Concurrency Control" chapter (mvcc.sgml) without a clue that
>> sequences aren't transactional. I think maybe a mention in the
>> Introduction section of that chapter with a <ref> would be
>> appropriate.
>
>
> How about this? Is it accurate to suggest that sequences behave as if they
> were always in "dirty read" isolation?

I don't think so. I would think that a dirty read would allow
unresolved data to be visible, but upon rollback of the other
transaction would stop seeing the "dirty" data. That doesn't describe
sequences. A better explanation is that sequence advancement is
autonomously committed.

> Or would you instead say that
> "changes made to a sequence are immediately visible to all other
> transactions" ?

Yes, that sounds better.

Cheers,

Jeff

On 08/19/2012 03:01 AM, Jeff Janes wrote:
>> >Or would you instead say that
>> >"changes made to a sequence are immediately visible to all other
>> >transactions" ?
> Yes, that sounds better.

OK, how about the attached series, then?

The 2nd probably needs improvement - and I expect I've missed some other
areas that aren't strictly transactional.

Comments?

Working branch:
https://github.com/ringerc/postgres/tree/sequence_documentation_fixes

--
Craig Ringer

Trying again with the attachments; the archiver only seemed to see the
first patch despite all three being attached. Including patches inline;
if you want 'em prettier, see:

https://github.com/ringerc/postgres/tree/sequence_documentation_fixes

Subject: [PATCH 1/3] Make sure you can't read through mvcc.sgml without
realising that not everything is MVCC.

---
doc/src/sgml/mvcc.sgml | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
new file mode 100644
index 8f88582..9dc65f5
*** a/doc/src/sgml/mvcc.sgml
--- b/doc/src/sgml/mvcc.sgml
***************
*** 260,265 ****
--- 260,277 ----
command <xref linkend="sql-set-transaction">.
</para>

+ <important>
+ <para>
+ Some <productname>PostgreSQL</productname> data types and
functions have
+ special rules regarding transactional behaviour. In
particular, changes
+ made to a <literal>SEQUENCE</literal> (and therefore the
counter of a
+ <literal>SERIAL</literal>) are immediately visible to all other
+ transactions and are not rolled back if the transaction that
made the
+ changes aborts. See <xref linkend="functions-sequence"> and
+ <xref linkend="datatype-serial">.
+ </para>
+ </important>
+
<sect2 id="xact-read-committed">
<title>Read Committed Isolation Level</title>

-- 1.7.11.2

Subject: [PATCH 2/3] Collect a list of features with abberant
transactional behaviour

---
doc/src/sgml/mvcc.sgml | 67
++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
new file mode 100644
index 9dc65f5..e2930c9
*** a/doc/src/sgml/mvcc.sgml
--- b/doc/src/sgml/mvcc.sgml
*************** SELECT pg_advisory_lock(q.id) FROM
*** 1540,1543 ****
--- 1540,1610 ----
indexes should be used instead.
</para>
</sect1>
+
+ <sect1 id="mvcc-exceptions">
+ <title>Exceptions to normal transactional rules</title>
+
+ <para>
+ Some PostgreSQL features, functions and data types differ from the
+ usual transactional behaviour described in this chapter. Differences
+ are generally mentioned in the documentation sections for the
+ features they affect. Such exceptions are collected here for
+ easy reference.
+ </para>
+
+ <para>
+ The following actions and features don't follow the typical
+ transactional rules:
+ </para>
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ Serial pseudo-types <xref linkend="datatype-serial">
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ <literal>SEQUENCE</literal>s - <xref linkend="functions-sequence">
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ Advisory locks - <xref linkend="advisory-locks">
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ Disk writes to files outside the database, as performed by
+ <literal>COPY ... TO</literal>, adminpack functions, and other
add-ons.
+ See <xref linkend="sql-copy">, <xref linkend="adminpack">.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ Any network I/O or inter-process communication not explicitly
+ described as transactional in its documentation. For example,
+ sending an email from PL/PerlU would not be transactional;
+ the email would be sent before the transaction commits and
+ could not be un-sent if the transaction were to roll back.
+ </listitem>
+ </itemizedlist>
+
+ <note>
+ <para>
+ When working with external non-transactional resources like files
+ on disk or network sockets the two-phase commit feature can be
+ useful. See: <xref linkend="sql-prepare-transaction">
+ </para>
+ <para>
+ LISTEN/NOTIFY provides a lighter weight but still
transaction-friendly method of
+ triggering changes outside the database in response to changes
inside the
+ database. A LISTENing helper program running outside the database can
+ perform actions when it gets a NOTIFY after a transaction
commits. See:
+ <xref linkend="sql-notify">.
+ </para>
+ </note>
+
+ </sect1>
+
</chapter>
-- 1.7.11.2

Subject: [PATCH 3/3] Change xref of <important/> note re SERIAL to point
to mvcc-exceptions

---
doc/src/sgml/mvcc.sgml | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
new file mode 100644
index e2930c9..0de4b75
*** a/doc/src/sgml/mvcc.sgml
--- b/doc/src/sgml/mvcc.sgml
***************
*** 267,274 ****
made to a <literal>SEQUENCE</literal> (and therefore the
counter of a
<literal>SERIAL</literal>) are immediately visible to all other
transactions and are not rolled back if the transaction that
made the
! changes aborts. See <xref linkend="functions-sequence"> and
! <xref linkend="datatype-serial">.
</para>
</important>

--- 267,273 ----
made to a <literal>SEQUENCE</literal> (and therefore the
counter of a
<literal>SERIAL</literal>) are immediately visible to all other
transactions and are not rolled back if the transaction that
made the
! changes aborts. See <xref linkend="mvcc-exceptions">.
</para>
</important>

-- 1.7.11.2

On Mon, Aug 20, 2012 at 4:45 AM, Craig Ringer <ringerc(at)ringerc(dot)id(dot)au> wrote:
> Trying again with the attachments; the archiver only seemed to see the first
> patch despite all three being attached. Including patches inline; if you
> want 'em prettier, see:
>
> https://github.com/ringerc/postgres/tree/sequence_documentation_fixes
>
>
> Subject: [PATCH 1/3] Make sure you can't read through mvcc.sgml without
> realising that not everything is MVCC.
>

The first of these three patches looks good to me, so I committed it.
I am not convinced that the others are ready to go in. AFAICS, there
hasn't been any discussion of whether a list of non-transactional
features would be a useful thing to have, or if so where it should be
located in the docs and what should go into it. I'm not necessarily
opposed to adding something, but I think it needs some actual
discussion before we commit anything.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 08/21/2012 11:18 PM, Robert Haas wrote:
> On Mon, Aug 20, 2012 at 4:45 AM, Craig Ringer <ringerc(at)ringerc(dot)id(dot)au> wrote:
>> Trying again with the attachments; the archiver only seemed to see the first
>> patch despite all three being attached. Including patches inline; if you
>> want 'em prettier, see:
>>
>> https://github.com/ringerc/postgres/tree/sequence_documentation_fixes
>>
>>
>> Subject: [PATCH 1/3] Make sure you can't read through mvcc.sgml without
>> realising that not everything is MVCC.
>>
>
> The first of these three patches looks good to me, so I committed it.
> I am not convinced that the others are ready to go in. AFAICS, there
> hasn't been any discussion of whether a list of non-transactional
> features would be a useful thing to have, or if so where it should be
> located in the docs and what should go into it. I'm not necessarily
> opposed to adding something, but I think it needs some actual
> discussion before we commit anything.

Fine by me; I just thought a concrete proposed change might get people
talking about it better than my doing some more broad hand-waving on the
topic.

Anyone?

Should we add a section that lists exceptions to normal transactional
behaviour in one place, so instead of having to say "SEQUENCEs and some
other features" or "various types, functions and features" there's
something *concrete* to point to when discussing transactional oddities?

+
+ <sect1 id="mvcc-exceptions">
+ <title>Exceptions to normal transactional rules</title>
+
+ <para>
+ Some PostgreSQL features, functions and data types differ from the
+ usual transactional behaviour described in this chapter. Differences
+ are generally mentioned in the documentation sections for the
+ features they affect. Such exceptions are collected here for
+ easy reference.
+ </para>
+
+ <para>
+ The following actions and features don't follow the typical
+ transactional rules:
+ </para>
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ Serial pseudo-types <xref linkend="datatype-serial">
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ <literal>SEQUENCE</literal>s - <xref linkend="functions-sequence">
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ Advisory locks - <xref linkend="advisory-locks">
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ Disk writes to files outside the database, as performed by
+ <literal>COPY ... TO</literal>, adminpack functions, and other
add-ons.
+ See <xref linkend="sql-copy">, <xref linkend="adminpack">.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ Any network I/O or inter-process communication not explicitly
+ described as transactional in its documentation. For example,
+ sending an email from PL/PerlU would not be transactional;
+ the email would be sent before the transaction commits and
+ could not be un-sent if the transaction were to roll back.
+ </listitem>
+ </itemizedlist>
+
+ <note>
+ <para>
+ When working with external non-transactional resources like files
+ on disk or network sockets the two-phase commit feature can be
+ useful. See: <xref linkend="sql-prepare-transaction">
+ </para>
+ <para>
+ LISTEN/NOTIFY provides a lighter weight but still
transaction-friendly method of
+ triggering changes outside the database in response to changes
inside the
+ database. A LISTENing helper program running outside the database can
+ perform actions when it gets a NOTIFY after a transaction
commits. See:
+ <xref linkend="sql-notify">.
+ </para>
+ </note>
+
+ </sect1>
+
</chapter>