Re: [Spacewalk-devel] [PATCH] Avoid a possible concurrency issue on RhnSet update
Hello Silvio, I investigated that: we get ConstraintViolationException with Oracle and WrappedSQLException in Postgres. That depends on SqlExceptionTranslator.sqlException(), which can translate Oracle-specific errors but not Postgres ones. This is not good. We actually shall fix the code to catch WrappedSQLException on Oracle as well. For the time being I committed your patch (with an extra comment) as: 3cf0c8132c8fa4c9b79b41478b7d6679457196c2 Thank you, -- Tomas Lestach Red Hat Satellite Engineering, Red Hat ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Avoid a possible concurrency issue on RhnSet update
On 09/05/2013 02:26 AM, Jan Pazdziora wrote: Please note that this will not work -- the fact that you check content of some table in your session and your transaction and based on that run the insert operation does not mean that the other session cannot do exactly the same, both will find the record not there, and you will get the ORA-1 anyway. Good point: actually I was thinking about that statement being the only one in its transaction, but that's actually not the case. I think I will do as Thomas suggested, handling exceptions better in order to be as safe as possible when ignoring them, yet accepting failures in this specific case. I will be back with a patch as soon as I have tested it. Thanks for the discussion by the way, as problems in this topic are traditionally difficult to handle! Regards, -- Silvio Moioli SUSE LINUX Products GmbH Maxfeldstraße 5, 90409 Nürnberg Germany ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Avoid a possible concurrency issue on RhnSet update
On 09/03/2013 02:33 PM, Tomas Lestach wrote: to be honest, I personally do not like seeing DB index names in the application code. I agree that it is not really an elegant solution, but I could not find another way of ignoring such specific exceptions. Ideas welcome :-) (Btw. does it work on PG, when the index name is stated uppercase?) Actually, we could not reproduce the problem in Postgres at all. If the proposed solution turns out to be acceptable, I will research that. Isn't it possible to use 'SELECT FOR UPDATE' in these cases? I do not really think so, because AFAIU there is actually no SELECT involved. Failing statements are INSERTs that add tuples like (user, label, system, id), all built from HTTP request parameters, so there is no need to fetch data from the database in order to build them. In order not to add duplicates those INSERTs are preceded by DELETEs rather than SELECTs, so I guess write locks are acquired anyway. No, if both transactions start at the same time and one of them commits, the other transaction does not see those changes. Maybe I am missing something here but this is not what I observe. On Postgres, you can easily prove this using pgAdmin III and two Query windows. In the first window input: BEGIN; INSERT INTO rhnset (user_id, label, element, element_two, element_three) VALUES ( (SELECT id FROM web_contact LIMIT 1), 'test', 1, 1, 1 ); F5 (this leaves the transaction pending uncommitted [1]) And in the second one: BEGIN; SELECT * FROM rhnset WHERE label = 'test'; F5 You should not see the above test row. Now switch back to the first window and input: COMMIT; F5 (this commits the pending transaction) Finally switch back to the second window and repeat the query: SELECT * FROM rhnset WHERE label = 'test'; F5 You should see the added row. Note that the second transaction did see the update while not being committed. In fact if you run COMMIT; Postgres will close it (running COMMIT a second time will emit a warning that no transactions were pending). As I wrote, this is the expected behavior since the default READ COMMITTED isolation level is used [2]: [...] two successive SELECT commands can see different data, even though they are within a single transaction, if other transactions commit changes during execution [...] The same proof can be done with Oracle SQL Developer with minor syntax fixes. Note that you will have to open two connections to the same database because, by default, all SQL Worksheets there will refer to the same transaction. On 09/03/2013 04:14 PM, Paul Robert Marino wrote: I'm a little rusty on my Oracle but in PostgreSQL I think this could be resolved using the Isolation level within the transaction That could be done, but since a transaction is opened and closed at the beginning and end of an HTTP request (at least most of the time), that would mean to either change isolation level for all requests or introducing a mechanism to flag certain actions/URLs to get a different isolation level. Also, code using any transaction at an isolation level above READ COMMITTED should be modified to handle transaction serialization errors and be prepared to retry failed transactions, which AFAIK is never done now. possibly snapshots within the transaction That would mean saving snapshot IDs between HTTP requests. Again, doable, but sounds quite difficult at least to me! Yet another possibility would be to INSERT only tuples not already present by set difference, something like this query: INSERT INTO rhnset (user_id, label, element, element_two, element_three) SELECT :user_id, :label, :element, :element_two, :element_three FROM dual WHERE (:user_id, :label, :element, :element_two, :element_three) NOT IN ( SELECT * FROM rhnset ) Is that better in your opinion? [1] http://www.postgresql.org/message-id/937d27e10810020633k164e7f4bsb5213d86d2bfb...@mail.gmail.com [2] http://www.postgresql.org/docs/9.2/static/transaction-iso.html#XACT-READ-COMMITTED Regards, -- Silvio Moioli SUSE LINUX Products GmbH Maxfeldstraße 5, 90409 Nürnberg Germany ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Avoid a possible concurrency issue on RhnSet update
(Btw. does it work on PG, when the index name is stated uppercase?) Actually, we could not reproduce the problem in Postgres at all. This is interesting. Isn't it possible to use 'SELECT FOR UPDATE' in these cases? I do not really think so, because AFAIU there is actually no SELECT involved. Failing statements are INSERTs that add tuples like (user, label, system, id), all built from HTTP request parameters, so there is no need to fetch data from the database in order to build them. I see. No, if both transactions start at the same time and one of them commits, the other transaction does not see those changes. Maybe I am missing something here but this is not what I observe. Right. I am sorry, I was wrong. Yet another possibility would be to INSERT only tuples not already present by set difference, something like this query: INSERT INTO rhnset (user_id, label, element, element_two, element_three) SELECT :user_id, :label, :element, :element_two, :element_three FROM dual WHERE (:user_id, :label, :element, :element_two, :element_three) NOT IN ( SELECT * FROM rhnset ) Is that better in your opinion? Options: * We will accept this solution (the last one). * We'll accept also your first exception solution, just remove the index name, please. Actually I would expect WrappedSQLException gets thrown (not ConstraintViolationException). In case you would catch WrappedSQLException, just check it contains some information about constraint violation. In case you catch directly ConstraintViolationException, it's ok. * I am not sure, if we can explicitly tell hibernate to lock the table for us, what could be another option. So, just choose the one you like the most and I'll accept it. :-) Thank you, -- Tomas Lestach Red Hat Satellite Engineering, Red Hat ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Avoid a possible concurrency issue on RhnSet update
On Wed, Sep 04, 2013 at 08:35:27AM +0200, Silvio Moioli wrote: On 09/03/2013 02:33 PM, Tomas Lestach wrote: to be honest, I personally do not like seeing DB index names in the application code. I agree that it is not really an elegant solution, but I could not find another way of ignoring such specific exceptions. Ideas welcome :-) You can do a select after that exception to see if the record in the database is what you intended it to be. If it is (and you got an exception in your session), you can assume it was the other session which set it up. You don't really care if the operation in your session suceeded -- you care about the result. Of course, whether and how this will work with some application sessions (hibernate?), I can't really say. -- Jan Pazdziora Principal Software Engineer, Identity Management Engineering, Red Hat ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Avoid a possible concurrency issue on RhnSet update
On Wed, Sep 04, 2013 at 08:35:27AM +0200, Silvio Moioli wrote: That would mean saving snapshot IDs between HTTP requests. Again, doable, but sounds quite difficult at least to me! Yet another possibility would be to INSERT only tuples not already present by set difference, something like this query: INSERT INTO rhnset (user_id, label, element, element_two, element_three) SELECT :user_id, :label, :element, :element_two, :element_three FROM dual WHERE (:user_id, :label, :element, :element_two, :element_three) NOT IN ( SELECT * FROM rhnset ) Is that better in your opinion? Please note that this will not work -- the fact that you check content of some table in your session and your transaction and based on that run the insert operation does not mean that the other session cannot do exactly the same, both will find the record not there, and you will get the ORA-1 anyway. The only viable approach is to accept that the same operations will happen, and either losen the unique constraint requirement, or handle the exceptions which will from time to time happen. Or just live with the fact that in some scenarios, the failures will occur. -- Jan Pazdziora Principal Software Engineer, Identity Management Engineering, Red Hat ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Avoid a possible concurrency issue on RhnSet update
Hello Silvio, to be honest, I personally do not like seeing DB index names in the application code. (Btw. does it work on PG, when the index name is stated uppercase?) Isn't it possible to use 'SELECT FOR UPDATE' in these cases? (The other transaction would need to wait till the first one commits.) Hi, The attached patch attempts to fix an issue we found while running some Selenium-based automated UI tests. Basically multiple requests that involve an RhnSet can overlap if the user is clicking very quickly and that might result in an ISE because RHNSET.RHN_SET_USER_LABEL_ELEM_UNQ is violated. We verified that using a system's Errata page, specifically in code that gets executed from ErrataList.do to ErrataConfirm.do. In particular, if you check a box selecting an errata in ErrataList.do then an AJAX call is fired to DWRItemSelector.select(), which among other things updates the underlying rhnset by adding a row. Then, if you very quickly click on Apply Errata, ListRhnSetHelper will also try to add the same row because DWRItemSelector hasn't finished yet and, in case of particularly bad luck, the RDBMS will complain that the unique constraint has been violated. AFAIU this is possible from an RDBMS point of view since, both in the Oracle and in Postgres, the READ COMMITTED transaction isolation level is used. This means that a transaction can see data changing if other transactions COMMIT during its lifetime [2]. No, if both transactions start at the same time and one of them commits, the other transaction does not see those changes. If the other transaction makes the same changes, it fails first at commit time on the mentioned index. Regards, -- Tomas Lestach Red Hat Satellite Engineering, Red Hat Reproducing this issue is not very easy, since it is RDBMS, network and server-load dependent. We succeeded in reliably repeat it in SUSE Manager on Oracle using Selenium IDE for Firefox[1] to automate the clicks. Of course, this can also happen at more human time scales if, for example, server is under heavy load. The attached patch just ignores constraint violations in this specific case, as I see no point in throwing an exception when the row to INSERT is simply already there. AFAIU that does not have unwanted side-effects, but review is nevertheless very welcome. Regards, [1] http://docs.seleniumhq.org/download/ [2] http://www.postgresql.org/docs/9.2/static/transaction-iso.html -- Silvio Moioli SUSE LINUX Products GmbH Maxfeldstraße 5, 90409 Nürnberg Germany ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Avoid a possible concurrency issue on RhnSet update
I'm a little rusty on my Oracle but in PostgreSQL I think this could be resolved using the Isolation level within the transaction, and possibly snapshots within the transaction http://www.postgresql.org/docs/9.2/interactive/sql-set-transaction.html An other possibility is to request an exclusive lock could be requested for queries which have this potential but that method should be avoided if possible. On Tue, Sep 3, 2013 at 8:33 AM, Tomas Lestach tlest...@redhat.com wrote: Hello Silvio, to be honest, I personally do not like seeing DB index names in the application code. (Btw. does it work on PG, when the index name is stated uppercase?) Isn't it possible to use 'SELECT FOR UPDATE' in these cases? (The other transaction would need to wait till the first one commits.) Hi, The attached patch attempts to fix an issue we found while running some Selenium-based automated UI tests. Basically multiple requests that involve an RhnSet can overlap if the user is clicking very quickly and that might result in an ISE because RHNSET.RHN_SET_USER_LABEL_ELEM_UNQ is violated. We verified that using a system's Errata page, specifically in code that gets executed from ErrataList.do to ErrataConfirm.do. In particular, if you check a box selecting an errata in ErrataList.do then an AJAX call is fired to DWRItemSelector.select(), which among other things updates the underlying rhnset by adding a row. Then, if you very quickly click on Apply Errata, ListRhnSetHelper will also try to add the same row because DWRItemSelector hasn't finished yet and, in case of particularly bad luck, the RDBMS will complain that the unique constraint has been violated. AFAIU this is possible from an RDBMS point of view since, both in the Oracle and in Postgres, the READ COMMITTED transaction isolation level is used. This means that a transaction can see data changing if other transactions COMMIT during its lifetime [2]. No, if both transactions start at the same time and one of them commits, the other transaction does not see those changes. If the other transaction makes the same changes, it fails first at commit time on the mentioned index. Regards, -- Tomas Lestach Red Hat Satellite Engineering, Red Hat Reproducing this issue is not very easy, since it is RDBMS, network and server-load dependent. We succeeded in reliably repeat it in SUSE Manager on Oracle using Selenium IDE for Firefox[1] to automate the clicks. Of course, this can also happen at more human time scales if, for example, server is under heavy load. The attached patch just ignores constraint violations in this specific case, as I see no point in throwing an exception when the row to INSERT is simply already there. AFAIU that does not have unwanted side-effects, but review is nevertheless very welcome. Regards, [1] http://docs.seleniumhq.org/download/ [2] http://www.postgresql.org/docs/9.2/static/transaction-iso.html -- Silvio Moioli SUSE LINUX Products GmbH Maxfeldstraße 5, 90409 Nürnberg Germany ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
