psycopg2: proper positioning of .commit() within try: except: blocks

[Karsten Hilbert]

Dear all,

unto now I had been thinking this is a wise idiom (in code
that needs not care whether it fails to do what it tries to
do^1):

conn = psycopg2.connection(...)
curs = conn.cursor()
try:
curs.execute(SOME_SQL)
except PSYCOPG2-Exception:
some logging being done, and, yes, I
can safely inhibit propagation^1
finally:
conn.commit() # will rollback, if SOME_SQL failed
conn.close()

So today I head to learn that conn.commit() may very well
raise a DB related exception, too:

psycopg2.errors.SerializationFailure: could not serialize access due to read/write dependencies among transactions
DETAIL: Reason code: Canceled on identification as a pivot, during commit attempt.
TIP: The transaction might succeed if retried.

Now, what is the proper placement of the .commit() ?

(doing "with ... as conn:" does not free me of committing appropriately)

Should I

try:
curs.execute(SOME_SQL)
conn.commit()
except PSYCOPG2-Exception:
some logging being done, and, yes, I
can safely inhibit propagation^1
finally:
conn.close() # which should .rollback() automagically in case we had not reached to .commit()

?

Thanks for insights,
Karsten

#-------------------------------
^1:

This particular code is writing configuration defaults
supplied in-code when no value is yet to be found in the
database. If it fails, no worries, the supplied default
is used by follow-on code and storing it is re-tried next
time around.

#-------------------------------
Exception details:

Traceback (most recent call last):
File "/usr/share/gnumed/Gnumed/wxpython/gmGuiMain.py", line 3472, in OnInit
frame = gmTopLevelFrame(None, id = -1, title = _('GNUmed client'), size = (640, 440))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/share/gnumed/Gnumed/wxpython/gmGuiMain.py", line 191, in __init__
self.LayoutMgr = gmHorstSpace.cHorstSpaceLayoutMgr(self, -1)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/share/gnumed/Gnumed/wxpython/gmHorstSpace.py", line 215, in __init__
self.top_panel = gmTopPanel.cTopPnl(self, -1)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/share/gnumed/Gnumed/wxpython/gmTopPanel.py", line 52, in __init__
wxgTopPnl.wxgTopPnl.__init__(self, *args, **kwargs)
File "/usr/share/gnumed/Gnumed/wxGladeWidgets/wxgTopPnl.py", line 33, in __init__
self._TCTRL_patient_selector = cActivePatientSelector(self, wx.ID_ANY, "")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/share/gnumed/Gnumed/wxpython/gmPatSearchWidgets.py", line 1295, in __init__
cfg.get2 (
File "/usr/share/gnumed/Gnumed/pycommon/gmCfg.py", line 248, in get2
self.set (
File "/usr/share/gnumed/Gnumed/pycommon/gmCfg.py", line 367, in set
rw_conn.commit() # will rollback if transaction failed
^^^^^^^^^^^^^^^^
psycopg2.errors.SerializationFailure: could not serialize access due to read/write dependencies among transactions
DETAIL: Reason code: Canceled on identification as a pivot, during commit attempt.
TIP: The transaction might succeed if retried.

2024-08-20 22:17:04 INFO gm.cfg [140274204403392 UpdChkThread-148728] (/usr/share/gnumed/Gnumed/pycommon/gmCfg.py::get2() #148): creating option [horstspace.update.consider_latest_branch] with default [True]
2024-08-20 22:17:04 DEBUG gm.db_pool [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmConnectionPool.py::exception_is_connection_loss() #667): interpreting: could not serialize access due to read/write dependencies among transactions
DETAIL: Reason code: Canceled on identification as a pivot, during commit attempt.
TIP: The transaction might succeed if retried.

2024-08-20 22:17:04 DEBUG gm.logging [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmLog2.py::log_stack_trace() #170): exception: could not serialize access due to read/write dependencies among transactions
DETAIL: Reason code: Canceled on identification as a pivot, during commit attempt.
TIP: The transaction might succeed if retried.

2024-08-20 22:17:04 DEBUG gm.logging [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmLog2.py::log_stack_trace() #171): type: <class 'psycopg2.errors.SerializationFailure'>
2024-08-20 22:17:04 DEBUG gm.logging [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmLog2.py::log_stack_trace() #172): list of attributes:
2024-08-20 22:17:04 DEBUG gm.logging [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmLog2.py::log_stack_trace() #178): add_note: <built-in method add_note of SerializationFailure object at 0x7f942a3c9cf0>
2024-08-20 22:17:04 DEBUG gm.logging [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmLog2.py::log_stack_trace() #178): args: ('could not serialize access due to read/write dependencies among transactions\nDETAIL: Reason code: Canceled on identification as a pivot, during commit attempt.\nTIP: The transaction might succeed if retried.\n',)
2024-08-20 22:17:04 DEBUG gm.logging [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmLog2.py::log_stack_trace() #178): cursor: None
2024-08-20 22:17:04 DEBUG gm.logging [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmLog2.py::log_stack_trace() #178): diag: <psycopg2.extensions.Diagnostics object at 0x7f942a2b9e10>
2024-08-20 22:17:04 DEBUG gm.logging [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmLog2.py::log_stack_trace() #178): pgcode: 40001
2024-08-20 22:17:04 DEBUG gm.logging [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmLog2.py::log_stack_trace() #178): pgerror: ERROR: could not serialize access due to read/write dependencies among transactions
DETAIL: Reason code: Canceled on identification as a pivot, during commit attempt.
TIP: The transaction might succeed if retried.

2024-08-20 22:17:04 DEBUG gm.logging [140274459512896 MainThread] (/usr/share/gnumed/Gnumed/pycommon/gmLog2.py::log_stack_trace() #178): with_traceback: <built-in method with_traceback of SerializationFailure object at 0x7f942a3c9cf0>

--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Rob Cliffe]

Post by Karsten Hilbert
Dear all,
unto now I had been thinking this is a wise idiom (in code
that needs not care whether it fails to do what it tries to
conn = psycopg2.connection(...)
curs = conn.cursor()
curs.execute(SOME_SQL)
some logging being done, and, yes, I
can safely inhibit propagation^1
conn.commit() # will rollback, if SOME_SQL failed
conn.close()
So today I head to learn that conn.commit() may very well
psycopg2.errors.SerializationFailure: could not serialize access due to read/write dependencies among transactions
DETAIL: Reason code: Canceled on identification as a pivot, during commit attempt.
TIP: The transaction might succeed if retried.
Now, what is the proper placement of the .commit() ?
(doing "with ... as conn:" does not free me of committing appropriately)
Should I
curs.execute(SOME_SQL)
conn.commit()
some logging being done, and, yes, I
can safely inhibit propagation^1
conn.close() # which should .rollback() automagically in case we had not reached to .commit()
?
Thanks for insights,
Karsten

I would put the curs.execute and the conn.commit in separate
try...except blocks.  That way you know which one failed, and can put
appropriate info in the log, which may help trouble-shooting.
(The general rule is to keep try...except blocks small.  And of course
only catch the exceptions you are interested in, which you seem to be
already doing.)
Best wishes
Rob Cliffe

[Stefan Ram]

The expression after "except" seems to be calculating a difference.

(Lately, I've been seeing some super long subject lines around here,
so here's a thought on how this one could've been worded shorter:
"Positioning of a database .commit() within a try block")

[Karsten Hilbert]

Post by Karsten Hilbert
unto now I had been thinking this is a wise idiom (in code
that needs not care whether it fails to do what it tries to
conn = psycopg2.connection(...)

https://www.psycopg.org/docs/extensions.html#psycopg2.extensions.ISOLATION_LEVEL_SERIALIZABLE
psycopg2.extensions.ISOLATION_LEVEL_SERIALIZABLE

I do indeed.

Or is that in some other concurrent transaction?

In fact in that codebase all transactions -- running
concurrently or not -- are set to SERIALIZABLE.

They are not psycopg2.extensions.ISOLATION_LEVEL_AUTOCOMMIT,
for that matter.

Post by Karsten Hilbert
curs = conn.cursor()
curs.execute(SOME_SQL)
some logging being done, and, yes, I
can safely inhibit propagation^1
conn.commit() # will rollback, if SOME_SQL failed

It will if you use with conn:, otherwise it up to you to do the rollback()
Are you are doing a rollback() in except PSYCOPG2-Exception: ?

No I don't but - to my understanding - an ongoing transaction
is being closed upon termination of the hosting connection.
Unless .commit() is explicitely being issued somewhere in the
code that closing of a transaction will amount to a ROLLBACK.

In case of SQL having failed within a given transaction a
COMMIT will fail-but-rollback, too (explicit ROLLBACK would
succeed while a COMMIT would fail and, in-effect, roll back).

IOW, when SOME_SQL has failed it won't matter that I close
the connection with conn.commit() and it won't matter that
conn.commit() runs a COMMIT on the database -- an open
transaction having run that failed SQL will still roll back
as if ROLLBACK had been issued. Or else my mental model is
wrong.

https://www.psycopg.org/docs/connection.html#connection.close

In the particular case I was writing about the SQL itself
succeeded but then the COMMIT failed due to serialization. I
was wondering about where to best place any needed
conn.commit(). My knee-jerk reaction was to then put it last
in the try: block...

All this is probably more related to Python than to PostgreSQL.

Thanks,
Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Karsten Hilbert]

In the case you show you are doing commit() before the close() so any errors in the
transactions will show up then. My first thought would be to wrap the commit() in a
try/except and deal with error there.

Right, and this was suggested elsewhere ;)

And, yeah, the actual code is much more involved :-D

#------------------------------------------------------------------------
def __safely_close_cursor_and_rollback_close_conn(close_cursor=None, rollback_tx=None, close_conn=None):
if close_cursor:
try:
close_cursor()
except PG_ERROR_EXCEPTION as pg_exc:
_log.exception('cannot close cursor')
gmConnectionPool.log_pg_exception_details(pg_exc)
if rollback_tx:
try:
# need to rollback so ABORT state isn't retained in pooled connections
rollback_tx()
except PG_ERROR_EXCEPTION as pg_exc:
_log.exception('cannot rollback transaction')
gmConnectionPool.log_pg_exception_details(pg_exc)
if close_conn:
try:
close_conn()
except PG_ERROR_EXCEPTION as pg_exc:
_log.exception('cannot close connection')
gmConnectionPool.log_pg_exception_details(pg_exc)

#------------------------------------------------------------------------
def run_rw_queries (
link_obj:_TLnkObj=None,
queries:_TQueries=None,
end_tx:bool=False,
return_data:bool=None,
get_col_idx:bool=False,
verbose:bool=False
) -> tuple[list[dbapi.extras.DictRow], dict[str, int] | None]:
"""Convenience function for running read-write queries.

Typically (part of) a transaction.

Args:
link_obj: None, cursor, connection
queries:

* a list of dicts [{'cmd': <string>, 'args': <dict> or <tuple>)
* to be executed as a single transaction
* the last query may usefully return rows, such as:

SELECT currval('some_sequence');
or
INSERT/UPDATE ... RETURNING some_value;

end_tx:

* controls whether the transaction is finalized (eg.
COMMITted/ROLLed BACK) or not, this allows the
call to run_rw_queries() to be part of a framing
transaction
* if link_obj is a *connection* then "end_tx" will
default to False unless it is explicitly set to
True which is taken to mean "yes, you do have full
control over the transaction" in which case the
transaction is properly finalized
* if link_obj is a *cursor* we CANNOT finalize the
transaction because we would need the connection for that
* if link_obj is *None* "end_tx" will, of course, always
be True, because we always have full control over the
connection, not ending the transaction would be pointless

return_data:

* if true, the returned data will include the rows
the last query selected
* if false, it returns None instead

get_col_idx:

* True: the returned tuple will include a dictionary
mapping field names to column positions
* False: the returned tuple includes None instead of a field mapping dictionary

Returns:

* (None, None) if last query did not return rows
* ("fetchall() result", <index>) if last query returned any rows and "return_data" was True

* for *index* see "get_col_idx"
"""
assert queries is not None, '<queries> must not be None'

if link_obj is None:
conn = get_connection(readonly = False)
curs = conn.cursor()
conn_close = conn.close
tx_commit = conn.commit
tx_rollback = conn.rollback
curs_close = curs.close
notices_accessor = conn
else:
conn_close = lambda *x: None
tx_commit = lambda *x: None
tx_rollback = lambda *x: None
curs_close = lambda *x: None
if isinstance(link_obj, dbapi._psycopg.cursor):
curs = link_obj
notices_accessor = curs.connection
elif isinstance(link_obj, dbapi._psycopg.connection):
if end_tx:
tx_commit = link_obj.commit
tx_rollback = link_obj.rollback
curs = link_obj.cursor()
curs_close = curs.close
notices_accessor = link_obj
else:
raise ValueError('link_obj must be cursor, connection or None but not [%s]' % link_obj)

for query in queries:
try:
args = query['args']
except KeyError:
args = None
try:
curs.execute(query['cmd'], args)
if verbose:
gmConnectionPool.log_cursor_state(curs)
for notice in notices_accessor.notices:
_log.debug(notice.replace('\n', '/').replace('\n', '/'))
del notices_accessor.notices[:]
# DB related exceptions
except dbapi.Error as pg_exc:
_log.error('query failed in RW connection')
gmConnectionPool.log_pg_exception_details(pg_exc)
for notice in notices_accessor.notices:
_log.debug(notice.replace('\n', '/').replace('\n', '/'))
del notices_accessor.notices[:]
__safely_close_cursor_and_rollback_close_conn (
curs_close,
tx_rollback,
conn_close
)
# privilege problem ?
if pg_exc.pgcode == PG_error_codes.INSUFFICIENT_PRIVILEGE:
details = 'Query: [%s]' % curs.query.decode(errors = 'replace').strip().strip('\n').strip().strip('\n')
if curs.statusmessage != '':
details = 'Status: %s\n%s' % (
curs.statusmessage.strip().strip('\n').strip().strip('\n'),
details
)
if pg_exc.pgerror is None:
msg = '[%s]' % pg_exc.pgcode
else:
msg = '[%s]: %s' % (pg_exc.pgcode, pg_exc.pgerror)
raise gmExceptions.AccessDenied (
msg,
source = 'PostgreSQL',
code = pg_exc.pgcode,
details = details
)

# other DB problem
gmLog2.log_stack_trace()
raise

# other exception
except Exception:
_log.exception('error running query in RW connection')
gmConnectionPool.log_cursor_state(curs)
for notice in notices_accessor.notices:
_log.debug(notice.replace('\n', '/').replace('\n', '/'))
del notices_accessor.notices[:]
gmLog2.log_stack_trace()
__safely_close_cursor_and_rollback_close_conn (
curs_close,
tx_rollback,
conn_close
)
raise

data = None
col_idx = None
if return_data:
try:
data = curs.fetchall()
except Exception:
_log.exception('error fetching data from RW query')
gmLog2.log_stack_trace()
__safely_close_cursor_and_rollback_close_conn (
curs_close,
tx_rollback,
conn_close
)
raise

if get_col_idx:
col_idx = get_col_indices(curs)
curs_close()
tx_commit()
conn_close()
return (data, col_idx)

#------------------------------------------------------------------------


Best,
Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Karsten Hilbert]

Post by Karsten Hilbert
Right, and this was suggested elsewhere ;)
And, yeah, the actual code is much more involved :-D

I see that.
The question is does the full code you show fail?
The code sample you show in your original post is doing something very different then
what you show in your latest post. At this point I do not understand the exact problem
we are dealing with.

We are not dealing with an unsolved problem. I had been
asking for advice where to best place that .commit() call in
case I am overlooking benefits and drawbacks of choices.

The

try:
do something
except:
log something
finally:
.commit()

cadence is fairly Pythonic and elegant in that it ensures the
the .commit() will always be reached regardless of exceptions
being thrown or not and them being handled or not.

It is also insufficient because the .commit() itself may
elicit exceptions (from the database).

So there's choices:

Ugly:

try:
do something
except:
log something
finally:
try:
.commit()
except:
log some more

Fair but not feeling quite safe:

try:
do something
.commit()
except:
log something

Boring and repetitive and safe(r):

try:
do something
except:
log something
try:
.commit()
except:
log something

I eventually opted for the last version, except for factoring
out the second try: except: block.

Best,
Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Greg Ewing]

Post by Karsten Hilbert
do something
log something
.commit()
cadence is fairly Pythonic and elegant in that it ensures the
the .commit() will always be reached regardless of exceptions
being thrown or not and them being handled or not.

That seems wrong to me. I would have thought the commit should only
be attempted if everything went right.

What if there's a problem in your code that causes a non-SQL-related
exception when some but not all of the SQL statements in the
transaction bave been issued? The database doesn't know something
has gone wrong, so it will happily commit a partially-completed
transaction and possibly corrupt your data.

This is how I normally do things like this:

try:
do something
.commit()
except:
log something
.rollback()

Doing an explicit rollback ensures that the transaction is always
rolled back if it is interrupted for any reason.

[Jon Ribbens]

Post by Greg Ewing

Post by Karsten Hilbert
do something
log something
.commit()
cadence is fairly Pythonic and elegant in that it ensures the
the .commit() will always be reached regardless of exceptions
being thrown or not and them being handled or not.

That seems wrong to me. I would have thought the commit should only
be attempted if everything went right.
What if there's a problem in your code that causes a non-SQL-related
exception when some but not all of the SQL statements in the
transaction bave been issued? The database doesn't know something
has gone wrong, so it will happily commit a partially-completed
transaction and possibly corrupt your data.
do something
.commit()
log something
.rollback()
Doing an explicit rollback ensures that the transaction is always
rolled back if it is interrupted for any reason.

What if there's an exception in your exception handler? I'd put the
rollback in the 'finally' handler, so it's always called. If you've
already called 'commit' then the rollback does nothing of course.

[Stefan Ram]

Post by Jon Ribbens
What if there's an exception in your exception handler? I'd put the
rollback in the 'finally' handler, so it's always called.

To make this happen, you'd better kick off with the rollback right
at the beginning of the "finally:" block. Otherwise, some gnarly
exception might rear its ugly head before the rollback gets called!

[Lawrence D'Oliveiro]

Post by Jon Ribbens
What if there's an exception in your exception handler? I'd put the
rollback in the 'finally' handler, so it's always called. If you've
already called 'commit' then the rollback does nothing of course.

In any DBMS worth its salt, rollback is something that happens
automatically if the transaction should fail to complete for any reason.

This applies for any failure reason, up to and including a program or
system crash.

[Jon Ribbens]

Post by Lawrence D'Oliveiro

Post by Jon Ribbens
What if there's an exception in your exception handler? I'd put the
rollback in the 'finally' handler, so it's always called. If you've
already called 'commit' then the rollback does nothing of course.

In any DBMS worth its salt, rollback is something that happens
automatically if the transaction should fail to complete for any reason.
This applies for any failure reason, up to and including a program or
system crash.

If it's a program or system crash, sure, but anything less than that -
how would the database even know, unless the program told it?

[Lawrence D'Oliveiro]

Post by Jon Ribbens

Post by Lawrence D'Oliveiro

Post by Jon Ribbens
What if there's an exception in your exception handler? I'd put the
rollback in the 'finally' handler, so it's always called. If you've
already called 'commit' then the rollback does nothing of course.

In any DBMS worth its salt, rollback is something that happens
automatically if the transaction should fail to complete for any reason.
This applies for any failure reason, up to and including a program or
system crash.

If it's a program or system crash, sure, but anything less than that -
how would the database even know, unless the program told it?

The database only needs to commit when it is explicitly told. Anything
less -- no commit.

[Jon Ribbens]

Post by Lawrence D'Oliveiro

Post by Jon Ribbens

Post by Lawrence D'Oliveiro

Post by Jon Ribbens
What if there's an exception in your exception handler? I'd put the
rollback in the 'finally' handler, so it's always called. If you've
already called 'commit' then the rollback does nothing of course.

In any DBMS worth its salt, rollback is something that happens
automatically if the transaction should fail to complete for any reason.
This applies for any failure reason, up to and including a program or
system crash.

If it's a program or system crash, sure, but anything less than that -
how would the database even know, unless the program told it?

The database only needs to commit when it is explicitly told. Anything
less -- no commit.

So the Python code is half-way through a transaction when it throws
a (non-database-related) exception and that thread of execution is
aborted. The database connection returns to the pool, and is re-used
by another thread which continues using it to perform a different
sequence of operations ... ending in a COMMIT, which commits
one-and-a-half transactions.

[Karsten Hilbert]

Post by Jon Ribbens

Post by Lawrence D'Oliveiro
The database only needs to commit when it is explicitly told. Anything
less -- no commit.

So the Python code is half-way through a transaction when it throws
a (non-database-related) exception and that thread of execution is
aborted. The database connection returns to the pool, and is re-used
by another thread which continues using it to perform a different
sequence of operations ... ending in a COMMIT, which commits
one-and-a-half transactions.

Right, but that's true only when writable connections are
being pooled, which should be avoidable in many cases.

Any pool worth its salt should rollback any potentially
pending transactions of a connection when it is given back
that pooled connection. Unless explicitely told not to.

Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Karsten Hilbert]

Post by Jon Ribbens
So the Python code is half-way through a transaction when it throws
a (non-database-related) exception and that thread of execution is
aborted. The database connection returns to the pool,

How does it return to the pool ?

Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Jon Ribbens]

Post by Karsten Hilbert

Post by Jon Ribbens
So the Python code is half-way through a transaction when it throws
a (non-database-related) exception and that thread of execution is
aborted. The database connection returns to the pool,

How does it return to the pool ?

It's just any circumstance in which a bit of your code uses a database
"cursor" (which isn't a cursor) that it didn't create moments before.

[Lawrence D'Oliveiro]

Post by Lawrence D'Oliveiro
The database only needs to commit when it is explicitly told. Anything
less -- no commit.

So the Python code is half-way through a transaction when it throws a
(non-database-related) exception and that thread of execution is
aborted. The database connection returns to the pool ...

The DBMS connection is deleted. The DBMS discards all context created for
this connection, including any transactions in progress. Gone.

The database structures on persistent storage are also carefully designed
with transaction safety in mind. So any partial transaction data saved on
persistent storage that remains after a system crash can be identified as
such and discarded, leaving the database in its pre-transaction state.

[Jon Ribbens]

Post by Lawrence D'Oliveiro

Post by Lawrence D'Oliveiro
The database only needs to commit when it is explicitly told. Anything
less -- no commit.

So the Python code is half-way through a transaction when it throws a
(non-database-related) exception and that thread of execution is
aborted. The database connection returns to the pool ...

The DBMS connection is deleted.

How does that happen then?

[Lawrence D'Oliveiro]

Post by Jon Ribbens

Post by Lawrence D'Oliveiro

Post by Lawrence D'Oliveiro
The database only needs to commit when it is explicitly told.
Anything less -- no commit.

So the Python code is half-way through a transaction when it throws a
(non-database-related) exception and that thread of execution is
aborted. The database connection returns to the pool ...

The DBMS connection is deleted.

How does that happen then?

You write code to do it.

[Jon Ribbens]

Post by Lawrence D'Oliveiro

Post by Jon Ribbens

Post by Lawrence D'Oliveiro

Post by Lawrence D'Oliveiro
The database only needs to commit when it is explicitly told.
Anything less -- no commit.

So the Python code is half-way through a transaction when it throws a
(non-database-related) exception and that thread of execution is
aborted. The database connection returns to the pool ...

The DBMS connection is deleted.

How does that happen then?

You write code to do it.

Ok. So we've moved away from "In any DBMS worth its salt, rollback is
something that happens automatically" and now you're saying it isn't
automatic after all, "you write code to do it". That was my point.
The database provides the tools, but it isn't psychic.

[Karsten Hilbert]

Post by Jon Ribbens
Ok. So we've moved away from "In any DBMS worth its salt, rollback is
something that happens automatically"

Nope. The original post asked something entirely different.

Post by Jon Ribbens
and now you're saying it isn't automatic after all,

No again, such shenanigans only start to happen when pooling
is brought into the equation.

Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Jon Ribbens]

Post by Karsten Hilbert

Post by Jon Ribbens
Ok. So we've moved away from "In any DBMS worth its salt, rollback is
something that happens automatically"

Nope. The original post asked something entirely different.

No it didn't.

Post by Karsten Hilbert

Post by Jon Ribbens
and now you're saying it isn't automatic after all,

No again, such shenanigans only start to happen when pooling
is brought into the equation.

No they don't.

[Lawrence D'Oliveiro]

Post by Jon Ribbens

Post by Lawrence D'Oliveiro

Post by Jon Ribbens

Post by Lawrence D'Oliveiro

Post by Jon Ribbens

Post by Lawrence D'Oliveiro
The database only needs to commit when it is explicitly told.
Anything less -- no commit.

So the Python code is half-way through a transaction when it throws
a (non-database-related) exception and that thread of execution is
aborted. The database connection returns to the pool ...

The DBMS connection is deleted.

How does that happen then?

You write code to do it.

Ok. So we've moved away from "In any DBMS worth its salt, rollback is
something that happens automatically" and now you're saying it isn't
automatic after all, "you write code to do it".

The database code already performs that function. As far as the client is
concerned, the function happens automatically.

And it’s not just code, it’s data. The database structures on persistent
storage are also carefully designed with transaction safety in mind. So
any partial transaction data saved on persistent storage that remains
after a system crash can be identified as such and discarded, leaving the
database in its pre-transaction state.

[Jon Ribbens]

Post by Lawrence D'Oliveiro

Post by Jon Ribbens

Post by Lawrence D'Oliveiro

Post by Jon Ribbens

Post by Lawrence D'Oliveiro

Post by Jon Ribbens

Post by Lawrence D'Oliveiro
The database only needs to commit when it is explicitly told.
Anything less -- no commit.

So the Python code is half-way through a transaction when it throws
a (non-database-related) exception and that thread of execution is
aborted. The database connection returns to the pool ...

The DBMS connection is deleted.

How does that happen then?

You write code to do it.

Ok. So we've moved away from "In any DBMS worth its salt, rollback is
something that happens automatically" and now you're saying it isn't
automatic after all, "you write code to do it".

The database code already performs that function. As far as the client is
concerned, the function happens automatically.

... but only if "you write code to do it".

Post by Lawrence D'Oliveiro
And it’s not just code, it’s data. The database structures on persistent
storage are also carefully designed with transaction safety in mind. So
any partial transaction data saved on persistent storage that remains
after a system crash can be identified as such and discarded, leaving the
database in its pre-transaction state.

Yes, nobody's disputing that. A good database will do what you tell it,
and keep the data you give it. But what if you tell it the wrong thing
or give it the wrong data? It's like, for example, a RAID array will
save you from a faulty disk, but will not save you from the software
writing incorrect data, which the RAID array will then faithfully copy
across to all the disks overwriting the good data.

[Lawrence D'Oliveiro]

But what if you tell it the wrong thing ...

To get back to the original point of this thread, all that rigmarole to
try to ensure to call “rollback” in case of an exception is completely
unnecessary: the DBMS will take care of that for you.

[Jon Ribbens]

Post by Lawrence D'Oliveiro

But what if you tell it the wrong thing ...

To get back to the original point of this thread, all that rigmarole to
try to ensure to call “rollback” in case of an exception is completely
unnecessary: the DBMS will take care of that for you.

No, it won't.

[Greg Ewing]

Post by Jon Ribbens

Post by Karsten Hilbert
do something
.commit()
log something
.rollback()

What if there's an exception in your exception handler? I'd put the
rollback in the 'finally' handler, so it's always called.

Good point. Putting the rollback first would be safer/

[Karsten Hilbert]

Post by Greg Ewing

Post by Karsten Hilbert
do something
log something
.commit()
cadence is fairly Pythonic and elegant in that it ensures the
the .commit() will always be reached regardless of exceptions
being thrown or not and them being handled or not.

That seems wrong to me. I would have thought the commit should only
be attempted if everything went right.
What if there's a problem in your code that causes a non-SQL-related
exception when some but not all of the SQL statements in the
transaction bave been issued? The database doesn't know something
has gone wrong, so it will happily commit a partially-completed
transaction and possibly corrupt your data.

A-ha !

try:
run_some_SQL_that_succeeds()
print(no_such_name) # tongue-in-cheek
1 / 0 # for good measure
except SOME_DB_ERROR:
print('some DB error, can be ignored for now')
finally:
commit()

which is wrong, given that the failing *Python* statements
may very well belong into the *business level* "transaction"
which a/the database transaction is part of.

See, that's why I was asking in the first place :-)

I was overlooking implications.

Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Karsten Hilbert]

Post by Greg Ewing

Post by Karsten Hilbert
do something
log something
.commit()
cadence is fairly Pythonic and elegant in that it ensures the
the .commit() will always be reached regardless of exceptions
being thrown or not and them being handled or not.

That seems wrong to me. I would have thought the commit should only
be attempted if everything went right.

It is only attempted when "everything" went right. The fault
in my thinking was what the "everything" might encompass.
When some SQL fails it won't matter whether I say
conn.commit() or conn.rollback() or, in fact, nothing at all
-- the (DB !) transaction will be rolled back in any case.

Post by Greg Ewing
What if there's a problem in your code that causes a non-SQL-related
exception when some but not all of the SQL statements in the
transaction bave been [-- even successfully --] issued?
do something
.commit()
log something
.rollback()

... but ...

Post by Greg Ewing
Doing an explicit rollback ensures that the transaction is always
rolled back if it is interrupted for any reason.

explicit is better than implicit ;-)

Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Lawrence D'Oliveiro]

do something .commit()
log something .rollback()
Doing an explicit rollback ensures that the transaction is always rolled
back if it is interrupted for any reason.

Don’t bother. Let the DBMS deal with that for you. That’s what it’s for.

Also, maybe you meant “finally” instead of “except”?

[Rob Cliffe]

Post by Karsten Hilbert

Post by Karsten Hilbert
Right, and this was suggested elsewhere ;)
And, yeah, the actual code is much more involved :-D

I see that.
The question is does the full code you show fail?
The code sample you show in your original post is doing something very different then
what you show in your latest post. At this point I do not understand the exact problem
we are dealing with.

We are not dealing with an unsolved problem. I had been
asking for advice where to best place that .commit() call in
case I am overlooking benefits and drawbacks of choices.
The
do something
log something
.commit()
cadence is fairly Pythonic and elegant in that it ensures the
the .commit() will always be reached regardless of exceptions
being thrown or not and them being handled or not.
It is also insufficient because the .commit() itself may
elicit exceptions (from the database).
do something
log something
.commit()
log some more
do something
.commit()
log something
do something
log something
.commit()
log something
I eventually opted for the last version, except for factoring
out the second try: except: block.
Best,
Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

Unless I'm missing something, the 1st & 3rd versions always do the
commit() even after the first bit fails, which seems wrong.
I suggest the 1st version but replacing "finally" by "else".  Then the
try-commit-except will not be executed if the "something" fails.
Perhaps the extra indentation of the second try block is a bit ugly, but
it is more important that it does the right thing.
If it is convenient (it may not be) to put the whole thing in a
function, you may feel that the follwing is less ugly:

try:
do something
except:
log something
return
try:
.commit()
except:
log some more
return

Best wishes
Rob Cliffe

[Karsten Hilbert]

Post by Karsten Hilbert
do something
log something
.commit()
log some more
do something
.commit()
log something
do something
log something
.commit()
log something
I eventually opted for the last version, except for factoring
out the second try: except: block.

Unless I'm missing something, the 1st & 3rd versions always do the commit() even after
the first bit fails, which seems wrong.

Well, it does run a Python function called "commit". That
function will call "COMMIT" on the database. The end result
will be correct (at the SQL level) because the COMMIT will
not effect a durable commit of data when the SQL in "do
something" had failed.

We have, however, elicited that there may well be other
things belonging into the running business level transaction
which may fail and which should lead to data not being
committed despite being technically correct at the SQL level.

I suggest the 1st version but replacing "finally" by "else".  Then the try-commit-except
will not be executed if the "something" fails.

The whole point was to consolidate the commit into one place.
It is unfortunately named, though. It should be called
"close_transaction".

Perhaps the extra indentation of the second try block is a bit ugly, but it is more
important that it does the right thing.
If it is convenient (it may not be) to put the whole thing in a function, you may feel

The whole thing does reside inside a function but the exit-early pattern

do something
log something
return
.commit()
log some more
return

won't help as there's more stuff to be done inside that function.

Thanks,
Karsten


For what it's worth here's the current state of code:

#------------------------------------------------------------------------
def __safely_close_cursor_and_rollback_close_conn(close_cursor=None, rollback_tx=None, close_conn=None):
if close_cursor:
try:
close_cursor()
except PG_ERROR_EXCEPTION as pg_exc:
_log.exception('cannot close cursor')
gmConnectionPool.log_pg_exception_details(pg_exc)
if rollback_tx:
try:
# need to rollback so ABORT state isn't retained in pooled connections
rollback_tx()
except PG_ERROR_EXCEPTION as pg_exc:
_log.exception('cannot rollback transaction')
gmConnectionPool.log_pg_exception_details(pg_exc)
if close_conn:
try:
close_conn()
except PG_ERROR_EXCEPTION as pg_exc:
_log.exception('cannot close connection')
gmConnectionPool.log_pg_exception_details(pg_exc)

#------------------------------------------------------------------------
def __log_notices(notices_accessor=None):
for notice in notices_accessor.notices:
_log.debug(notice.replace('\n', '/').replace('\n', '/'))
del notices_accessor.notices[:]

#------------------------------------------------------------------------
def __perhaps_reraise_as_permissions_error(pg_exc, curs):
if pg_exc.pgcode != PG_error_codes.INSUFFICIENT_PRIVILEGE:
return

# privilege problem -- normalize as GNUmed exception
details = 'Query: [%s]' % curs.query.decode(errors = 'replace').strip().strip('\n').strip().strip('\n')
if curs.statusmessage != '':
details = 'Status: %s\n%s' % (
curs.statusmessage.strip().strip('\n').strip().strip('\n'),
details
)
if pg_exc.pgerror is None:
msg = '[%s]' % pg_exc.pgcode
else:
msg = '[%s]: %s' % (pg_exc.pgcode, pg_exc.pgerror)
raise gmExceptions.AccessDenied (
msg,
source = 'PostgreSQL',
code = pg_exc.pgcode,
details = details
)

#------------------------------------------------------------------------
def run_rw_queries (
link_obj:_TLnkObj=None,
queries:_TQueries=None,
end_tx:bool=False,
return_data:bool=None,
get_col_idx:bool=False,
verbose:bool=False
) -> tuple[list[dbapi.extras.DictRow], dict[str, int] | None]:
"""Convenience function for running read-write queries.

Typically (part of) a transaction.

Args:
link_obj: None, cursor, connection
queries:

* a list of dicts [{'cmd': <string>, 'args': <dict> or <tuple>)
* to be executed as a single transaction
* the last query may usefully return rows, such as:

SELECT currval('some_sequence');
or
INSERT/UPDATE ... RETURNING some_value;

end_tx:

* controls whether the transaction is finalized (eg.
COMMITted/ROLLed BACK) or not, this allows the
call to run_rw_queries() to be part of a framing
transaction
* if link_obj is a *connection* then "end_tx" will
default to False unless it is explicitly set to
True which is taken to mean "yes, you do have full
control over the transaction" in which case the
transaction is properly finalized
* if link_obj is a *cursor* we CANNOT finalize the
transaction because we would need the connection for that
* if link_obj is *None* "end_tx" will, of course, always
be True, because we always have full control over the
connection, not ending the transaction would be pointless

return_data:

* if true, the returned data will include the rows
the last query selected
* if false, it returns None instead

get_col_idx:

* True: the returned tuple will include a dictionary
mapping field names to column positions
* False: the returned tuple includes None instead of a field mapping dictionary

Returns:

* (None, None) if last query did not return rows
* ("fetchall() result", <index>) if last query returned any rows and "return_data" was True

* for *index* see "get_col_idx"
"""
assert queries is not None, '<queries> must not be None'
assert isinstance(link_obj, (dbapi._psycopg.connection, dbapi._psycopg.cursor, type(None))), '<link_obj> must be None, a cursor, or a connection, but [%s] is of type (%s)' % (link_obj, type(link_obj))

if link_obj is None:
conn = get_connection(readonly = False)
curs = conn.cursor()
conn_close = conn.close
tx_commit = conn.commit
tx_rollback = conn.rollback
curs_close = curs.close
notices_accessor = conn
else:
conn_close = lambda *x: None
tx_commit = lambda *x: None
tx_rollback = lambda *x: None
curs_close = lambda *x: None
if isinstance(link_obj, dbapi._psycopg.cursor):
curs = link_obj
notices_accessor = curs.connection
elif isinstance(link_obj, dbapi._psycopg.connection):
curs = link_obj.cursor()
curs_close = curs.close
notices_accessor = link_obj
if end_tx:
tx_commit = link_obj.commit
tx_rollback = link_obj.rollback
for query in queries:
try:
args = query['args']
except KeyError:
args = None
try:
curs.execute(query['cmd'], args)
if verbose:
gmConnectionPool.log_cursor_state(curs)
__log_notices(notices_accessor)
# DB related exceptions
except dbapi.Error as pg_exc:
_log.error('query failed in RW connection')
gmConnectionPool.log_pg_exception_details(pg_exc)
__log_notices(notices_accessor)
__safely_close_cursor_and_rollback_close_conn (
curs_close,
tx_rollback,
conn_close
)
__perhaps_reraise_as_permissions_error(pg_exc, curs)
# not a permissions problem
gmLog2.log_stack_trace()
raise

# other exceptions
except Exception:
_log.exception('error running query in RW connection')
gmConnectionPool.log_cursor_state(curs)
__log_notices(notices_accessor)
gmLog2.log_stack_trace()
__safely_close_cursor_and_rollback_close_conn (
curs_close,
tx_rollback,
conn_close
)
raise

if not return_data:
curs_close()
tx_commit()
conn_close()
return (None, None)

data = None
try:
data = curs.fetchall()
except Exception:
_log.exception('error fetching data from RW query')
gmLog2.log_stack_trace()
__safely_close_cursor_and_rollback_close_conn (
curs_close,
tx_rollback,
conn_close
)
raise

col_idx = None
if get_col_idx:
col_idx = get_col_indices(curs)
curs_close()
tx_commit()
conn_close()
return (data, col_idx)

#------------------------------------------------------------------------

--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Greg Ewing]

That code doesn't inspire much confidence in me. It's far too
convoluted with too much micro-management of exceptions.

I would much prefer to have just *one* place where exceptions are
caught and logged.

[Lawrence D'Oliveiro]

I would much prefer to have just *one* place where exceptions are caught
and logged.

Why catch exceptions at all? The only kind of database-related exception
I’ve felt the need to catch so far is the occasional IntegrityError from
trying to insert a record with a duplicate key (and that only in certain
situations). Anything else (particularly SQL syntax errors) I would rather
just leave to the default exception-handling mechanism -- why waste time
reinventing its information-reporting abilities?

[Karsten Hilbert]

Post by Greg Ewing
That code doesn't inspire much confidence in me. It's far too
convoluted with too much micro-management of exceptions.
I would much prefer to have just *one* place where exceptions are
caught and logged.

I am open to suggestions.

Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B

[Karsten Hilbert]

Post by Greg Ewing
That code doesn't inspire much confidence in me. It's far too
convoluted with too much micro-management of exceptions.

It is catching two exceptions, re-raising both of them,
except for re-raising one of them as another kind of
exception. What would you doing differently and how ?

Post by Greg Ewing
I would much prefer to have just *one* place where exceptions are
caught and logged.

There's, of course, a top level handler which logs and
user-displays-as-appropriate any exceptions. This is code
from a much larger codebase.

Karsten
--
GPG 40BE 5B0E C98E 1713 AFA6 5BC0 3BEA AC80 7D4F C89B