[ https://issues.apache.org/jira/browse/OFBIZ-2353?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14115067#comment-14115067 ]
Jacques Le Roux edited comment on OFBIZ-2353 at 8/29/14 9:56 AM: ----------------------------------------------------------------- [~jacopoc] bq. the algorithm used by OFBiz was designed to work with multiple OFBiz instances connected to the same database and I doubt that the reported issue was caused by it; it was instead probably caused y the fact that the SequenceUtil class has some incorrect code that makes it not thread-safe. I'm sorry to say that I don't agree with you. It's not about being thread-safe, which, if I understand you well, is only local to a machine. It's about preventing simultaneous accesses from different machines to the same tuple in a cluster shared SequenceValueItem DB table. But maybe I did not understood you well and you have another idea to prevent the end of the day SELECT of a tuple in the SequenceValueItem table? bq. why two SELECT FOR UPDATE statements? I just added the FOR UPDATEs, the 2 SELECTs where already there. I did not change anything else as http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/SequenceUtil.java?r1=1144537&r2=1144536&pathrev=1144537 shows. I always try to be as less intrusive as possible when changing core code like that. bq. it avoids the collisions by acquiring a lock That's true, simple. Also a way to decrease collisions, hence better performance, is to increase the bank size... Both can be used, though in our project the default was OK. bq. however the code is redundant It would be if this change was not needed. You can see it as a workaround if you want and if you are able to better handle it I will not be against, but that need to be proof tested. About the locking part, I found interesting that [Postgres is locking rows when updating (UPDATE... WHERE... as Adrian suggested) but that can result in deadlocks|https://stackoverflow.com/questions/10245560/deadlocks-in-postgresql-when-running-update] and I guess other DBMS have the same kind of strategy. Just curious, you say "The issue is still there" do you encounter issues in a clustered environment using the clustered=true parameter or do you just want to clean things? BTW I guess you know you don't need to explicity set the clustered=true property in general.properties if you use the DCC mechanism, which is anyway almost always needed in a clustered environment. About OFBIZ-3557, one thing we could do to prevent the gaps would be to create SECAs on global-rollback event on the createInvoice service. It's just an idea offhand, I will consider it and put feedback in OFBIZ-3557... [~adri...@hlmksw.com] you said bq. SELECT FOR UPDATE will lock records. I think it would be better to use UPDATE... WHERE... I hastily answered bq. UPDATE... WHERE... will lock rows instead of tables indeed, at least with postgres. This is wrong. If you look carefully at the code it's: {code} sql = "SELECT " + SequenceUtil.this.idColName + " FROM " + SequenceUtil.this.tableName + " WHERE " + SequenceUtil.this.nameColName + "='" + this.seqName + "'" + " FOR UPDATE"; {code} So, according to http://www.postgresql.org/docs/9.3/static/explicit-locking.html#LOCKING-ROWS (I guess other DBMS have a similar mechanism) we don't lock the whole table, only the relevant "row" (tuple actually) qualified by this.seqName. I can't see how you can increase performance w/o preventing collisions in a clustered environment otherwise. Also as mentionned above UPDATE... WHERE... is prone to deadlocks. was (Author: jacques.le.roux): [~jacopoc] bq. the algorithm used by OFBiz was designed to work with multiple OFBiz instances connected to the same database and I doubt that the reported issue was caused by it; it was instead probably caused y the fact that the SequenceUtil class has some incorrect code that makes it not thread-safe. I'm sorry to say that I don't agree with you. It's not about being thread-safe, which, if I understand you well, is only local to a machine. It's about preventing simultaneous accesses from different machines to the same tuple in a cluster shared SequenceValueItem DB table. But maybe I did not understood you well and you have another idea to prevent the end of the day SELECT of a tuple in the SequenceValueItem table? bq. why two SELECT FOR UPDATE statements? I just added the FOR UPDATEs, the 2 SELECTs where already there. I did not change anything else as http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/SequenceUtil.java?r1=1144537&r2=1144536&pathrev=1144537 shows. I always try to be as less intrusive as possible when changing core code like that. bq. it avoids the collisions by acquiring a lock That's true, simple. Also a way to decrease collisions, hence better performance, is to increase the bank size... Both can be used, though in our project the default was OK. bq. however the code is redundant It would be if this change was not needed. You can see it as a workaround if you want and if you are able to better handle it I will not be against, but that need to be proof tested. About the locking part, I found interesting that [Postgres is locking rows when updating (UPDATE... WHERE... as Adrian suggested) but that can result in deadlocks|https://stackoverflow.com/questions/10245560/deadlocks-in-postgresql-when-running-update] and I guess other DBMS have the same kind of strategy. Just curious, you say "The issue is still there" do you encounter issues in a clustered environment using the clustered=true parameter or do you just want to clean things? BTW I guess you know you don't need to explicity set the clustered=true property in general.properties if you use the DCC mechanism, which is anyway almost always needed in a clustered environment. About OFBIZ-3557, one thing we could do to prevent the gaps would be to create SECAs on global-rollback event on the createInvoice service. It's just an idea offhand, I will consider it and put feedback in OFBIZ-3557... [~adri...@hlmksw.com] you said bq. SELECT FOR UPDATE will lock records. I think it would be better to use UPDATE... WHERE... I hastily answered bq. UPDATE... WHERE... will lock rows instead of tables indeed, at least with postgres. This is wrong. If you look carefully at the code it's: {code} sql = "SELECT " + SequenceUtil.this.idColName + " FROM " + SequenceUtil.this.tableName + " WHERE " + SequenceUtil.this.nameColName + "='" + this.seqName + "'" + " FOR UPDATE"; {code} So, according to http://www.postgresql.org/docs/9.3/static/explicit-locking.html#LOCKING-ROWS (I guess other DBMS have a similar mechanism) we don't lock the whole table, only the relevant "row" (tuple actually) qualified by this.seqName. I can't see how you can increase performance w/o preventing collisions in a clustered environment otherwise. Also as mentionned above UPDATE... WHERE... is prone to deadlocks. > SequenceUtil may generate duplicate IDs in Load Balancing mode > --------------------------------------------------------------- > > Key: OFBIZ-2353 > URL: https://issues.apache.org/jira/browse/OFBIZ-2353 > Project: OFBiz > Issue Type: Bug > Components: framework > Affects Versions: Release Branch 4.0, Release Branch 09.04, Trunk > Reporter: Philippe Mouawad > Assignee: Jacopo Cappellato > Priority: Critical > Fix For: Release Branch 10.04, Release Branch 11.04, Trunk > > Attachments: OFBIZ-2353 SELECT FOR UPDATE solution.patch, OFBIZ-2353 > SELECT FOR UPDATE solution.patch > > > If Ofbiz is deploy on 2 servers in Load Balancing Mode > SequenceUtil will generate duplicate IDs because synchronization is done at > JVM level instead of doing it in DB. > A good replacement implementation would be: > org.hibernate.id.enhanced.TableGenerator > But it would involve a dependency on Hibernate > Philippe > www.ubik-ingenierie.com -- This message was sent by Atlassian JIRA (v6.2#6252)