Modify

Opened 5 years ago

Closed 3 years ago

#5667 closed defect (fixed)

[Patch] Added compatibility with PostgreSQL

Reported by: s.noack@… Owned by: hasienda
Priority: normal Component: TracFormsPlugin
Severity: normal Keywords: PostgreSQL compatibility
Cc: s.noack@… Trac Release: 0.11

Description

Based on #3290, I have modified formdb.py to work with PostgreSQL. The changes done by #3290 basically worked except the creation of tables and indexes. Following changes were required to make it work with PostgreSQL:

- The syntax for auto incrementing fields of PostgreSQL differs from sqlite and mysql.
- Primary keys must be defined in the line of the column, instead of at the end of the CREATE TABLE statement.
- Use DROP INDEX ... instead of ALTER TABLE ... DROP INDEX ...
- Omit DESC keyword when creating indexes.

Maybe you can switch for the database backend when creating the tables and indexes. It would be nice if it would work out of the box with PostgreSQL. TracFormsPlugin is the only of 5 plugins I am using which didn't worked out of the box after migrating to MySQL.

Attachments (7)

formdb.py (11.0 KB) - added by s.noack@… 5 years ago.
20110501_postgres.patch (6.5 KB) - added by hasienda 3 years ago.
patch derived from previous contribution, last db schema changes adapted too
20110503_postgres.patch (8.7 KB) - added by hasienda 3 years ago.
revised compatibility patch
20110503_postgres.2.patch (8.7 KB) - added by hasienda 3 years ago.
unquoted column names for PostgreSQL
20110504_postgres.patch (9.4 KB) - added by hasienda 3 years ago.
compatibility patch updated with second create table and -last_id fix
20110506_debug.patch (787 bytes) - added by hasienda 3 years ago.
enable debug traceback for all calls to fetchone
20110508_sql-rewrite.patch (49.3 KB) - added by hasienda 3 years ago.
major SQL rewrite following best practice for Trac < 0.12, see Trac db API docs

Download all attachments as: .zip

Change History (32)

Changed 5 years ago by s.noack@…

comment:1 Changed 4 years ago by rjollos

  • Summary changed from Added compatibly with PostgreSQL to [Patch] Added compatibly with PostgreSQL

comment:2 Changed 4 years ago by rjollos

See also #5608.

comment:3 Changed 4 years ago by hasienda

  • Keywords PostgreSQL compatibility added
  • Owner changed from rharkins to hasienda
  • Status changed from new to assigned

Thanks for the ground work.

Patch looks good so far (not tested by now), so you made fixing that issue really easy as far as I can see. So this should be resolved soon.

comment:4 Changed 3 years ago by hasienda

Uh, this is not a patch but a whole new file.

I may find some time to diff it against a current version and add similar syntax to the newer upgrades as well. Still in general a true diff (diff -u upstream_file modified_file) plus mentioning the code revision used as base for the modifications would be appreciated.

comment:5 follow-up: Changed 3 years ago by s.noack@…

I don't remember why I don't have created a patch. However as far as i remeber it was based on TracForms 0.2.

comment:6 in reply to: ↑ 5 Changed 3 years ago by hasienda

Replying to s.noack@placement24.com:

I don't remember why I don't have created a patch. However as far as i remeber it was based on TracForms 0.2.

Ok, thanks for clarification.

I'm very busy integrating new features these days, so you could largely reduce the time between now and PostgreSQL of the current db schema by updating your patch to trunk. Interested? I'm ensure, that I'm willing to integrate changes that look reasonable and come pre-tested, only I still can't do testing of alternative db engines on my own due to time constraints.

Changed 3 years ago by hasienda

patch derived from previous contribution, last db schema changes adapted too

comment:7 follow-up: Changed 3 years ago by hasienda

Anyone able and willing to test the updated changes?

Your feedback would significantly speedup that work. Thanks in advance.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by nelsojost

Replying to hasienda:

Anyone able and willing to test the updated changes?

Your feedback would significantly speedup that work. Thanks in advance.

I've been following the development of TracFormsPlugin, and I was happy to know that it is progressing. My system is: Trac 0.12 + PostgreSQL 8.4 + Python 2.6.

I applied the referred patch in 0.3 and 0.4dev releases, and when running trac-admin <...> upgrade I got some sintaxe error messages, which I correct manually in the code. Here is the little diff:

382d381
<                 );
558d556
<                 )
765d762
<                 )

But still I got another error message:

ProgrammingError: type "tracform_id" does not exist
LINE 4:                      tracform_id 'id', updated_on 'time',
                             ^

This SQL appear in the line 630 from formdb.py, and I don't know what changes need to be make.

PS: I have tested before the patch formdb.py sended by s.noack@placement24.com, on tracforms 0.2, and it parcially works: at least, the enviroment upgrade was succeded. The macro in deed generates the form components, but the submit buttons just don't work, i.e., changes was not recorded in database.

Changed 3 years ago by hasienda

revised compatibility patch

comment:9 in reply to: ↑ 8 Changed 3 years ago by hasienda

Nice to hear about interest in the development.

Replying to nelsojost:

![...] I got some sintaxe error messages, which I correct manually in the code. Here is the

Ok, this is easy. Just missed correct matching brackets. I think that the semicolon is not needed in this statements as it appears inconsistently. Please check, if this is clean now.

But still I got another error message: ![...]
This SQL appear in the line 630 from formdb.py, and I don't know what changes need to be make.

I've checked the PostgreSQL docs and this is most probably related:

The AS Key Word

In the SQL standard, the optional key word AS is just noise and can be omitted without affecting the meaning. The PostgreSQL parser requires this key word when renaming output columns because ...

I've added a specific, alternative statement that could work. Only I'm not entirely convinced regarding the quoted and unquoted column names.

PS: I have tested before the patch formdb.py sended by s.noack@placement24.com, on tracforms 0.2, and it parcially works: at least, the enviroment upgrade was succeded. The macro in deed generates the form components, but the submit buttons just don't work, i.e., changes was not recorded in database.

Don't worry. I think we could get it working in no time, if you stay with me for resolving this ticket. 0.2 is history, actually focussing on 0.4, but I deferred 0.3.1 until now to include the PostgreSQL compatibility code there as well.

Because you mentioned submit buttons: Be sure to grant required permission (FORM_EDIT_VAL) as needed to re-enable them for TracForms >= 0.3 .

comment:10 follow-up: Changed 3 years ago by nelsojost

Thanks for the response. I tried the new patch (TracForms-0.4dev_r10152). Here is the log:

	SQL ERROR: syntax error at or near "'id'"
LINE 4:                      tracform_id AS 'id', updated_on AS 'tim...
                                            ^

Although I use PostgreSQL, I don't know much about databases - barely know something about SQL itself. Learn more about this is in my TO-DO list.

I also tried the following:

  1. 'id' AS tracform_id, which fix sintax error, but generates a new one (which is not so surprising):
    ProgrammingError: column "id" does not exist
    LINE 1: SELECT id,time,old_state FROM forms_history
    
  2. 'tracform_id' AS id.
    ERROR: EXECUTING SQL:
    
                CREATE INDEX forms_history_id_idx
                    ON forms_history(id)
                
    	()
    	SQL ERROR: data type unknown has no default 
    operator class for access method "btree"
    HINT:  You must specify an operator class for the 
    index or define a default operator class for the 
    data type.
    

This was me fooling around, little blindly. Hope to be more helpful next time.

Changed 3 years ago by hasienda

unquoted column names for PostgreSQL

comment:11 in reply to: ↑ 10 Changed 3 years ago by hasienda

Replying to nelsojost:

Thanks for the response. I tried the new patch (TracForms-0.4dev_r10152). Here is the log:

Thank you for the quick response. So it was at least a partial fix.

This was me fooling around, little blindly. Hope to be more helpful next time.

Never mind. I've attached another small variation, that could get it done. I'm looking forward to your findings. Good luck.

comment:12 follow-up: Changed 3 years ago by nelsojost

Moving foward! I've tested the new patch. In deed, the right sintax is tracform_id AS id - without single quotes. SQL for table forms_history did run OK, but there was another one for table forms_fields (line 703) - I adapted by creating a postgre option and correcting sintaxe just like in forms_history instructions. Finally, the all the db update did run OK.

Now, testing the plugin in action following the "Shopping List" tutorial (nice documentation, by the way), the form are generated fine - macro successfully processed. Unfortunately, "Got 'em!" button still doesn't work.

Trac[tracdb] ERROR: EXECUTING SQL:
SELECT CURRVAL('id_id_seq')
	()
	SQL ERROR: relation "id_id_seq" does not exist
LINE 1: SELECT CURRVAL('id_id_seq')
                       ^

PS: This time I've make sure to grant FORM_EDIT_VAL permission.

comment:13 in reply to: ↑ 12 Changed 3 years ago by hasienda

Replying to nelsojost:

Moving foward! I've tested the new patch. In deed, the right sintax is tracform_id AS id - without single quotes. SQL for table forms_history did run OK, but there was another one for table forms_fields (line 703) - I adapted by creating a postgre option and correcting sintaxe just like in forms_history instructions. Finally, the all the db update did run OK.

Got it, so this should be fine. Thank you.

Now, testing the plugin in action following the "Shopping List" tutorial (nice documentation, by the way), the form are generated fine - macro successfully processed. Unfortunately, "Got 'em!" button still doesn't work.

Trac[tracdb] ERROR: EXECUTING SQL:
SELECT CURRVAL('id_id_seq')
	()
	SQL ERROR: relation "id_id_seq" does not exist
LINE 1: SELECT CURRVAL('id_id_seq')
                       ^

PS: This time I've make sure to grant FORM_EDIT_VAL permission.

If you didn't grant that permission, it simply wouldn't be there. "Doesn't work" is a functional problem. This time it's about the db cursor implementation (see tracdb.py too).

The error is raised from executing formdb.py line 131-138:

                    form_id = cursor("""
                        INSERT INTO forms
                            (realm, resource_id, subcontext,
                            state, author, time)
                            VALUES (%s, %s, %s, %s, %s, %s)
                        """, realm, resource_id, subcontext,
                        state, author, updated_on) \
                        .last_id('forms', 'id')

and the logic behind the last_id property is in tracdb.py line 103-104:

    def last_id(self, cursor, table, column='id'):
        return self.db.get_last_id(self, table, column)

inherited from the logic in Trac itself (see trac.db.postgres_backend):

    def get_last_id(self, cursor, table, column='id'):
        cursor.execute("SELECT CURRVAL('%s_%s_seq')" % (table, column))
        return cursor.fetchone()[0]

Information explaining what's going on here could be found i.e. at http://www.postgresql.org/docs/8.2/interactive/functions-sequence.html

Especially from the last two definitions you can see that last_id in formdb.py line 138 is missing the cursor attribute. I've tested with SQLite, and it does no harm. This is included in the next patch version for you. I suspect, that we've found a hidden bug due to stricter parser logic of PostrgreSQL. Looks like digging into the new SQL flavor has already payed back.

Summary of late changes:

  • tracformsplugin/trunk/0.11/tracforms/formdb.py

    diff --git a/tracformsplugin/trunk/0.11/tracforms/formdb.py b/tracformsplugin/trunk/0.11/tracforms/formdb.py
    a b  
    135135                            VALUES (%s, %s, %s, %s, %s, %s) 
    136136                        """, realm, resource_id, subcontext, 
    137137                        state, author, updated_on) \ 
    138                         .last_id('forms', 'id') 
     138                        .last_id(cursor, 'forms', 'id') 
    139139                else: 
    140140                    cursor(""" 
    141141                        UPDATE  forms 
     
    706706                     tracform_id 'id', field,  
    707707                     updater 'author', updated_on 'time' 
    708708                FROM tracform_fields 
     709            """, 
     710            postgres = """ 
     711            CREATE TABLE forms_fields 
     712                AS SELECT 
     713                     tracform_id AS id, field, 
     714                     updater AS author, updated_on AS time 
     715                FROM tracform_fields 
    709716            """) 
    710717        cursor(""" 
    711718            DROP INDEX tracform_fields_tracform_id_field 

Since this has been a travel to the outer perimeters of my programming and SQL knowledge, I'm eagerly awaiting your findings. Good luck.

Changed 3 years ago by hasienda

compatibility patch updated with second create table and -last_id fix

comment:14 follow-up: Changed 3 years ago by nelsojost

Hi. I put the missing cursor attribute like in the diff. Something curious happen: the handler /form/update sends the following message (that I coudn't find in debug log):

'DBCursor' object has no attribute 'fetchone'

When trying refresh the page (resending /form/update), I got Conflict message - values already set in db? Going back to the wiki page: the values was recorded in deed. I've try new submits: all works fine!

I also try with new pages: this error really just happen in the first submit. And with tickets, I have to use the macro [[subpage()]] because [[Include()]] doesn't work - also here, the error occours only in the first submit.

comment:15 in reply to: ↑ 14 Changed 3 years ago by hasienda

Replying to nelsojost:

Hi. I put the missing cursor attribute like in the diff. Something curious happen: the handler /form/update sends the following message (that I coudn't find in debug log):

'DBCursor' object has no attribute 'fetchone'

Last thing first: I often encounter such a situation myself, that there's nothing in the logs, if the error is displayed in the browser window. Maybe I'll find some time to improve direction/duplication of output for such events. But this is more of a long-term goal.

Another research is needed for the error regarding the fetchone attribute. I'll look into it and comment later. Thank you again for your testing effort. But it looks encouraging, that we've obviously moved beyond the point, where we encounter fatal errors.

When trying refresh the page (resending /form/update), I got Conflict message - values already set in db? Going back to the wiki page: the values was recorded in deed. I've try new submits: all works fine!

Expected. You can't send the same revision twice. You need to go back to the parent page URL. I could only try to add a redirect-back-to-referrer-on-error logic and show the failure on top of the page. What do you think?

I also try with new pages: this error really just happen in the first submit.

This is a very helpful observation indeed.

And with tickets, I have to use the macro [[subpage()]] because [[Include()]] doesn't work - also here, the error occours only in the first submit.

Really? I did use the IncludeMacro hundreds of times in my tickets with Trac 0.13dev without problems. But regarding TracFormsPlugin this is slightly off-topic here. Better mention this inside a ticket directly assigned to IncludeMacro, please.

Changed 3 years ago by hasienda

enable debug traceback for all calls to fetchone

comment:16 Changed 3 years ago by hasienda

Patch above should enable you to see more inside the log. You may choose to send output attached to an email directly to me.

comment:17 Changed 3 years ago by hasienda

(In [10168]) TracFormsPlugin: Add compatibility SQL statements for PostgreSQL, refs #5608 and #5667.

This has been developed from a formdb.py of tracforms-0.2 modified by

  1. Noack, further testing done by Nelso. Thanks for your contribution.

Despite remarkable progress this is only nearly working. After some
hours of try-n-error I decided to not follow this path any longer.
As a matter of fact the whole db code of this plugin is broken by design.
I've already straightened it a bit, but the custom db cursor implementation
seems evil. Reinventing connection wrapper code by own compatibility was
never smart and is bound to be finally brocken after upcoming switch from
env.get_db_cnx to the new with_transaction, that has much more
intelligent transaction handling in general.

Changed 3 years ago by hasienda

major SQL rewrite following best practice for Trac < 0.12, see Trac db API docs

comment:18 Changed 3 years ago by hasienda

Attached latest changes, that should lead to code for the next major TracForms version:

 formdb.py |  717 +++++++++++++++++++++++---------------------------------------
 model.py  |    6 
 tracdb.py |  212 +++++++-----------
 web_ui.py |    1 
 4 files changed, 356 insertions(+), 580 deletions(-)

As you'll see this is a big code-cleanup resulting in totally different db handling. By design this will release db connections faster i.e. optimized for concurrent db access. And largely based on proved Trac core code we'll be able to support upcoming Trac versions without major headache.

I'd be happy, if you could patch this on-top of current trunk, test and report back your findings.

Hint: I've introduced a second, direct install method that is executed, if no previous TracForms version is detected. So you're no longer jumping through all the loops of TracForms db schema history but will be lifted up to the top in an instant. This should improve first install experience a lot. Give it a try inside of a (fresh) test environment with PostgreSQL db configured as well, please.

comment:19 Changed 3 years ago by hasienda

Side-note: We'll need compatibility code or a dedicated 0.13 branch to cope with the current with_transaction db handling since 0.12 - again slightly modified in 0.13, where the old env.get_db_cnx is already strongly depreciated now. Obviously it will no longer be available in future Trac versions.

comment:20 follow-up: Changed 3 years ago by nelsojost

Sorry about the delay. Today I've tested the newest trunk, with both a "fresh" enviroment, and with the old one (with tables already created). Neither of them did work. I mean, the error 'DBCursor' object has no attribute 'fetchone' still happens only in the first submit.

I put the traceback calls like in the debug patch and did sent the debug log to your email (hoff.st@web.de, as in setup.py). I hope it can be useful.

Nice work with de plugin and the dedication to make it right.

comment:21 in reply to: ↑ 20 Changed 3 years ago by hasienda

Replying to nelsojost:

Sorry about the delay.

Hey, np.

Today I've tested the newest trunk,
[...]

Well, the last patch has not been committed 'til now, so this was most probably not the correct code for the test. Since it's such a big change, I intended to test more before actually committing it. Sorry for the irritation caused that way at your side. See your mail for my direct reply/next step.

comment:22 Changed 3 years ago by hasienda

Just a quick catch-up for public tracking of some private test releases and corresponding testing.

After a request at trac-dev and kindly response by Remy Blank with some valuable hints on the right way to use Trac db cursors I was able to improve the code to a nearly working state by now:

Commenting an email by nelsojost on 05/17/2011 20:27:

I've tested "20110514_tracforms_sql-fixup.tgz" first with the existing db.
All works really well! Not a single error message on a debug log.
I also took a look into database records: all seems ok.

But when testing in a fresh new enviroment, an error occours in upgrade attempt:
IntegrityError: duplicate key value violates unique constraint "system_pkey"

So a final commit is near, hopefully this week considering the current pace of development. The shortcut saved us some more useless iterations here.

More testers for MySQL and PostgreSQL still very welcome. Please contact me, if you're interested in the early (pre-release) beta for TracForms 0.4 - before the next commit. Soon it'll be all public again anyway.

comment:23 Changed 3 years ago by hasienda

  • Summary changed from [Patch] Added compatibly with PostgreSQL to [Patch] Added compatibility with PostgreSQL

comment:24 Changed 3 years ago by hasienda

(In [10211]): TracFormsPlugin: Anwser a long standing question from inside formdb.py, refs #5667.

# Please, please tell me there's a better way to do this...

Yes, it is: Don't try to re-invent the Trac db cursor but just go and use the
native one, and i.e. MySQL/PostgreSQL compatibility comes for free by itself.

SQL rewrite details

  • introduce native Trac db type selector
  • drop DBCursor class with a lot of proprietary, unused special methods
  • remove 'DROP' SQL statements before dropping entire tables anyway
  • separate index creation SQL statements obsoleted and common suffix for Trac db indexes to new TracForms indexes created automagicly using db_connector.to_sql()

Thanks to osimons, who made this finally clear to me and again encoraged me
to do things in a cleaner fashion. Thanks to rblank for valuable hints on
proper use of Trac db cursors. And many thanks to Nelso G. Jost for
patiently testing published code as well as many more private iterations
of code changes in Trac environments using the PostgreSQL db connector.

comment:25 Changed 3 years ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [10216]) TracFormsPlugin: Releasing version 0.4, pushing development to 0.5, closes #5318, #5608, #5667.

Codename 'Nelso'. ;-) This is the first release supporting PostgreSQL for
storing forms data, effectively working equally well with all db backends,
that are currently supported by Trac itself.
Another big step towards a stable plugin. Thanks to all the kind people,
who helped with their contributions in many ways. It was another rewarding
experience to find the kinks and iron them out.
Now everybody enjoy the new Trac power under the hood.

Add Comment

Modify Ticket

Action
as closed .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from hasienda. Next status will be '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.