Bug 8252 - Non-standard sequence naming leads to PostgreSQL error
Non-standard sequence naming leads to PostgreSQL error
Status: RESOLVED FIXED
Product: OJS
Classification: Unclassified
Component: Installer
2.4.3
All All
: P3 normal
Assigned To: PKP Support
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-10 07:03 PDT by Tom Christensen
Modified: 2013-06-25 17:06 PDT (History)
1 user (show)

See Also:
Version Reported In:
Also Affects:


Attachments
Patch against OJS 2.4.2 (4.52 KB, patch)
2013-06-19 05:15 PDT, Tom Christensen
Details | Diff
Avoid renames v2 for OJS 2.4.2 (6.27 KB, patch)
2013-06-20 03:58 PDT, Tom Christensen
Details | Diff
Avoid dangling sequence for OJ 2.4.2 (1.25 KB, patch)
2013-06-20 04:00 PDT, Tom Christensen
Details | Diff
Patch against OJS 2.4.x lib/pkp (1.51 KB, patch)
2013-06-20 17:38 PDT, Alec Smecher
Details | Diff
Patch against OJS 2.4.2 (20.07 KB, patch)
2013-06-25 17:01 PDT, Alec Smecher
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Christensen 2013-06-10 07:03:15 PDT
In one of our test instances upgraded from 2.3.7 to 2.4.2 I ran into this error when trying to add an article to a journal:
DB Error: ERROR: duplicate key value violates unique constraint "articles_pkey1"

In a different test instance also upgraded from 2.3.7 to 2.4.2 but with an older snapshot of production data, I could not reproduce this instead I get a different error:
DB Error: ERROR: invalid input syntax for integer: ""

Inspecting the debug output reveals a previous related error:
(postgres7): SELECT article_id FROM articles WHERE oid=0
-1: ERROR: column "oid" does not exist at character 39

More digging reveals that PO_InsertID in adodb-postgresql64.inc.php falls back to using oids when it cannot find a sequence.
Looking at the defined sequences it becomes obvious why this happens, somehow articles_article_id_seq has been renamed articles_article_id_seq1 during the upgrade!
Worse is that it is not the only one:
article_galleys_galley_id_seq1
article_supplementary_files_supp_id_seq1
articles_article_id_seq1
issues_issue_id_seq1
notifications_notification_id_seq1

Both upgraded instances have the same list of renamed sequences so it is probably not just a coincidence.
Comment 1 Alec Smecher 2013-06-11 11:44:12 PDT
Tom, hmm, this sounds like it might be an ADODB bug -- ADODB takes care of generating table rename SQL, but it looks like it's leaving the tables in an intermittent state. (This might also be affected by the version of PostgreSQL, as they may have changed their sequence management details too.)

Can you post your version of PostgreSQL, and as much detail as you can scare up to help us duplicate the issue locally?
Comment 2 Tom Christensen 2013-06-19 05:15:35 PDT
I'm using PostgreSQL 8.4 (.13)

The affected tables and sequences are the ones in dbscripts/xml/upgrade/2.4.0_idupgrade_*.
The problem is that they direct ADODB to do a rename on the table which ofcourse does not rename the associated sequence (or pkey index name for that matter).
When the new table is later created by ADODB postgres creates the sequence with a suffix of '1' since a sequence with the preferred name already exists in the old renamed table.

I can see two ways to solve this, either deal with the sideeffects by adding explicit renames or avoid the sideeffects by creating a copy of the table and dropping the original (this is how ADODB normally handles table modifications on postgres).
I would much prefer to avoid the sideeffects as they may be unpredictable so I will attach a patch that does CREATE TABLE AS/DROP TABLE instead of rename.
I think this syntax should work for MySQL aswell but I cannot test this.

I tested this on a copy of our current OJS 2.3.7 production database and the upgrade completed without issue and the sequences now have the expected names.
Comment 3 Tom Christensen 2013-06-19 05:15:58 PDT
Created attachment 3936 [details]
Patch against OJS 2.4.2
Comment 4 Alec Smecher 2013-06-19 16:30:58 PDT
Hi Tom -- Yuck. (That's referring to this bug, not your input on it, which is helpful as usual.) 

Mining through some ancient history, PostgreSQL's ADODB support used to presume that OIDs were being used throughout. That's a bad idea.

There were two fixes introduced to this: one proposed on the ADODB forums (which made it eventually into ADODB), described here:
http://phplens.com/lens/lensforum/msgs.php?phplens_forummsg=new&id=16767&PHPSESSID=cd371eb5ddbba18f47d9d7d7b661ced2

That fix hard-codes the sequence name. We never used it because it's also a function specific to the PostgreSQL driver and doesn't override ADODB's "get last insert ID" functions.

Instead, one of our devs coded an OJS-specific fix almost a decade ago:
https://github.com/pkp/pkp-lib/blob/ojs-stable-2_4/lib/adodb/diff/adodb-postgres64.inc.php.diff

This *also* hard-codes the sequence name.

So we have the same bug appearing both in ADODB and in our modification to it.

The datadict-postgres.inc.php class, rather than hard-coding a guess that may or may not be correct, detects the sequence name properly. OTOH the "proper" thing to do would be to let the sequence be whatever arbitrary name PostgreSQL wants to make it, and detect that name and use it in the implementation of PO_Insert_ID (our bug) and pg_insert_id (ADODB's), just like the data dictionary implementation does.

Let me dust off my PostgreSQL installations and see what I can do.
Comment 5 Alec Smecher 2013-06-19 16:57:40 PDT
Tom, the one PostgreSQL database dump that I have is not showing this behavior on upgrade -- which is strange, because it includes RENAME TABLE statements during upgrade. If it's possible, could you send me a database dump to remove that variable from the equation? (Using PostgreSQL 8.3) Feel free to anonymize it as you see fit.
Comment 6 Tom Christensen 2013-06-20 02:45:57 PDT
> Tom, the one PostgreSQL database dump that I have is not showing this
> behavior on upgrade -- which is strange, because it includes RENAME TABLE
> statements during upgrade. If it's possible, could you send me a database
> dump to remove that variable from the equation? (Using PostgreSQL 8.3) Feel
> free to anonymize it as you see fit.

Unfortunately I cannot give you a copy of our database.

However this small example trivially shows the issue:
ojs2=# CREATE TABLE t1 (
id SERIAL,
comments TEXT,
PRIMARY KEY (id)
);
NOTICE:  CREATE TABLE will create implicit sequence "t1_id_seq" for serial column "t1.id"
NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "t1_pkey" for table "t1"
CREATE TABLE
ojs2=# \di
              List of relations
 Schema |  Name   | Type  |  Owner   | Table 
--------+---------+-------+----------+-------
 public | t1_pkey | index | postgres | t1
(1 row)

ojs2=# \ds
            List of relations
 Schema |   Name    |   Type   |  Owner   
--------+-----------+----------+----------
 public | t1_id_seq | sequence | postgres
(1 row)

ojs2=# \d t1
                          Table "public.t1"
  Column  |  Type   |                    Modifiers                    
----------+---------+-------------------------------------------------
 id       | integer | not null default nextval('t1_id_seq'::regclass)
 comments | text    | 
Indexes:
    "t1_pkey" PRIMARY KEY, btree (id)


ojs2=# ALTER TABLE t1 RENAME TO t1_old;
ALTER TABLE
ojs2=# CREATE TABLE t1 (
id SERIAL,
comments TEXT,
PRIMARY KEY (id)
);
NOTICE:  CREATE TABLE will create implicit sequence "t1_id_seq1" for serial column "t1.id"
NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "t1_pkey1" for table "t1"
CREATE TABLE
ojs2=# \ds
             List of relations
 Schema |    Name    |   Type   |  Owner   
--------+------------+----------+----------
 public | t1_id_seq  | sequence | postgres
 public | t1_id_seq1 | sequence | postgres
(2 rows)

ojs2=# \di
               List of relations
 Schema |   Name   | Type  |  Owner   | Table  
--------+----------+-------+----------+--------
 public | t1_pkey  | index | postgres | t1_old
 public | t1_pkey1 | index | postgres | t1
(2 rows)

ojs2=# \d t1
                           Table "public.t1"
  Column  |  Type   |                    Modifiers                     
----------+---------+--------------------------------------------------
 id       | integer | not null default nextval('t1_id_seq1'::regclass)
 comments | text    | 
Indexes:
    "t1_pkey1" PRIMARY KEY, btree (id)

ojs2=# 

Using CREATE TABLE AS .. SELECT avoids the issue entirely:
ojs2=# drop table t1;
DROP TABLE
ojs2=# ALTER TABLE t1_old RENAME TO t1;
ALTER TABLE
ojs2=# CREATE TABLE t1_old AS SELECT * FROM t1;
SELECT
ojs2=# \d t1_old
     Table "public.t1_old"
  Column  |  Type   | Modifiers 
----------+---------+-----------
 id       | integer | 
 comments | text    | 

ojs2#
No sequences and no primary key indexes to get in they way when recreating the table.

Note that it's not really so much about the rename itself it's the creating a new table which happens to have a SERIAL field generating the same implicit name as an existing sequence.
This is why only the explicit table rename combined with recreating the table using the old SERIAL field name causes the problem.
Comment 7 Tom Christensen 2013-06-20 03:57:06 PDT
(In reply to comment #2)
> The affected tables and sequences are the ones in
> dbscripts/xml/upgrade/2.4.0_idupgrade_*.

Close inspection revealed another instance in 2.4.0_notifications.xml.

What's more interesting is that in this script there is an example of what I've proposed as a way to deal with postgres, doing SELECT INTO (CREATE TABLE AS .. SELECT)/DROP TABLE instead of a rename. Unfortunately it was only done for notification_settings and not notifications.
I will attach an updated patch that fixes this issue aswell as converts 2.4.0_idupgrade_*.xml to use the same style.

I also found a dangling sequence after the upgrade: review_rounds_review_round_id_seq_tmp
This looks like an attempt in 2.4.0_preupdate_reviewrounds.xml at dealing with this issue by preemptively calling the sequence something else before ADODB modifies the table later.
At some point this might have been relevant but in the current code it does not seem to serve any purpose.
I reduced this to just creating the primary key as a SERIAL and the upgrade completed correctly with no dangling sequence.
Diffing debug output before and after this change showed no other differences.
Will attach a patch to fix this aswell.
Comment 8 Tom Christensen 2013-06-20 03:58:53 PDT
Created attachment 3937 [details]
Avoid renames v2 for OJS 2.4.2
Comment 9 Tom Christensen 2013-06-20 04:00:35 PDT
Created attachment 3938 [details]
Avoid dangling sequence for OJ 2.4.2

Avoid dangling sequence after the review_rounds update when using PostgreSQL.
Comment 10 Alec Smecher 2013-06-20 17:38:11 PDT
Created attachment 3940 [details]
Patch against OJS 2.4.x lib/pkp

I was able to duplicate the problem by upgrading specifically from 2.3.7 -- crossing from an earlier version appeared to cause the table to be re-created by ADODB after the rename operations for some reason, which reinstated the original sequence names.

The master branch makes heavier use of the <rename ...> element, which really ought to work as advertised, and I'd rather not code this separately for each DB if I can avoid it. The root cause of the bug is an incorrect assumption that ADODB can guess the sequence name correctly. I'll check out the dangling sequence and clean that up -- but meanwhile, can you try the following modification to ADODB that removes the assumption about sequence names? It matches the implementation already written in the data dictionary code. Apply it in lib/pkp with -p1. It works on my installation.

If this checks out, I'll contribute it back to ADODB.

Note that there's a handy pg_get_serial_sequence available for some versions of PostgreSQL, but it doesn't appear to go back to our oldest supported version of 7.1.
Comment 11 Tom Christensen 2013-06-21 01:41:44 PDT
(In reply to comment #10)
> can you try the
> following modification to ADODB that removes the assumption about sequence
> names?
> 
It seems to work.

I removed my local workarounds and after the upgrade I see sequence names with a '1' suffix.
Starting a submission process now seems to work where it would bomb out before.

However I noted that the submission was given an id of '1' despite there being several thousand articles already.
This happens because in the 2.4.0_idupgrade_*2.xml scripts a sequence is being explicitly named in a SETVAL, however due to sideeffects of the rename, it's the wrong sequence being updated.
It seems you will need pg_get_serial_sequence or something equivalent to write a query that can handle this.

> Note that there's a handy pg_get_serial_sequence available for some versions
> of PostgreSQL, but it doesn't appear to go back to our oldest supported
> version of 7.1.

I would recommend dropping support for PostgreSQL 7.x altogether, nobody should really be using those very old versions anymore.
If you really want to continue support of 7.x I found a pg_get_serial_sequence implementation here:
http://pgfoundry.org/tracker/index.php?func=detail&aid=1000463&group_id=1000151&atid=616
It is untested since I do not have that old PostgreSQL available.
Comment 12 Alec Smecher 2013-06-21 13:41:02 PDT
Correct ADODB 'get last insert index' function
https://github.com/pkp/pkp-lib/commit/23b1e1ffe48d2fcbeb47504df10af5d6743f7b2a
Comment 13 Alec Smecher 2013-06-21 13:44:01 PDT
Correct ADODB 'get last insert index' function (master branch)
https://github.com/pkp/pkp-lib/commit/0544a83c22da245f6fd8fa11e0eb9a44e6dbeeb2
Comment 14 Alec Smecher 2013-06-21 13:45:02 PDT
Correct OJS sequence use; update PostgreSQL requirement
https://github.com/pkp/ojs/commit/0b1fb920eba39e7ca6cbf5323570e99a1b44ec95
Comment 15 Alec Smecher 2013-06-21 13:51:02 PDT
Correct OJS sequence use; update PostgreSQL requirement (master branch)
https://github.com/pkp/ojs/commit/71d64aa5fb761edaea188f613932101ee648cd56
Comment 16 Alec Smecher 2013-06-21 13:52:11 PDT
Tom, the following two patches should wrap this all up:

https://github.com/pkp/pkp-lib/commit/23b1e1ffe48d2fcbeb47504df10af5d6743f7b2a.diff (apply from lib/pkp)
https://github.com/pkp/ojs/commit/0b1fb920eba39e7ca6cbf5323570e99a1b44ec95.diff (apply in OJS directory)

Todo: contribute back to ADODB; clean up diffs in ADODB directory; add to recommended patches list.
Comment 17 Alec Smecher 2013-06-21 14:07:25 PDT
Posted to the ADODB forums for consideration at http://phplens.com/lens/lensforum/msgs.php?id=19403
Comment 18 Tom Christensen 2013-06-25 02:33:11 PDT
(In reply to comment #16)
> Tom, the following two patches should wrap this all up:
> 
> https://github.com/pkp/pkp-lib/commit/
> 23b1e1ffe48d2fcbeb47504df10af5d6743f7b2a.diff (apply from lib/pkp)
> https://github.com/pkp/ojs/commit/0b1fb920eba39e7ca6cbf5323570e99a1b44ec95.
> diff (apply in OJS directory)
> 

Confirmed.

With those two patches on top of the currently recommended patches the upgrade completes and the sequences are correctly initialized (tested by starting a new article submission and looking at the assigned id).
Comment 19 Alec Smecher 2013-06-25 17:01:44 PDT
Created attachment 3941 [details]
Patch against OJS 2.4.2
Comment 20 Alec Smecher 2013-06-25 17:05:26 PDT
Closing. Thanks for all your help, Tom.
Comment 21 Alec Smecher 2013-06-25 17:06:03 PDT
Remove diffs from ADODB lib directory
https://github.com/pkp/pkp-lib/commit/a1065cff4a0c27cd3ce1011d62f6e9cf8e6a7c9a