Skip to content

fix: fix issue 1449#1450

Open
enkilee wants to merge 1 commit into
IvorySQL:masterfrom
enkilee:fix-ivy_guc-bug
Open

fix: fix issue 1449#1450
enkilee wants to merge 1 commit into
IvorySQL:masterfrom
enkilee:fix-ivy_guc-bug

Conversation

@enkilee

@enkilee enkilee commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

fix: #1449

Summary by CodeRabbit

  • Bug Fixes

    • Restricted ivorysql.enable_case_switch from being configured through database role settings, improving configuration consistency.
  • Documentation

    • Clarified the inline description for the default row ID setting.
  • Style

    • Applied minor formatting improvements to internal configuration declarations and helper code.

给 ivorysql.enable_case_switch 的 flags 加上 GUC_DISALLOW_IN_DB_ROLE_SETTING,与 compatible_mode 一致
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

ivy_guc.c adds GUC_DISALLOW_IN_DB_ROLE_SETTING to ivorysql.enable_case_switch and applies formatting-only updates to NLS declarations, a local variable declaration, and the default_with_rowids comment.

Changes

GUC settings

Layer / File(s) Summary
Case-switch GUC restriction
src/backend/utils/misc/ivy_guc.c
Adds GUC_DISALLOW_IN_DB_ROLE_SETTING to the ivorysql.enable_case_switch registration.
NLS declaration and documentation cleanup
src/backend/utils/misc/ivy_guc.c
Reformats NLS declarations and a local size_t len declaration, and reflows the default_with_rowids comment without changing behavior.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

Suggested reviewers: bigplaice, 123123213weqw

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is too generic and only references an issue number, so it does not clearly describe the change. Rename the PR to briefly state the main fix, such as adding GUC_DISALLOW_IN_DB_ROLE_SETTING to ivorysql.enable_case_switch.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed The changes add GUC_DISALLOW_IN_DB_ROLE_SETTING to ivorysql.enable_case_switch, matching the linked issue's requirement.
Out of Scope Changes check ✅ Passed The remaining edits are formatting and comment reflow around the same GUC change, with no clear unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/backend/utils/misc/ivy_guc.c`:
- Around line 189-193: Update the documentation comment for default_with_rowids
to state that it applies only in Oracle compatibility mode and only when table
creation does not specify an explicit ROWID option; avoid describing it as
applying to all newly created tables unconditionally.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9d1d8c8f-10e6-4f3c-a0fc-eb652baa14f6

📥 Commits

Reviewing files that changed from the base of the PR and between 15d3b5b and 6345260.

⛔ Files ignored due to path filters (1)
  • src/backend/utils/misc/guc_parameters.dat is excluded by !**/*.dat
📒 Files selected for processing (1)
  • src/backend/utils/misc/ivy_guc.c

Comment on lines +189 to +193
* When enabled, all newly created tables will automatically include an
* Oracle-compatible ROWID pseudo-column. This provides compatibility with
* Oracle applications that rely on ROWID for row identification.
*
* Default: off
* Context: USERSET (can be changed by any user)
* Default: off Context: USERSET (can be changed by any user)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Clarify the default_with_rowids scope.

Line 189 says all newly created tables receive ROWID columns, but the runtime uses this GUC only in Oracle compatibility mode and only when no explicit ROWID option is provided. Please document those conditions to avoid misleading users.

Proposed wording
-	 * When enabled, all newly created tables will automatically include an
-	 * Oracle-compatible ROWID pseudo-column. This provides compatibility with
-	 * Oracle applications that rely on ROWID for row identification.
+	 * When enabled in Oracle compatibility mode, tables without an explicit
+	 * ROWID option default to an Oracle-compatible ROWID pseudo-column.
📝 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
* When enabled, all newly created tables will automatically include an
* Oracle-compatible ROWID pseudo-column. This provides compatibility with
* Oracle applications that rely on ROWID for row identification.
*
* Default: off
* Context: USERSET (can be changed by any user)
* Default: off Context: USERSET (can be changed by any user)
* When enabled in Oracle compatibility mode, tables without an explicit
* ROWID option default to an Oracle-compatible ROWID pseudo-column.
*
* Default: off Context: USERSET (can be changed by any user)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/utils/misc/ivy_guc.c` around lines 189 - 193, Update the
documentation comment for default_with_rowids to state that it applies only in
Oracle compatibility mode and only when table creation does not specify an
explicit ROWID option; avoid describing it as applying to all newly created
tables unconditionally.

@hs-liuxh

Copy link
Copy Markdown
Collaborator

Thanks for your contribution! I'll review it shortly.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ivorysql.enable_case_switch 缺少 GUC_DISALLOW_IN_DB_ROLE_SETTING 保护

2 participants