Opened 7 years ago

Closed 5 years ago

# Database schema is incorrect or casts are needed (PostgreSQL 8.4 database)

Reported by: Owned by: dbely Robert Corsaro normal AnnouncerPlugin critical patch database API Steffen Hoffmann 0.12

### Description

The database schema you use during upgrade seems to have incorrect field types:

    SCHEMA = [
Table('subscription', key='id')[
Column('id', auto_increment=True),
Column('time', type='int64'),
Column('changetime', type='int64'),
Column('class'),
Column('sid'),
Column('authenticated', type='int'),
Column('distributor'),
Column('format'),
Column('priority', type='int'),
],
Table('subscription_attribute', key='id')[
Column('id', auto_increment=True),
Column('sid'),
Column('authenticated', type='int'),
Column('class'),
Column('realm'),
Column('target')
]
]



Specifically:

1. What type should have 'authenticated' field? It's declared as int but used as bool. Due to that in many places I have a crash like following:
  File "/usr/local/lib/python2.6/dist-packages/TracAnnouncer-0.12.1.dev-py2.6.egg/announcer/model.py", line 350, in do_select
""", (sid,authenticated,klass))
File "/usr/lib/python2.6/dist-packages/trac/db/util.py", line 122, in execute
return self.cursor.execute(sql_escape_percent(sql), args)
ProgrammingError: operator does not exist: integer = boolean
LINE 5:                  AND authenticated=true
^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.


Or maybe the filed type is correct but explicit type casts should be added indeed?

1. What type should have 'time' fields?
  File "/usr/local/lib/python2.6/dist-packages/TracAnnouncer-0.12.1.dev-py2.6.egg/announcer/model.py", line 79, in do_insert
subscription['class']))
File "/usr/lib/python2.6/dist-packages/trac/db/util.py", line 122, in execute
return self.cursor.execute(sql_escape_percent(sql), args)
ProgrammingError: column "time" is of type bigint but expression is of type timestamp with time zone
LINE 5:                  VALUES (CURRENT_TIMESTAMP, CURRENT_TIMESTAM...
^
HINT:  You will need to rewrite or cast the expression.


I have fixed that with

VALUES (EXTRACT(EPOCH FROM CURRENT_TIMESTAMP), EXTRACT(EPOCH FROM CURRENT_TIMESTAMP), %s, %s, %s, %s, %s, %s, %s)


But I don't know if it's the correct way.

Probably there are other similar bugs that should be fixed as well.

### comment:1 follow-up:  2 Changed 7 years ago by Oliver Metz

I tried to fix the issues you mentioned above. Attached patch is what I'm running at the moment. Don't know if I catched up all errors...

btw. The authenticated field in trac's session table is int, too.

### comment:2 in reply to:  1 Changed 7 years ago by dbely

I tried to fix the issues you mentioned above. Attached patch is what I'm running at the moment. Don't know if I catched up all errors...

btw. The authenticated field in trac's session table is int, too.

Thanks. This part looks like to be working. Since then I've found another similar problem: "delete ticket <number>" command crashes trac-admin with error

  File "/usr/local/lib/python2.6/dist-packages/Trac-0.12.2dev_r10363-py2.6.egg/trac/ticket/admin.py", line 841, in do_remove
ticket.delete()
File "/usr/local/lib/python2.6/dist-packages/Trac-0.12.2dev_r10363-py2.6.egg/trac/ticket/model.py", line 421, in delete
listener.ticket_deleted(self)
File "/usr/local/lib/python2.6/dist-packages/TracAnnouncer-0.12.1.dev-py2.6.egg/announcer/subscribers.py", line 833, in ticket_deleted
self.env, klass, 'ticket', ticket.id)
File "/usr/local/lib/python2.6/dist-packages/TracAnnouncer-0.12.1.dev-py2.6.egg/announcer/model.py", line 326, in delete_by_class_realm_and_target
@env.with_transaction(db)
File "/usr/local/lib/python2.6/dist-packages/Trac-0.12.2dev_r10363-py2.6.egg/trac/db/api.py", line 73, in transaction_wrapper
fn(ldb)
File "/usr/local/lib/python2.6/dist-packages/TracAnnouncer-0.12.1.dev-py2.6.egg/announcer/model.py", line 334, in do_delete
""", (realm, klass, target))
File "/usr/local/lib/python2.6/dist-packages/Trac-0.12.2dev_r10363-py2.6.egg/trac/db/util.py", line 65, in execute
return self.cursor.execute(sql_escape_percent(sql), args)
ProgrammingError: operator does not exist: text = integer
LINE 5:                     AND target = 508
^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.
ProgrammingError: operator does not exist: text = integer


I have fixed this with

     def delete_by_class_realm_and_target(cls, env, klass, realm, target, db=None):
@env.with_transaction(db)
def do_delete(db):
cursor = db.cursor()
cursor.execute("""
DELETE FROM subscription_attribute
WHERE realm = %s
AND class = %s
AND target = %s
-            """, (realm, klass, target))
+            """, (realm, klass, str(target)))


but probably that's not the only place affected.

### Changed 6 years ago by stephen.anderson@…

Alternate patch that should work on everything except SQLite

### comment:3 Changed 6 years ago by anonymous

One problem with olistudent's patch is I don't think EXTRACT(EPOCH...) works on anything but Postgres. I've attached an alternate patch that calculates times the same way that tickets do.

The problem with it is my patch uses a table type of "boolean", which the Trac db adapter for SQLite currently can't handle - SQLite turns booleans into numeric values, and the adapter can't cast them back.

### Changed 6 years ago by stephen.anderson@…

Updated (merged) patch that should work on all DBs, and fixes the error on ticket delete

### comment:4 Changed 6 years ago by stephen.anderson@…

I've merged olistudent's changes together with mine and uploaded it as a new patch, which should:

• Work on all DBs
• Fix the ticket delete problem (which was caused by trying to refer to page.name instead of ticket.id within the Subscriber)

### comment:5 follow-ups:  7  10  14 Changed 5 years ago by Stephan Geulette

Hi, I have all those problems and the proposed patch corrects all. Why doesn't patch the code ? I have tried to commit but it's forbidden...

### comment:7 in reply to:  5 Changed 5 years ago by Ryan J Ollos

Cc: Steffen Hoffmann added; anonymous removed

Hi, I have all those problems and the proposed patch corrects all. Why doesn't patch the code ? I have tried to commit but it's forbidden...

The plugin hasn't been maintained in some time, but now the goal must be to make sure we are fixing it the correct way, and that it works on all supported DBs, rather than pushing a fix which corrects one problem and creates another. For that, you will have to bear with us, while we make sure to get the correct fix in place. We appreciate your patience and feedback when we get a fix in place.

### comment:8 follow-up:  9 Changed 5 years ago by Ryan J Ollos

Resolution: → duplicate new → closed

There are 3 issues discussed in this ticket:

1. The authenticated variable is not properly cast from a bool to an int in model.py. This issue is documented in #10384.
2. There is concern that non-naive timestamps are not correctly cast to bigint. This issue is documented in #7975.
3. Error deleting tickets, which also appears to be a casting problem. This issue is documented in #8577.

Since all of the issues in this ticket have been captured at finer granularity in other tickets, this one is being closed as a duplicate, and concerned parties are asked to follow the cited tickets.

### comment:9 in reply to:  8 Changed 5 years ago by Steffen Hoffmann

Since all of the issues in this ticket have been captured at finer granularity in other tickets, this one is being closed as a duplicate, and concerned parties are asked to follow the cited tickets.

Thanks Ryan for starting ticket triage and linking related issues for ease of tracking issues down to the root cause and resolving it.

Ok, but should anyone still want to re-open, please change ticket owner to myself, or expect no response for quite a long time.

### comment:10 in reply to:  5 Changed 5 years ago by Steffen Hoffmann

Hi, I have all those problems and the proposed patch corrects all. Why doesn't patch the code ? I have tried to commit but it's forbidden...

Almost there. I really appreciate the work done in this ticket. The fix is quite complete, but i.e. it doesn't care for existing wrong values in the Trac db table, right? So this fix has to go in concert with a clean-up script to set a known-good state. I'll have it ready in a few days as part of a major db code rework. Stay tuned.

### comment:11 Changed 5 years ago by Steffen Hoffmann

(In [12302]) TracAnnouncer: Part 7 of 7 - Finally: Go from present to future, refs #5774, #7975, #8065, #9742 and #10384.

Now we've bridged the gap and provide an upgrade path for each historic schema revision of this plugin, while data migration is incomplete yet, especially regarding subscription attributes stored in session_attribute (before v3). Due to component name changes the conversion will be rather complicated, and therefore corresponding research and development is postponed to a future date and will largely depend on explicit demand as well.

(Due to erroneously writing #8056 this didn't get here.)

### comment:12 Changed 5 years ago by Steffen Hoffmann

(In [12303]) TracAnnouncer: Convert type to match db table definitions, refs #7975, #8065 and #10384.

These changes are based on work by olistudent, Stephen Anderson and rea. I made sure, that we respect PEP8 as well, at least as far as Trac core does.

Thanks to all of you for testing, reports and suggestions towards a portable fix, and - ultimately - patience to get it finally resolved.

### comment:13 Changed 5 years ago by Steffen Hoffmann

(In [12304]) TracAnnouncer: Correctly convert id for ticket resources, refs #8065 and #9154.

### comment:14 in reply to:  5 Changed 5 years ago by Steffen Hoffmann

Hi, I have all those problems and the proposed patch corrects all. Why doesn't patch the code ? I have tried to commit but it's forbidden...

Hope, that all concerns are addressed even more portable now. If not, please follow-up on #10384 as hinted above.

### Modify Ticket

Change Properties