[
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...
[[email protected]] 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...
[[email protected]] 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)
