koji

#2837 failed queries are always logged as errors, even when expected
Closed: Fixed by tkopecek. Opened by mikem.

Closed: Fixed
Reopen Issue

For the most part, a failed query is an error and it makes sense to log it. However, there are a few places where we expect them to happen regularly. One such place in in the recent updates to the protonmsg plugin, which has:

    c.execute('BEGIN')
    try:
        c.execute('LOCK TABLE proton_queue IN ACCESS EXCLUSIVE MODE NOWAIT')
    except psycopg2.OperationalError:
        LOG.debug('skipping db queue due to lock')
        return

This part of the code logs the failure at the debug level, which is fine, but the underlying code in execute() will always log the query as an error.

        try:
            ret = self.cursor.execute(operation, parameters)
        except Exception:
            self.logger.error('Query failed. Query was: %s', self.quote(operation, parameters))
            raise

This can make for a lot of scary looking noise in the logs. The protonmsg plugin hits this quite frequently.

I think the only other time we might care is when we use Savepoint.

Metadata Update from @mikem:
- Custom field Size adjusted to None

I'm not sure what the best approach is here, hence the discussion tag. I see a few options:

  1. add an optional arg to CursorWrapper.execute() and _dml() (passing through to the latter) that disables error logging for the call
  2. as above, but extend this option passthrough to other places where queries are generated (e.g. InsertProcessor, UpdateProcessor, QueryProcessor, _singleValue, _fetchMulti, _multiRow)
  3. add some sort of flag in koji.db (would have to be a threadlocal)
  4. stop logging the error in execute() and instead raise a more robust error

I lean towards 1.

Option 1 really only solves the problem for protonmsg, and it feels a little strange since we're trying to get away from using the older, more direct sql functions. However, it is a simple solution.

Option 2 seems like way to many function signatures to change for such a small effect.

Option 3 seems like questionable design

Option 4 is a simple solution. In the past, masking exceptions was a bad pattern, but with the advent of pep-3134, this is less of an issue. On the other hand, we probably don't want to return raw queries to clients (though we could probably find a way to avoid that).

I'm also for 1) It looks to me, that it should be very explicit that query is expected to fail.

Metadata Update from @tkopecek:
- Issue set to the milestone: 1.28

PR #3173

Metadata Update from @jcupova:
- Issue tagged with: testing-ready

Commit 7fe0d10d fixes this issue

Commit e7db7d8b fixes this issue

Metadata Update from @mfilip:
- Issue tagged with: testing-done

This issue has been migrated to Fedora Forge:
https://forge.fedoraproject.org/koji/koji/issues/2837

Please continue any further discussion there.
Metadata
None

discussion testing-done testing-ready

None
None
Low
None
Related Pull Requests
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.