Modify

Opened 4 years ago

Closed 23 months ago

Last modified 22 months ago

#8065 closed defect (duplicate)

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

Reported by: dbely Owned by: doki_pen
Priority: normal Component: AnnouncerPlugin
Severity: critical Keywords: patch database API
Cc: hasienda Trac Release: 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'),
            Column('adverb')
        ],
        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.

Attachments (3)

fix_postgres_8_4_issues.patch (4.7 KB) - added by olistudent 4 years ago.
fix_postgres_8_4_alternate_nosqlite.patch (3.3 KB) - added by stephen.anderson@… 3 years ago.
Alternate patch that should work on everything except SQLite
fix_postgres_8_4_alternate_nosqlite.2.patch (6.0 KB) - added by stephen.anderson@… 3 years ago.
Updated (merged) patch that should work on all DBs, and fixes the error on ticket delete

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 4 years ago by olistudent

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.

Changed 4 years ago by olistudent

comment:2 in reply to: ↑ 1 Changed 4 years ago by dbely

Replying to olistudent:

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 3 years ago by stephen.anderson@…

Alternate patch that should work on everything except SQLite

comment:3 Changed 3 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 3 years ago by stephen.anderson@…

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

comment:4 Changed 3 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: Changed 2 years ago by sgeulette

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:6 Changed 23 months ago by anonymous

See also #10384

comment:7 in reply to: ↑ 5 Changed 23 months ago by rjollos

  • Cc hasienda added

Replying to sgeulette:

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: Changed 23 months ago by rjollos

  • Resolution set to duplicate
  • Status changed from new to 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 23 months ago by hasienda

  • Keywords patch database API added

Replying to rjollos:
...

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 22 months ago by hasienda

Replying to sgeulette:

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 22 months ago by hasienda

(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 22 months ago by hasienda

(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 22 months ago by hasienda

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

comment:14 in reply to: ↑ 5 Changed 22 months ago by hasienda

Replying to sgeulette:

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.

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.