From 7cc21bffb3cb1a7e85e7ab3b95b860ccc3bf5328 Mon Sep 17 00:00:00 2001 From: Artyom Ivanov Date: Fri, 15 Nov 2024 19:10:14 +0300 Subject: [PATCH 1/4] Do not allow run concurrent sweep instances --- src/include/firebird/impl/msg/jrd.h | 1 + src/include/gen/Firebird.pas | 1 + src/jrd/Database.cpp | 4 ++-- src/jrd/Database.h | 3 ++- 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/include/firebird/impl/msg/jrd.h b/src/include/firebird/impl/msg/jrd.h index 84055568c20..136bc28c583 100644 --- a/src/include/firebird/impl/msg/jrd.h +++ b/src/include/firebird/impl/msg/jrd.h @@ -987,3 +987,4 @@ FB_IMPL_MSG(JRD, 984, incompatible_format_patterns, -901, "HY", "000", "@1 incom FB_IMPL_MSG(JRD, 985, only_one_pattern_can_be_used, -901, "HY", "000", "Can use only one of these patterns @1") FB_IMPL_MSG(JRD, 986, can_not_use_same_pattern_twice, -901, "HY", "000", "Cannot use the same pattern twice: @1") FB_IMPL_MSG(JRD, 987, sysf_invalid_gen_uuid_version, -833, "42", "000", "Invalid GEN_UUID version (@1). Must be 4 or 7") +FB_IMPL_MSG(JRD, 988, sweep_concurrent_instance, -901, "42", "000", "Another instance of sweep is already running") diff --git a/src/include/gen/Firebird.pas b/src/include/gen/Firebird.pas index 9c1be4d79af..4de43377d4a 100644 --- a/src/include/gen/Firebird.pas +++ b/src/include/gen/Firebird.pas @@ -5740,6 +5740,7 @@ IProfilerStatsImpl = class(IProfilerStats) isc_only_one_pattern_can_be_used = 335545305; isc_can_not_use_same_pattern_twice = 335545306; isc_sysf_invalid_gen_uuid_version = 335545307; + isc_sweep_concurrent_instance = 335545308; isc_gfix_db_name = 335740929; isc_gfix_invalid_sw = 335740930; isc_gfix_incmp_sw = 335740932; diff --git a/src/jrd/Database.cpp b/src/jrd/Database.cpp index c88b61171dd..7e56ea98b7a 100644 --- a/src/jrd/Database.cpp +++ b/src/jrd/Database.cpp @@ -316,7 +316,7 @@ namespace Jrd if (old & DBB_sweep_in_progress) { clearSweepStarting(); - return false; + ERR_post(Arg::Gds(isc_sweep_concurrent_instance)); } if (dbb_flags.compareExchange(old, old | DBB_sweep_in_progress)) @@ -336,7 +336,7 @@ namespace Jrd fb_utils::init_status(tdbb->tdbb_status_vector); dbb_flags &= ~DBB_sweep_in_progress; - return false; + ERR_post(Arg::Gds(isc_sweep_concurrent_instance)); } } else diff --git a/src/jrd/Database.h b/src/jrd/Database.h index b8ef78e878a..c8263f15f4f 100644 --- a/src/jrd/Database.h +++ b/src/jrd/Database.h @@ -657,7 +657,8 @@ class Database : public pool_alloc // returns true if sweeper thread could start bool allowSweepThread(thread_db* tdbb); - // returns true if sweep could run + // Returns true if sweep could run. + // Exception can be thrown if another sweep instance is already running. bool allowSweepRun(thread_db* tdbb); // reset sweep flag and release sweep lock void clearSweepFlags(thread_db* tdbb); From 154c3cf56154392378cda3485a3791c656986869 Mon Sep 17 00:00:00 2001 From: Artyom Ivanov Date: Mon, 18 Nov 2024 14:29:11 +0300 Subject: [PATCH 2/4] Silently ignore error about concurrent sweep instance when we try to run auto sweep --- src/jrd/tra.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/jrd/tra.cpp b/src/jrd/tra.cpp index 3acd8489761..8bf694e5e0b 100644 --- a/src/jrd/tra.cpp +++ b/src/jrd/tra.cpp @@ -2756,6 +2756,9 @@ namespace { status.check(); AutoRelease att(prov->attachDatabase(&status, dbName.c_str(), dpbLen, dpbBytes)); + if (fb_utils::containsErrorCode(status->getErrors(), isc_sweep_concurrent_instance)) + return; + status.check(); } From 9912b8a3cfcb39ef8ac00cb89579319b2a0887a6 Mon Sep 17 00:00:00 2001 From: Artyom Ivanov Date: Tue, 19 Nov 2024 16:42:14 +0300 Subject: [PATCH 3/4] Add more error messages that describe a reason of failed sweep startup --- src/include/firebird/impl/msg/jrd.h | 5 ++++- src/include/gen/Firebird.pas | 5 ++++- src/jrd/Database.cpp | 28 +++++++++++++++++----------- src/jrd/Database.h | 5 ++--- src/jrd/tra.cpp | 8 ++------ 5 files changed, 29 insertions(+), 22 deletions(-) diff --git a/src/include/firebird/impl/msg/jrd.h b/src/include/firebird/impl/msg/jrd.h index 136bc28c583..e2fb64d6113 100644 --- a/src/include/firebird/impl/msg/jrd.h +++ b/src/include/firebird/impl/msg/jrd.h @@ -987,4 +987,7 @@ FB_IMPL_MSG(JRD, 984, incompatible_format_patterns, -901, "HY", "000", "@1 incom FB_IMPL_MSG(JRD, 985, only_one_pattern_can_be_used, -901, "HY", "000", "Can use only one of these patterns @1") FB_IMPL_MSG(JRD, 986, can_not_use_same_pattern_twice, -901, "HY", "000", "Cannot use the same pattern twice: @1") FB_IMPL_MSG(JRD, 987, sysf_invalid_gen_uuid_version, -833, "42", "000", "Invalid GEN_UUID version (@1). Must be 4 or 7") -FB_IMPL_MSG(JRD, 988, sweep_concurrent_instance, -901, "42", "000", "Another instance of sweep is already running") +FB_IMPL_MSG(JRD, 988, sweep_unable_to_run, -901, "42", "000", "Unable to run sweep") +FB_IMPL_MSG(JRD, 989, sweep_concurrent_instance, -901, "42", "000", "Another instance of sweep is already running") +FB_IMPL_MSG(JRD, 990, sweep_read_only, -901, "42", "000", "Database in read only state") +FB_IMPL_MSG(JRD, 991, sweep_attach_no_cleanup, -901, "42", "000", "Attachment has no cleanup flag set") diff --git a/src/include/gen/Firebird.pas b/src/include/gen/Firebird.pas index 4de43377d4a..8c21277ad64 100644 --- a/src/include/gen/Firebird.pas +++ b/src/include/gen/Firebird.pas @@ -5740,7 +5740,10 @@ IProfilerStatsImpl = class(IProfilerStats) isc_only_one_pattern_can_be_used = 335545305; isc_can_not_use_same_pattern_twice = 335545306; isc_sysf_invalid_gen_uuid_version = 335545307; - isc_sweep_concurrent_instance = 335545308; + isc_sweep_unable_to_run = 335545308; + isc_sweep_concurrent_instance = 335545309; + isc_sweep_read_only = 335545310; + isc_sweep_attach_no_cleanup = 335545311; isc_gfix_db_name = 335740929; isc_gfix_invalid_sw = 335740930; isc_gfix_incmp_sw = 335740932; diff --git a/src/jrd/Database.cpp b/src/jrd/Database.cpp index 7e56ea98b7a..a08c71dbe9b 100644 --- a/src/jrd/Database.cpp +++ b/src/jrd/Database.cpp @@ -47,6 +47,14 @@ using namespace Firebird; +namespace +{ + void unableToRunSweepException(const Arg::Gds& reason) + { + ERR_post(Arg::Gds(isc_sweep_unable_to_run) << reason); + } +} + namespace Jrd { bool Database::onRawDevice() const @@ -299,16 +307,16 @@ namespace Jrd } } - bool Database::allowSweepRun(thread_db* tdbb) + void Database::initiateSweepRun(thread_db* tdbb) { - SPTHR_DEBUG(fprintf(stderr, "allowSweepRun %p\n", this)); + SPTHR_DEBUG(fprintf(stderr, FB_FUNCTION " %p\n", this)); if (readOnly()) - return false; + unableToRunSweepException(Arg::Gds(isc_sweep_read_only)); Jrd::Attachment* const attachment = tdbb->getAttachment(); if (attachment->att_flags & ATT_no_cleanup) - return false; + unableToRunSweepException(Arg::Gds(isc_sweep_attach_no_cleanup)); while (true) { @@ -316,18 +324,18 @@ namespace Jrd if (old & DBB_sweep_in_progress) { clearSweepStarting(); - ERR_post(Arg::Gds(isc_sweep_concurrent_instance)); + unableToRunSweepException(Arg::Gds(isc_sweep_concurrent_instance)); } if (dbb_flags.compareExchange(old, old | DBB_sweep_in_progress)) break; } - SPTHR_DEBUG(fprintf(stderr, "allowSweepRun - set DBB_sweep_in_progress\n")); + SPTHR_DEBUG(fprintf(stderr, FB_FUNCTION " - set DBB_sweep_in_progress\n")); if (!(dbb_flags & DBB_sweep_starting)) { - SPTHR_DEBUG(fprintf(stderr, "allowSweepRun - createSweepLock\n")); + SPTHR_DEBUG(fprintf(stderr, FB_FUNCTION " - createSweepLock\n")); createSweepLock(tdbb); if (!LCK_lock(tdbb, dbb_sweep_lock, LCK_EX, -1)) @@ -336,17 +344,15 @@ namespace Jrd fb_utils::init_status(tdbb->tdbb_status_vector); dbb_flags &= ~DBB_sweep_in_progress; - ERR_post(Arg::Gds(isc_sweep_concurrent_instance)); + unableToRunSweepException(Arg::Gds(isc_sweep_concurrent_instance)); } } else { - SPTHR_DEBUG(fprintf(stderr, "allowSweepRun - clearSweepStarting\n")); + SPTHR_DEBUG(fprintf(stderr, FB_FUNCTION " - clearSweepStarting\n")); attachment->att_flags |= ATT_from_thread; clearSweepStarting(); } - - return true; } void Database::clearSweepFlags(thread_db* tdbb) diff --git a/src/jrd/Database.h b/src/jrd/Database.h index c8263f15f4f..e4d2e0ee321 100644 --- a/src/jrd/Database.h +++ b/src/jrd/Database.h @@ -657,9 +657,8 @@ class Database : public pool_alloc // returns true if sweeper thread could start bool allowSweepThread(thread_db* tdbb); - // Returns true if sweep could run. - // Exception can be thrown if another sweep instance is already running. - bool allowSweepRun(thread_db* tdbb); + // Throw an exception if sweep cannot be run + void initiateSweepRun(thread_db* tdbb); // reset sweep flag and release sweep lock void clearSweepFlags(thread_db* tdbb); // reset sweep starting flag, release thread starting mutex diff --git a/src/jrd/tra.cpp b/src/jrd/tra.cpp index 8bf694e5e0b..70b0fb85208 100644 --- a/src/jrd/tra.cpp +++ b/src/jrd/tra.cpp @@ -1846,11 +1846,7 @@ void TRA_sweep(thread_db* tdbb) Database* const dbb = tdbb->getDatabase(); CHECK_DBB(dbb); - if (!dbb->allowSweepRun(tdbb)) - { - dbb->clearSweepFlags(tdbb); - return; - } + dbb->initiateSweepRun(tdbb); fb_assert(dbb->dbb_flags & DBB_sweep_in_progress); @@ -2756,7 +2752,7 @@ namespace { status.check(); AutoRelease att(prov->attachDatabase(&status, dbName.c_str(), dpbLen, dpbBytes)); - if (fb_utils::containsErrorCode(status->getErrors(), isc_sweep_concurrent_instance)) + if (fb_utils::containsErrorCode(status->getErrors(), isc_sweep_unable_to_run)) return; status.check(); From a0c46305b262b07de104e16b32418e04b26bd77e Mon Sep 17 00:00:00 2001 From: Artyom Ivanov Date: Wed, 20 Nov 2024 09:39:06 +0300 Subject: [PATCH 4/4] Pass ISC_STATUS instead of Arg::Gds to function --- src/jrd/Database.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/jrd/Database.cpp b/src/jrd/Database.cpp index a08c71dbe9b..eb5f17ed040 100644 --- a/src/jrd/Database.cpp +++ b/src/jrd/Database.cpp @@ -49,9 +49,9 @@ using namespace Firebird; namespace { - void unableToRunSweepException(const Arg::Gds& reason) + void unableToRunSweepException(ISC_STATUS reason) { - ERR_post(Arg::Gds(isc_sweep_unable_to_run) << reason); + ERR_post(Arg::Gds(isc_sweep_unable_to_run) << Arg::Gds(reason)); } } @@ -312,11 +312,11 @@ namespace Jrd SPTHR_DEBUG(fprintf(stderr, FB_FUNCTION " %p\n", this)); if (readOnly()) - unableToRunSweepException(Arg::Gds(isc_sweep_read_only)); + unableToRunSweepException(isc_sweep_read_only); Jrd::Attachment* const attachment = tdbb->getAttachment(); if (attachment->att_flags & ATT_no_cleanup) - unableToRunSweepException(Arg::Gds(isc_sweep_attach_no_cleanup)); + unableToRunSweepException(isc_sweep_attach_no_cleanup); while (true) { @@ -324,7 +324,7 @@ namespace Jrd if (old & DBB_sweep_in_progress) { clearSweepStarting(); - unableToRunSweepException(Arg::Gds(isc_sweep_concurrent_instance)); + unableToRunSweepException(isc_sweep_concurrent_instance); } if (dbb_flags.compareExchange(old, old | DBB_sweep_in_progress)) @@ -344,7 +344,7 @@ namespace Jrd fb_utils::init_status(tdbb->tdbb_status_vector); dbb_flags &= ~DBB_sweep_in_progress; - unableToRunSweepException(Arg::Gds(isc_sweep_concurrent_instance)); + unableToRunSweepException(isc_sweep_concurrent_instance); } } else