#824 Oracle Compatibility: Support for nls_currency/nls_iso_currency/…#831
Merged
gaoxueyu merged 2 commits intoIvorySQL:masterfrom Aug 4, 2025
Merged
Conversation
…urrency/nls_territory
Contributor
WalkthroughThis change introduces three new global configuration parameters ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GUC System
participant Validation Functions
User->>GUC System: Set nls_territory / nls_currency / nls_iso_currency
GUC System->>Validation Functions: Call nls_territory_check or nls_length_check
Validation Functions->>Validation Functions: Perform case conversion and validation
Validation Functions-->>GUC System: Return success or error
GUC System-->>User: Apply or reject parameter change
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/backend/utils/misc/ivy_guc.c(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/backend/utils/misc/ivy_guc.c (1)
src/backend/utils/error/elog.c (1)
errmsg(1069-1085)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: contrib_regression
Comment on lines
+524
to
+540
| static bool | ||
| nls_length_check(char **newval, void **extra, GucSource source) | ||
| { | ||
| if (DB_ORACLE == database_mode | ||
| && (IsNormalProcessingMode() || (IsUnderPostmaster && MyProcPort))) | ||
| { | ||
| if (strlen(*newval) > 255) | ||
| ereport(ERROR, (errmsg("parameter value longer than 255 characters"))); | ||
| else if (isdigit(**newval)) | ||
| ereport(ERROR, (errmsg("Cannot access NLS data files " | ||
| "or invalid environment specified"))); | ||
| else if (identifier_case_switch == INTERCHANGE) | ||
| nls_case_conversion(newval, 'b'); | ||
| } | ||
|
|
||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error messages and fix potential undefined behavior
The error messages are misleading and isdigit() needs proper casting to avoid undefined behavior with negative char values.
static bool
nls_length_check(char **newval, void **extra, GucSource source)
{
if (DB_ORACLE == database_mode
&& (IsNormalProcessingMode() || (IsUnderPostmaster && MyProcPort)))
{
if (strlen(*newval) > 255)
ereport(ERROR, (errmsg("parameter value longer than 255 characters")));
- else if (isdigit(**newval))
- ereport(ERROR, (errmsg("Cannot access NLS data files "
- "or invalid environment specified")));
+ else if (isdigit((unsigned char)**newval))
+ ereport(ERROR, (errmsg("parameter value cannot start with a digit")));
else if (identifier_case_switch == INTERCHANGE)
nls_case_conversion(newval, 'b');
}
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static bool | |
| nls_length_check(char **newval, void **extra, GucSource source) | |
| { | |
| if (DB_ORACLE == database_mode | |
| && (IsNormalProcessingMode() || (IsUnderPostmaster && MyProcPort))) | |
| { | |
| if (strlen(*newval) > 255) | |
| ereport(ERROR, (errmsg("parameter value longer than 255 characters"))); | |
| else if (isdigit(**newval)) | |
| ereport(ERROR, (errmsg("Cannot access NLS data files " | |
| "or invalid environment specified"))); | |
| else if (identifier_case_switch == INTERCHANGE) | |
| nls_case_conversion(newval, 'b'); | |
| } | |
| return true; | |
| } | |
| static bool | |
| nls_length_check(char **newval, void **extra, GucSource source) | |
| { | |
| if (DB_ORACLE == database_mode | |
| && (IsNormalProcessingMode() || (IsUnderPostmaster && MyProcPort))) | |
| { | |
| if (strlen(*newval) > 255) | |
| ereport(ERROR, (errmsg("parameter value longer than 255 characters"))); | |
| else if (isdigit((unsigned char)**newval)) | |
| ereport(ERROR, (errmsg("parameter value cannot start with a digit"))); | |
| else if (identifier_case_switch == INTERCHANGE) | |
| nls_case_conversion(newval, 'b'); | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In src/backend/utils/misc/ivy_guc.c around lines 524 to 540, improve the error
messages to be more descriptive and user-friendly. Also, fix the potential
undefined behavior by casting the character passed to isdigit() to unsigned char
before the check. Update the error messages to clearly explain the issue and
ensure the isdigit() call is safe by proper casting.
Comment on lines
+524
to
+558
| static bool | ||
| nls_length_check(char **newval, void **extra, GucSource source) | ||
| { | ||
| if (DB_ORACLE == database_mode | ||
| && (IsNormalProcessingMode() || (IsUnderPostmaster && MyProcPort))) | ||
| { | ||
| if (strlen(*newval) > 255) | ||
| ereport(ERROR, (errmsg("parameter value longer than 255 characters"))); | ||
| else if (isdigit(**newval)) | ||
| ereport(ERROR, (errmsg("Cannot access NLS data files " | ||
| "or invalid environment specified"))); | ||
| else if (identifier_case_switch == INTERCHANGE) | ||
| nls_case_conversion(newval, 'b'); | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| static bool | ||
| nls_territory_check(char **newval, void **extra, GucSource source) | ||
| { | ||
| if (DB_ORACLE == database_mode | ||
| && (IsNormalProcessingMode() || (IsUnderPostmaster && MyProcPort))) | ||
| { | ||
| if (pg_strcasecmp(*newval, "CHINA") == 0) | ||
| memcpy(*newval, "CHINA", 5); | ||
| else if (pg_strcasecmp(*newval, "AMERICA") == 0) | ||
| memcpy(*newval, "AMERICA", 7); | ||
| else | ||
| ereport(ERROR, (errmsg("Cannot access NLS data files " | ||
| "or invalid environment specified"))); | ||
| } | ||
|
|
||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify GUC string parameter memory management
The validation functions modify the input string buffers directly, which might not be safe depending on how the GUC system manages string parameters. Need to verify the correct pattern for modifying string values in GUC check hooks.
🏁 Script executed:
#!/bin/bash
# Description: Check how other GUC string validation functions handle string modifications
# Search for other string check functions in the codebase
echo "=== Searching for GUC string check function patterns ==="
rg -A 10 "static bool.*check.*\(char \*\*newval" src/backend/utils/misc/
# Look for examples of string manipulation in GUC check functions
echo "=== Searching for string modifications in GUC check functions ==="
ast-grep --pattern 'static bool $FUNC(char **newval, $$$) {
$$$
}'Length of output: 5920
Replace in-place string mutations with GUC‐safe allocations
GUC check hooks must not mutate the incoming buffer directly, as that can corrupt the GUC memory context. Instead, any modified string should be allocated anew (e.g. via pstrdup in the GUC context) and assigned back to *newval.
• src/backend/utils/misc/ivy_guc.c, nls_length_check (around line 536):
Replace
nls_case_conversion(newval, 'b');
with code that builds the converted string in GUC memory (e.g. char *conv = your_conversion_fn(*newval, 'b'); *newval = pstrdup(conv);).
• src/backend/utils/misc/ivy_guc.c, nls_territory_check (lines 549 & 551):
Replace
memcpy(*newval, "CHINA", 5);
memcpy(*newval, "AMERICA", 7);
with
*newval = pstrdup("CHINA");
*newval = pstrdup("AMERICA");
This change guarantees that the GUC framework can manage and free the new string correctly.
🤖 Prompt for AI Agents
In src/backend/utils/misc/ivy_guc.c around lines 536 and 549-551, the code
directly mutates the string pointed to by *newval, which can corrupt GUC memory
management. To fix this, replace the call to nls_case_conversion with code that
creates a new converted string in GUC memory (e.g., call a conversion function
that returns a new string, then assign *newval = pstrdup(converted_string)).
Similarly, replace the memcpy calls that overwrite *newval with assignments that
duplicate the constant strings using pstrdup (e.g., *newval = pstrdup("CHINA")
and *newval = pstrdup("AMERICA")). This ensures the GUC framework properly
manages the memory of the new strings.
Comment on lines
+542
to
+558
| static bool | ||
| nls_territory_check(char **newval, void **extra, GucSource source) | ||
| { | ||
| if (DB_ORACLE == database_mode | ||
| && (IsNormalProcessingMode() || (IsUnderPostmaster && MyProcPort))) | ||
| { | ||
| if (pg_strcasecmp(*newval, "CHINA") == 0) | ||
| memcpy(*newval, "CHINA", 5); | ||
| else if (pg_strcasecmp(*newval, "AMERICA") == 0) | ||
| memcpy(*newval, "AMERICA", 7); | ||
| else | ||
| ereport(ERROR, (errmsg("Cannot access NLS data files " | ||
| "or invalid environment specified"))); | ||
| } | ||
|
|
||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Unsafe string manipulation with memcpy
Using memcpy without including the null terminator is dangerous and can lead to buffer overflows. The current code doesn't copy the null terminator, which will result in unterminated strings.
static bool
nls_territory_check(char **newval, void **extra, GucSource source)
{
if (DB_ORACLE == database_mode
&& (IsNormalProcessingMode() || (IsUnderPostmaster && MyProcPort)))
{
if (pg_strcasecmp(*newval, "CHINA") == 0)
- memcpy(*newval, "CHINA", 5);
+ memcpy(*newval, "CHINA", 6); /* Include null terminator */
else if (pg_strcasecmp(*newval, "AMERICA") == 0)
- memcpy(*newval, "AMERICA", 7);
+ memcpy(*newval, "AMERICA", 8); /* Include null terminator */
else
- ereport(ERROR, (errmsg("Cannot access NLS data files "
- "or invalid environment specified")));
+ ereport(ERROR, (errmsg("invalid territory value, must be 'CHINA' or 'AMERICA'")));
}
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static bool | |
| nls_territory_check(char **newval, void **extra, GucSource source) | |
| { | |
| if (DB_ORACLE == database_mode | |
| && (IsNormalProcessingMode() || (IsUnderPostmaster && MyProcPort))) | |
| { | |
| if (pg_strcasecmp(*newval, "CHINA") == 0) | |
| memcpy(*newval, "CHINA", 5); | |
| else if (pg_strcasecmp(*newval, "AMERICA") == 0) | |
| memcpy(*newval, "AMERICA", 7); | |
| else | |
| ereport(ERROR, (errmsg("Cannot access NLS data files " | |
| "or invalid environment specified"))); | |
| } | |
| return true; | |
| } | |
| static bool | |
| nls_territory_check(char **newval, void **extra, GucSource source) | |
| { | |
| if (DB_ORACLE == database_mode | |
| && (IsNormalProcessingMode() || (IsUnderPostmaster && MyProcPort))) | |
| { | |
| if (pg_strcasecmp(*newval, "CHINA") == 0) | |
| memcpy(*newval, "CHINA", 6); /* Include null terminator */ | |
| else if (pg_strcasecmp(*newval, "AMERICA") == 0) | |
| memcpy(*newval, "AMERICA", 8); /* Include null terminator */ | |
| else | |
| ereport(ERROR, (errmsg("invalid territory value, must be 'CHINA' or 'AMERICA'"))); | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In src/backend/utils/misc/ivy_guc.c around lines 542 to 558, the use of memcpy
to copy strings "CHINA" and "AMERICA" does not include the null terminator,
risking buffer overflows and unterminated strings. Replace memcpy calls with a
safe string copy function like strcpy or explicitly copy the null terminator by
increasing the length by one to ensure the destination string is properly
null-terminated.
Comment on lines
+477
to
+523
| static void | ||
| nls_case_conversion(char **param, char type) | ||
| { | ||
| char *p; | ||
|
|
||
| CASE_CONVERSION: | ||
| if (type == 'u') | ||
| { | ||
| for (p = *param; p < *param + strlen(*param); ++p) | ||
| if (97 <= *p && *p <= 122) | ||
| *p -= 32; | ||
| *p = '\0'; | ||
| } | ||
| else if (type == 'l') | ||
| { | ||
| for (p = *param; p < *param + strlen(*param); ++p) | ||
| if (65 <= *p && *p <= 90) | ||
| *p += 32; | ||
| *p = '\0'; | ||
| } | ||
| else if (type == 'b') | ||
| { | ||
| bool has_upper = false, | ||
| has_lower = false; | ||
|
|
||
| for (p = *param; p < *param + strlen(*param); ++p) | ||
| { | ||
| if (65 <= *p && *p <= 90) | ||
| has_upper = true; | ||
| else if (97 <= *p && *p <= 122) | ||
| has_lower = true; | ||
| if (has_upper && has_lower) | ||
| return; | ||
| } | ||
|
|
||
| if (has_upper && !has_lower) | ||
| { | ||
| type = 'l'; | ||
| goto CASE_CONVERSION; | ||
| } | ||
| else if (!has_upper && has_lower) | ||
| { | ||
| type = 'u'; | ||
| goto CASE_CONVERSION; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor case conversion logic to eliminate goto and support non-ASCII characters
The current implementation has several issues:
- Uses
gotowhich makes the code harder to read and maintain - Manual ASCII value checks won't work correctly with non-ASCII characters
- Should use standard C library functions for better portability
static void
nls_case_conversion(char **param, char type)
{
char *p;
-CASE_CONVERSION:
- if (type == 'u')
- {
- for (p = *param; p < *param + strlen(*param); ++p)
- if (97 <= *p && *p <= 122)
- *p -= 32;
- *p = '\0';
- }
- else if (type == 'l')
- {
- for (p = *param; p < *param + strlen(*param); ++p)
- if (65 <= *p && *p <= 90)
- *p += 32;
- *p = '\0';
- }
- else if (type == 'b')
+ if (type == 'b')
{
bool has_upper = false,
has_lower = false;
for (p = *param; p < *param + strlen(*param); ++p)
{
- if (65 <= *p && *p <= 90)
+ if (isupper((unsigned char)*p))
has_upper = true;
- else if (97 <= *p && *p <= 122)
+ else if (islower((unsigned char)*p))
has_lower = true;
if (has_upper && has_lower)
return;
}
if (has_upper && !has_lower)
- {
type = 'l';
- goto CASE_CONVERSION;
- }
else if (!has_upper && has_lower)
- {
type = 'u';
- goto CASE_CONVERSION;
- }
+ else
+ return;
+ }
+
+ if (type == 'u')
+ {
+ for (p = *param; *p; ++p)
+ *p = toupper((unsigned char)*p);
+ }
+ else if (type == 'l')
+ {
+ for (p = *param; *p; ++p)
+ *p = tolower((unsigned char)*p);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static void | |
| nls_case_conversion(char **param, char type) | |
| { | |
| char *p; | |
| CASE_CONVERSION: | |
| if (type == 'u') | |
| { | |
| for (p = *param; p < *param + strlen(*param); ++p) | |
| if (97 <= *p && *p <= 122) | |
| *p -= 32; | |
| *p = '\0'; | |
| } | |
| else if (type == 'l') | |
| { | |
| for (p = *param; p < *param + strlen(*param); ++p) | |
| if (65 <= *p && *p <= 90) | |
| *p += 32; | |
| *p = '\0'; | |
| } | |
| else if (type == 'b') | |
| { | |
| bool has_upper = false, | |
| has_lower = false; | |
| for (p = *param; p < *param + strlen(*param); ++p) | |
| { | |
| if (65 <= *p && *p <= 90) | |
| has_upper = true; | |
| else if (97 <= *p && *p <= 122) | |
| has_lower = true; | |
| if (has_upper && has_lower) | |
| return; | |
| } | |
| if (has_upper && !has_lower) | |
| { | |
| type = 'l'; | |
| goto CASE_CONVERSION; | |
| } | |
| else if (!has_upper && has_lower) | |
| { | |
| type = 'u'; | |
| goto CASE_CONVERSION; | |
| } | |
| } | |
| } | |
| static void | |
| nls_case_conversion(char **param, char type) | |
| { | |
| char *p; | |
| if (type == 'b') | |
| { | |
| bool has_upper = false, | |
| has_lower = false; | |
| for (p = *param; *p; ++p) | |
| { | |
| if (isupper((unsigned char)*p)) | |
| has_upper = true; | |
| else if (islower((unsigned char)*p)) | |
| has_lower = true; | |
| if (has_upper && has_lower) | |
| return; | |
| } | |
| if (has_upper && !has_lower) | |
| type = 'l'; | |
| else if (!has_upper && has_lower) | |
| type = 'u'; | |
| else | |
| return; | |
| } | |
| if (type == 'u') | |
| { | |
| for (p = *param; *p; ++p) | |
| *p = toupper((unsigned char)*p); | |
| } | |
| else if (type == 'l') | |
| { | |
| for (p = *param; *p; ++p) | |
| *p = tolower((unsigned char)*p); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/backend/utils/misc/ivy_guc.c lines 477 to 523, refactor the
nls_case_conversion function to remove the goto statement by restructuring the
logic with loops or conditionals. Replace manual ASCII range checks with
standard C library functions like isupper, islower, toupper, and tolower to
properly handle non-ASCII characters and improve portability. Ensure the
function maintains the same behavior while improving readability and
correctness.
This was referenced Jan 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit