Update Python psycopg2 example with app retry loop

[rmloveland]

... and update the surrounding descriptive text to match the new code.

Addresses #5035.

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/56cf2cea2c8131d8a41fd7d28387ca8f87dc2631/

Edited pages:

• v19.1/build-a-python-app-with-cockroachdb.md

[bdarnell]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @rmloveland)

---

_includes/v19.1/app/insecure/basic-sample.py, line 41 at r1 (raw file):

    # Check the current balance.
    with conn.cursor() as cur:
        cur.execute("SELECT balance FROM accounts WHERE id = " + str(frm))

This read must occur within the transaction.

---

_includes/v19.1/app/insecure/basic-sample.py, line 62 at r1 (raw file):

                raise ValueError("Transaction did not succeed after 3 retries")

            cur.execute('BEGIN')

BEGIN is special and should not be executed as a normal statement. In python transactions are created automatically by default, so you don't need to run BEGIN explicitly.

---

_includes/v19.1/app/insecure/basic-sample.py, line 84 at r1 (raw file):

                logging.debug("e.pgcode: {}".format(e.pgcode))
                if e.pgcode == '40001':
                    cur.execute('ROLLBACK')

Use cur.rollback() instead of cur.execute('ROLLBACK').

---

_includes/v19.1/app/insecure/basic-sample.py, line 105 at r1 (raw file):

    # Make each statement commit immediately.
    conn.set_session(autocommit=True)

You're explicitly committing everything, so you want autocommit to be false (which is the default).

[rmloveland]

Thanks for the review, Ben. I think my latest changes address your feedback. PTAL!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)

---

_includes/v19.1/app/insecure/basic-sample.py, line 41 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This read must occur within the transaction.

Fixed (I hope) by moving it inside the try block in the retry loop (which is what I assume you mean by "within the transaction"?)

---

_includes/v19.1/app/insecure/basic-sample.py, line 62 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

BEGIN is special and should not be executed as a normal statement. In python transactions are created automatically by default, so you don't need to run BEGIN explicitly.

Fixed by removing.

---

_includes/v19.1/app/insecure/basic-sample.py, line 84 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Use cur.rollback() instead of cur.execute('ROLLBACK').

Fixed.

---

_includes/v19.1/app/insecure/basic-sample.py, line 105 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

You're explicitly committing everything, so you want autocommit to be false (which is the default).

Fixed by deleting.

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/ad871d6a03939383d4e23ed00467354e656e682a/

Edited pages:

• v19.1/build-a-python-app-with-cockroachdb.md

[bdarnell]

LGTM, although as I said in the gorm version it would be more idiomatic and realistic to make a run_transaction higher-order function instead of doing everything inline.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @rmloveland)

---

_includes/v19.1/app/insecure/basic-sample.py, line 16 at r2 (raw file):

        logging.debug("create_accounts(): status message: {}".format(cur.statusmessage))
        conn.commit()
        cur.close()

cur.close() is not necessary since you're using with conn.cursor().

I think it's better to call conn.commit after the cursor closes (i.e., outside the with block), although I'm not sure.

---

_includes/v19.1/app/insecure/basic-sample.py, line 23 at r2 (raw file):

        cur.execute("SELECT id, balance FROM accounts")
        logging.debug("print_balances(): status message: {}".format(cur.statusmessage))
        conn.commit()

The commit should come after fetchall().

---

_includes/v19.1/app/insecure/basic-sample.py, line 69 at r2 (raw file):

                            UPSERT INTO accounts (id, balance)
                            VALUES
                            (%s, ((SELECT balance FROM accounts WHERE id = %s) - %s)),

An UPSERT with two subselects is a weird way to write this. We already queried the balance above (at least for the from account), so I think the most natural way to write it would be to query both account balances in the first SELECT, then pass those balances in here instead passing in the IDs and re-querying.

There's also the clever solution:

UPDATE accounts SET balance = balance + (IF (id = ?, -1, 1) * ? ) 
    WHERE id IN (?, ?) RETURNING id, balance

(parameters: from, amount, from, to). Look at the returned values and see if the from balance is negative before deciding to commit or abort.

But I'd do the simple thing for examples like this.

[rmloveland]

Thanks, ended up doing that, in addition to addressing other items below. So much nicer!

PTAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)

---

_includes/v19.1/app/insecure/basic-sample.py, line 16 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

cur.close() is not necessary since you're using with conn.cursor().

I think it's better to call conn.commit after the cursor closes (i.e., outside the with block), although I'm not sure.

Deleted cur.close().

Updated to call conn.commit after the with block (It wasn't clear to me from looking at the psycopg.connection docs whether it's better or not, but the code worked after the update.)

---

_includes/v19.1/app/insecure/basic-sample.py, line 23 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

The commit should come after fetchall().

Fixed.

---

_includes/v19.1/app/insecure/basic-sample.py, line 69 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

An UPSERT with two subselects is a weird way to write this. We already queried the balance above (at least for the from account), so I think the most natural way to write it would be to query both account balances in the first SELECT, then pass those balances in here instead passing in the IDs and re-querying.

There's also the clever solution:

UPDATE accounts SET balance = balance + (IF (id = ?, -1, 1) * ? ) 
    WHERE id IN (?, ?) RETURNING id, balance

(parameters: from, amount, from, to). Look at the returned values and see if the from balance is negative before deciding to commit or abort.

But I'd do the simple thing for examples like this.

This should be addressed by the update to use run_transaction HOF wrapper. Now the transfer_funds function executes a SELECT and 2 UPDATEs in the same txn. Hopefully much simpler now.

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/8a89c6c678e352a01af9563f172c3f98ecae1956/

Edited pages:

• v19.1/build-a-python-app-with-cockroachdb.md

[bdarnell]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell)

[ericharmeling]

LGTM!