search

FirebirdSQL / firebird

Firebird server, client and tools
https://firebirdsql.org
1.26k stars 217 forks source link

Do not allow run concurrent sweep instances #8320

Open TreeHunter9 opened 6 days ago

TreeHunter9 commented 6 days ago

Old code has a bug where if we already have a sweep instance running, and some one call sweep once more, allowSweepRun() will return false and we are gonna call clearSweepFlags(), that will reset sweep flags and release a sweep lock. This will open an opportunity to run a second sweep instance for both Super and Classic server.

AlexPeshkoff commented 5 days ago

As far as I can see isc_sweep_concurrent_instance is returned to the client which started sweep. Where is is handled in a case when sweep started automatically? IMO it should be silently ignored.

TreeHunter9 commented 3 days ago

As I can see, allowSweepThread() will not allow us to run auto sweep if we have concurrent instances, but I added a check for isc_sweep_concurrent_instance anyway, just in case.

AlexPeshkoff commented 3 days ago

OK. I reviewed the code once again. Initial problem can be fixed much simpler:

diff --git a/src/jrd/tra.cpp b/src/jrd/tra.cpp index 3acd848976..f289c0d923 100644 --- a/src/jrd/tra.cpp +++ b/src/jrd/tra.cpp @@ -1848,7 +1848,6 @@ void TRA_sweep(thread_db* tdbb)

        if (!dbb->allowSweepRun(tdbb))
        {
-               dbb->clearSweepFlags(tdbb);
                return;
        }

But suggested patch contains new feature - user is notified what happened, without it sweep silently finishes in a moment. But I do not understand - why no diags in cases of RO database and explicitly set ATT_no_cleanup? If you want to add diags for sweep startup failure all cases should better be taken into an account.

TreeHunter9 commented 3 days ago

If you want to add diags for sweep startup failure all cases should better be taken into an account.

Yeah, I agree. I will add more error messages.

hvlad commented 3 days ago

But suggested patch contains new feature - user is notified what happened, without it sweep silently finishes in a moment.

How it works for auto-sweep ?

AlexPeshkoff commented 3 days ago

But suggested patch contains new feature - user is notified what happened, without it sweep silently finishes in a moment.

How it works for auto-sweep ?

See https://github.com/FirebirdSQL/firebird/pull/8320#issuecomment-2482790184