Am Sun, Sep 08, 2024 at 02:58:03PM +0100 schrieb Rob Cliffe via Python-list:

> >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.

> 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
> that the follwing is less ugly:

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

> try:
> do something
> except:
> log something
> return
> try:
> .commit()
> except:
> 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

On 9/09/24 2:13 am, Karsten Hilbert wrote:
> For what it's worth here's the current state of code:

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.

--
Greg

On Mon, 9 Sep 2024 13:48:32 +1200, Greg Ewing wrote:

> 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?

Am Mon, Sep 09, 2024 at 01:48:32PM +1200 schrieb Greg Ewing via Python-list:

> 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

Am Mon, Sep 09, 2024 at 01:48:32PM +1200 schrieb Greg Ewing via Python-list:

> 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 ?

> 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