Improve ICU version mismatch diagnostics

[dyemanov]

While connecting to a database copied from a different machine, I get error:

COLLATION UNICODE_CI for CHARACTER SET UTF8 is not installed

which does not offer any hint what is actually wrong. After looking at firebird.log, I see:

Wed Apr 13 18:52:20 2022
	loadICU failed

Wed Apr 13 18:52:20 2022
	initUnicodeCollation failed - UnicodeUtil::Utf16Collation::create failed

Given that I surely have ICU installed, this at least suggests that ICU version could be a problem. And yes, the problem disappears after gfix -icu. However, it would be more user-friendly if:

1. The log contains something like "expected ICU version X, encountered Y", or "ICU version X is not found" or something like this, mentioning that a particular version is not found.
2. The same ICU loading error is added as a secondary code to the status vector, so that the user could understand what's wrong without digging into the server log.

[AlexPeshkoff]

I'm afraid that the best we can suggest is 'Available ICU versions do not support existing collation <NAME>'. ICU version used for collation creation is not stored in index, only collator version is present. And it's correct choice cause multiple ICU versions may provide same collator version.

[aafemt]

Still it would be good to add into the message the required and actual collator's versions.

[AlexPeshkoff]

Required can be added, actual - too, but there is one issue with it. What if >1 ICU is installed and they provide different collators? Enumerate them all?
PS. Collator's version is typically poor human readable string.

[aafemt]

Even if multiple ICU versions are installed only one of them is loaded into Firebird, right? Readability of version string is ICU's support problem. Questions like "which version of ICU supports XXXXXXX collator" can be asked at them.

[AlexPeshkoff]

No, firebird can load >1 ICU library.

[asfernandes]

I will see if we can maintain ICU-VERSION in attributes for information purposes only.

Then we can have some messages like: "ICU library with collator version X.Y required, for example ICU version W.Z".

But this is for the log.

INTL api does not support custom errors. Unless we do another workaround on it.

[dyemanov]

If we cannot (or don't want to) change the INTL API, maybe it's worth extending it with a function returning the status to the caller (semantically similar to GetLastError() / errno)?

[asfernandes]

If we cannot (or don't want to) change the INTL API, maybe it's worth extending it with a function returning the status to the caller (semantically similar to GetLastError() / errno)?

That would make it difficult to change all INTL code to always set an error to be get by the engine. I'd also want to avoid adding TLS buffers to INTL.

Instead I would add new texttype lookup entrypoint accepting a char* status_buffer among its size, to be set by INTL code.

Why not ISC_STATUS[]? It would make things difficult as there is no way for INTL libraries to add strings to circular buffer.

[AlexPeshkoff]

On 4/19/22 15:25, Adriano dos Santos Fernandes wrote: If we cannot (or don't want to) change the INTL API, maybe it's worth extending it with a function returning the status to the caller (semantically similar to GetLastError() / errno)? That would make it difficult to change all INTL code to always set an error to be get by the engine. I'd also want to avoid adding TLS buffers to INTL. Instead I would add new texttype lookup entrypoint accepting a |char* status_buffer| among its size, to be set by INTL code. Why not ISC_STATUS[]? It would make things difficult as there is no way for INTL libraries to add strings to circular buffer.

We do not use circular buffer in engine since fb3, it's used only by yValve in ISC API gate. With status interface it's just not needed.

[asfernandes]

We do not use circular buffer in engine since fb3, it's used only by yValve in ISC API gate. With status interface it's just not needed.

INTL is a C-API. We should not add IStatus to it.

Nor should we'd go generate C API for our new-style API just because of INTL.

INTL is an API deserving a full refactoring/recreation. Until it we'd better do small workaround patches in it to achieve our main goals.

[AlexPeshkoff]

On 4/19/22 15:33, Adriano dos Santos Fernandes wrote: We do not use circular buffer in engine since fb3, it's used only by yValve in ISC API gate. With status interface it's just not needed. INTL is a C-API. We should not add IStatus to it. Nor should we'd go generate C API for our new-style API just because of INTL. INTL is an API deserving a full refactoring/recreation. Until it we'd better do small workaround patches in it to achieve our main goals.

I do not insist. Text string is much better than nothing (like now). But if needed there are a lot of ways to pass cloop interface to C API. Even if C generator is broken (if not - why not generate full API?) code needed to pass single interface with a few functions in it can be added manually. BTW, do you know about something using that API except our fbintl module?

[asfernandes]

BTW, do you know about something using that API except our fbintl module?

No.