> On Fri, 2010-05-21 at 06:50 -0700, Adrian Crum wrote:
> No, atomic transactions are not being used in this case. The
> invoice/order ID generation code is in a simple method, and simple
> methods default to having their own transaction. So, the ID can be
> generated successfully, but the invoice/order creation could fail.
>
Then the problem is that the actual data write is occurring outside the
simple method. There are two options I can see. Sorry for the
pseudocode, but you'll get the idea.
1) refactor the code so that the simple method encloses the document write:
simplemethod writeCustomDocument (customDocumentObject) {
oldID = getMaxOldID();
DocumentObject.ID = generateNewID(oldID);
writeDocument(DocumentObject);
}
In this case, both getMaxOldID and GenerateNewID probably need to be
hooks to customizable code (or you just replace the whole simple
method, using the original as a template), since a custom ID generator
sequence may not be able to rely on generic data comparison tests to
determine the 'highest' ID already in the table.
2) You could write a largely empty record (only the newID is required),
and save the real data content later. Once the ID is saved, there's no
risk of collision, but then you need to wonder what you do if the write
is abandoned before the real data ever gets written. You may end up
with a bunch of mostly-empty records. For the reasons outlined
previously, I think option 1) is better.
Nicholas Malin also raised some good points about multitenancy-- that
also complicates matters, but that probably can be encapsulated within
the custom code, I think, or a call to a generic method or SQL fragment
(think WHERE clause) that filters out documents from other tenants.
--
Matt Warnock <[email protected]>
RidgeCrest Herbals, Inc.
> --- On Fri, 5/21/10, Matt Warnock <[email protected]> wrote:
> > Adrian:
> >
> > I had been following it (with some interest) from the
> > start, but went
> > back and read it again. Also reviewed the JIRA issue,
> > which I had not
> > previously. I have also reviewed the patch you
> > submitted, though I am
> > not altogether certain what "special" invoice order
> > sequencing means in
> > this context, and in any event, I am no XML or ofbiz expert
> > (yet). But
> > is seems that IF "special" sequencing is in effect, then
> > this race
> > condition occurs.
>
> The special sequencing can be found in the simple methods that are changed in
> the patch.
>
> > Now, MOST businesses will require that every invoice number
> > is unique,
> > and MANY will further require that all order/invoice/check
> > numbers be
> > accounted for, even on prenumbered forms (no "gaps") for
> > auditing
> > purposes. Is that "special" to ofbiz?
> >
> > Alternatively, some businesses I have seen enforce a
> > particular format
> > on the invoice/order/document number (like OOOYYYYMMDDSSS,
> > where OOO is
> > the office number, followed by a date, and SSS is a
> > sequence number
> > within the day), and maybe that is where the trouble
> > lies. In the US,
> > Social Security numbers are generated on a "special"
> > pattern like this.
> >
> > In either event, I remain convinced, and on reviewing the
> > thread, David
> > Jones seems to agree, that this problem cannot be easily
> > solved except
> > with atomic SQL transactions. Is there some reason
> > they are not being
> > used in this situation? Or are they in use, and is
> > the OP using a
> > REALLY broken JDBC? Or are they in use, but a record write
> > has
> > inadvertently snuck outside the transaction?
> No, atomic transactions are not being used in this case. The
> invoice/order ID generation code is in a simple method, and simple
> methods default to having their own transaction. So, the ID can be
> generated successfully, but the invoice/order creation could fail.
>
> > At our company, we have two people putting orders in all
> > morning (and we
> > are a very small company), and the chance of a collision in
> > your
> > scenario is NOT rare, but quite high indeed, as the OP has
> > already
> > pointed out, even on a single server with multiple
> > users. But we would
> > only find this out after rollout, not during single-user
> > testing, and it
> > would be an apparently random system lockup, which would be
> > frustrating
> > to say the least. Kudos to the OP for spotting the
> > real nature of the
> > problem-- race conditions are often hard to identify.
> > :-)
> >
> > The issue is not that the sequence generator is "slow" to
> > store the new
> > invoice number, but rather that "last ID" read, the "new
> > ID" generator,
> > and the "new ID" write are not ALL encased inside a
> > transaction that
> > will ensure that the database is ALWAYS internally
> > consistent. The
> > "race" occurs because there is no enclosing transaction,
> > and the "race"
> > will never be completely controlled by being "fast"
> > enough.
> >
> > If there is a long lag between read and write (as where a
> > user is
> > entering order line items) then you either have to 1) do
> > the
> > read/increment/write at the END of the data entry process,
> > or 2) face
> > the high likelihood that two entry processes will collide,
> > and one of
> > them will have to roll back and then be renumbered.
> >
> > Most software I have seen is not very bright about this,
> > and it reads
> > and generates the new order number first, displaying it on
> > the input
> > screen as data is entered. That is not the
> > SQL-friendly way, and asks
> > for trouble. Users like it, because they often need
> > to write that
> > number down on the data entry form-- they just need
> > to be trained to
> > get that order ID as the last thing they do. :( I'm
> > strongly hoping
> > ofbiz will conform to better practices here, as it has
> > elsewhere.
> >
> > Whether you use a SQL standard "sequence" or not (or even
> > the mysql
> > "autoincrement" abomination), and regardless of the primary
> > key status
> > of the orderID (any SQL "unique" declaration could have the
> > same effect,
> > and a well designed order table should at LEAST have a
> > unique orderID,
> > whether it is part of the primary key or not), SQL
> > transactions should
> > resolve this issue easily and scalably.
> >
> > On the other hand, using locking or semaphores in the
> > application code
> > is reinventing the SQL wheel and fraught with possible
> > error. As David
> > suggested, there may be multiple front ends talking to a
> > single
> > database, so a SQL solution is best. I am merely
> > respectfully
> > suggesting that it would be better to let SQL do what it
> > does best, and
> > atomic transactions are the essence of good SQL.
> >
> > Hope this helps explain where I am coming from.
> >
> > --
> > Matt Warnock <[email protected]>
> > RidgeCrest Herbals, Inc.
> >
> > On Thu, 2010-05-20 at 22:26 -0700, Adrian Crum wrote:
> > > Matt,
> > >
> > > If you follow the thread from the start and visit the
> > Jira issue, you will see that we are discussing a very
> > specific problem - not transactions or sequences in
> > general.
> > >
> > > To summarize: There is code in OFBiz that generates
> > sequential ID numbers for orders and invoices. That code
> > does not follow the established pattern for generating
> > sequences, and as a result it will produce non-sequential
> > IDs. A Jira issue has been created and we are working on the
> > problem.
> > >
> > > -Adrian
> > >
> > > --- On Thu, 5/20/10, Matt Warnock <[email protected]>
> > wrote:
> > >
> > > > From: Matt Warnock <[email protected]>
> > > > Subject: Re: QuoteId and OrderId sequence,
> > possible race condition?
> > > > To: [email protected]
> > > > Date: Thursday, May 20, 2010, 5:15 PM
> > > > Isn't this exactly the kind of thing
> > > > that SQL transactions, sequences,
> > > > and foreign key constraints are designed to
> > solve?
> > > >
> > > > If the thread is using transactions properly, the
> > whole
> > > > transaction
> > > > should either be rolled back, or completed.
> > Either
> > > > way, the database
> > > > SHOULD be in a wholly consistent state. But
> > it
> > > > obviously isn't.
> > > >
> > > > So either we are doing something outside a
> > transaction,
> > > > that really
> > > > should be done INSIDE a transaction, or the
> > database has a
> > > > seriously
> > > > flawed implementation of transactions.
> > > > --
> > > > Matt Warnock <[email protected]>
> > > > RidgeCrest Herbals, Inc.
> > > >
> > > > On Thu, 2010-05-20 at 12:43 -0700, Adrian Crum
> > wrote:
> > > > > I doubt there will be any need for
> > additional
> > > > synchronization.
> > > > >
> > > > > I think I know what's going on in your
> > system. My
> > > > previous scenario
> > > > > wasn't correct because I thought the ID was
> > a part of
> > > > the primary key
> > > > > and there was an exception thrown while
> > storing the
> > > > new value - due to a
> > > > > duplicate key violation. Those IDs are not a
> > part of
> > > > the primary key, so
> > > > > that isn't the cause.
> > > > >
> > > > > Instead, I believe the ID is updated
> > successfully, but
> > > > a thread is slow
> > > > > in storing the new ID value - which ends up
> > > > decrementing the ID in the
> > > > > database instead of incrementing it. Then
> > the invoice
> > > > generation fails
> > > > > due to a duplicate key violation. From that
> > point on,
> > > > every attempt to
> > > > > increment the ID value and create a new
> > invoice with
> > > > it will fail
> > > > > because the new ID value already exists in
> > the invoice
> > > > file.
> > > > >
> > > > > -Adrian
> > > > >
> > > > > On 5/20/2010 11:57 AM, James McGill wrote:
> > > > > > On Thu, May 20, 2010 at 10:46 AM,
> > Adrian
> > > > Crum<[email protected]>
> >
> > > > wrote:
> > > > > >
> > > > > >> James,
> > > > > >>
> > > > > >> I uploaded a patch that might fix
> > your
> > > > problem:
> > > > > >>
> > > > > >>
> > > > > > Thanks Adrian. Your approach adds
> > JVM-based
> > > > synchronization to minilang
> > > > > > simple methods.
> > > > > >
> > > > > > I still wonder if this means the create
> > services
> > > > need to be synchronized and
> > > > > > not just the id generators.
> > > > > >
> > > > > > I'll try it out and keep you posted.
> > > > > >
> > > > > >
> > > > > > James
> > > > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
>
>
>
