Skip to content

Conversation

ilya071294
Copy link
Contributor

The fix is needed to avoid potential problems in Firebird QA.

… fix cases when it's not reloaded after modification/replacement

The fix is needed to avoid potential problems in Firebird QA.
@aafemt
Copy link
Contributor

aafemt commented May 5, 2025

Wouldn't it be better to make Windows version of getTime to return timespec and unify the rest of code?

Also it is simpler to use GetFileAttributesEx than FindFirstFile.

@ilya071294
Copy link
Contributor Author

Wouldn't it be better to make Windows version of getTime to return timespec and unify the rest of code?

I can do that if conversion to timespec on every checkLoadConfig call is appropriate. @AlexPeshkoff What do you think?

Also it is simpler to use GetFileAttributesEx than FindFirstFile.

Done.

@aafemt
Copy link
Contributor

aafemt commented May 6, 2025

I can do that if conversion to timespec on every checkLoadConfig call is appropriate.

IIRC this "conversion" is a couple of multiplication.

@dyemanov
Copy link
Member

@AlexPeshkoff do you have any objections to this change?

@AlexPeshkoff
Copy link
Member

Do something like this:
namespace Firebird {
class PreciseTime
{
public:
PreciseTime() : high(0), low(0) {}
#ifdef WIN_NT
PreciseTime(//Windows specific ctor
#else
PreciseTime(//Posix specific ctor
#endif

operator=(const PreciseTime&) = default;

private:
time_t high;
unsigned low;
};
} // namespace

Next:
Firebird::PreciseTime ConfigCache::File::getTime();

And return PreciseTime from that function - code around remains as clear as it was before.

@ilya071294
Copy link
Contributor Author

I think it should be ported to FB3/4/5. Any objections?

@dyemanov dyemanov merged commit 5ea7020 into FirebirdSQL:master Sep 24, 2025
22 of 23 checks passed
@dyemanov
Copy link
Member

I think it should be ported to FB3/4/5. Any objections?

I don't mind.

@ilya071294 ilya071294 deleted the fb_master_config_reload_fix branch September 24, 2025 08:27
ilya071294 added a commit that referenced this pull request Sep 24, 2025
… a higher precision to fix cases when it's not reloaded after modification/replacement (#8553)
ilya071294 added a commit that referenced this pull request Sep 24, 2025
… a higher precision to fix cases when it's not reloaded after modification/replacement (#8553)
ilya071294 added a commit that referenced this pull request Sep 24, 2025
… a higher precision to fix cases when it's not reloaded after modification/replacement (#8553)
@ilya071294 ilya071294 self-assigned this Sep 24, 2025
@pavel-zotov
Copy link

How to implement test for this ticket ? Does some API function or context variable [will] exist for checking config LUPD time ?

@ilya071294
Copy link
Contributor Author

How to implement test for this ticket ? Does some API function or context variable [will] exist for checking config LUPD time ?

At the moment, the only way to test it is to replace some *.conf, see the effect, and then do it one more time in less than 1 second. I believe such specific test is not necessary. Anyway, this fix will be practically tested by running other tests with *.conf replacement.

@pavel-zotov
Copy link

An issue exists related to snapshots with dates before fix: in some cases engine can detect changes in the databases.conf even if time between connections much less than 1 s (~200...250 ms).
Sent logs to Dmitry and Anton Zuev (RedBase).

@ilya071294
Copy link
Contributor Author

An issue exists related to snapshots with dates before fix: in some cases engine can detect changes in the databases.conf even if time between connections much less than 1 s (~200...250 ms). Sent logs to Dmitry and Anton Zuev (RedBase).

I guess it can really happen because it depends on the exact moment when the 1-st connection is created. For example, it may be created just before the time changes to the next second. In this case changes in databases.conf will be detected by the 2-nd connection with a high probability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants