code refactor of oracle-compatibility feature rowid #926
code refactor of oracle-compatibility feature rowid #926gaoxueyu merged 5 commits intoIvorySQL:masterfrom
Conversation
WalkthroughAdds Oracle-compatible ROWID/UROWID composite types and mutual INOUT implicit casts, creates uuid-ossp extension if missing, adds a GUC to auto-create rowid columns, updates tablefunc/schema to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/backend/parser/parse_utilcmd.c (3)
347-352: Avoid inserting duplicate 'rowid' option when LIKE is used.If stmt->options already has a rowid entry, lcons will duplicate it. Guard before inserting.
Apply this diff:
- if (like_found && cxt.hasrowid) - stmt->options = lcons(makeDefElem("rowid", - (Node *) makeInteger(true), - -1), - stmt->options); + if (like_found && cxt.hasrowid) + { + bool has_rowid = false; + foreach_node(DefElem, de, stmt->options) + { + if (pg_strcasecmp(de->defname, "rowid") == 0) { has_rowid = true; break; } + } + if (!has_rowid) + stmt->options = lcons(makeDefElem("rowid", (Node *) makeInteger(true), -1), stmt->options); + }
645-647: Don’t force NOCACHE when user supplied CACHE (ORA path).Unconditionally prepending nocache can conflict with explicit CACHE in seqoptions, producing redundant/contradictory options.
- Only add nocache if neither CACHE nor NOCACHE present.
- Or honor user-provided CACHE and skip injecting NOCACHE.
337-343: Type-safe list access.Use lfirst_node(ColumnDef, ...) for clarity and type safety, matching the rest of the file.
- ColumnDef *element = lfirst(elements); + ColumnDef *element = lfirst_node(ColumnDef, elements);contrib/ivorysql_ora/preload_ora_misc.sql (1)
13-17: Consider documenting why ROWID and UROWID are separate types.Both types have identical structure
(rowoid OID, rowno bigint). In Oracle, ROWID and UROWID serve different purposes (physical vs. universal identifiers), but here they're structurally identical. While this may be intentional for Oracle compatibility, it would be helpful to document:
- Why separate types are needed if they're structurally identical
- Whether future differentiation is planned
- Any behavioral differences in how they're used
Consider adding a comment explaining the distinction:
-- -- Oracle-compatible ROWID and UROWID types -- ROWID: Physical row identifier (rowoid OID, rowno bigint) -- UROWID: Universal ROWID for compatibility with Oracle applications -- Note: Currently identical structure; maintained as separate types -- for Oracle compatibility and potential future differentiation --
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/tablefunc/expected/ivy_tablefunc.outis excluded by!**/*.out
📒 Files selected for processing (7)
contrib/ivorysql_ora/preload_ora_misc.sql(1 hunks)contrib/tablefunc/sql/ivy_tablefunc.sql(1 hunks)src/backend/catalog/heap.c(1 hunks)src/backend/parser/parse_utilcmd.c(12 hunks)src/backend/utils/misc/ivy_guc.c(1 hunks)src/include/access/htup_details.h(1 hunks)src/oracle_test/regress/sql/ora_rowid.sql(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/backend/parser/parse_utilcmd.c (2)
src/backend/nodes/makefuncs.c (3)
makeRangeVar(473-487)makeDefElem(650-662)makeTypeNameFromOid(547-556)src/backend/utils/cache/lsyscache.c (1)
get_attoptions(1067-1094)
⏰ 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: meson_build (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (10)
contrib/tablefunc/sql/ivy_tablefunc.sql (1)
13-15: Comment aligns with conflict avoidance; LGTM.Using orarowid avoids clashing with a system column name. Queries below already reference orarowid consistently.
src/include/access/htup_details.h (1)
193-194: Typo fix only; LGTM.Comment correction does not affect behavior.
src/backend/parser/parse_utilcmd.c (3)
979-987: Oracle identity: implicit widening to int8 looks fine; ensure typmod is preserved.The conversion updates typeOid and typeName; typmod for int2/int4 isn’t applicable to int8, so preserving existing typmod isn’t needed. LGTM.
Please confirm downstream DDL/RLS code paths don’t assume identity column width (e.g., default sequences’ min/max) for INT2/INT4, since they’re now INT8.
307-313: Row name guard limited to ORA_PARSER; confirm desired behavior.Blocking user-defined "rowid" only under ORA_PARSER may still allow a conflicting user column name in non-Oracle mode. If ROWID is now a system concept, consider reserving "rowid" universally.
Would you like me to draft a follow-up that reserves the identifier "rowid" across modes?
4682-4691: ColumnRefOrFuncCall is unconditionally defined in all builds—no guards needed.The typedef at src/include/nodes/parsenodes.h:4416-4421 has no preprocessor guards and is always compiled. The struct exists in both Oracle and non-Oracle builds, so the usage at parse_utilcmd.c:4682 poses no risk of build failure. The codebase conventionally uses runtime checks (compatible_db == ORA_PARSER) rather than compile-time conditionals for Oracle-specific logic.
Likely an incorrect or invalid review comment.
src/backend/catalog/heap.c (1)
231-233: LGTM! Documentation improvement enhances clarity.The updated comment provides better context about the ROWID pseudo-column's purpose and Oracle compatibility. The terminology change from "Compatible Oracle" to "Oracle-compatible" is more grammatically correct.
src/oracle_test/regress/sql/ora_rowid.sql (2)
2-2: LGTM! Improved test documentation.The header now uses proper title capitalization and is more descriptive.
114-114: Good catches on the typo fixes.Fixed "clearup" → "cleanup" (line 114) and "CTEATE" → "CREATE" (line 119).
Also applies to: 119-119
src/backend/utils/misc/ivy_guc.c (1)
186-205: GUC parameter is properly integrated into table creation logic.Verification confirms
default_with_rowidsis correctly used ininterpretRowidOption()(parse_clause.c:3915), which is called during CREATE TABLE statement processing (parse_utilcmd.c:265). The function returns the GUC value when no explicit ROWID option is provided, and users can override viaWITH (ROWID = true/false)in the CREATE TABLE statement. The hasrowid flag flows through to the table descriptor, controlling ROWID column creation. The default value offalseappropriately prevents unexpected behavioral changes.contrib/ivorysql_ora/preload_ora_misc.sql (1)
22-22: Remove unnecessary uuid-ossp extension creation from this preload file.The extension is loaded but not used. The ROWID and UROWID types are defined as simple composite types (rowoid OID, rowno bigint) and do not depend on
sys_guid()or other uuid-ossp functions. A codebase-wide search confirmssys_guid()is never called outside test files, and no Oracle compatibility code relies on this extension.
| relnamelist = list_make3(makeString(snamespace), | ||
| makeString(cxt.relation->relname), | ||
| makeString(sname)); | ||
| makeString(cxt.relation->relname), | ||
| makeString(sname)); | ||
|
|
||
| altseqstmt->options = list_make1(makeDefElem("owned_by", | ||
| (Node *) relnamelist, -1)); | ||
| (Node *) relnamelist, -1)); | ||
| cxt.alist = lappend(cxt.alist, altseqstmt); |
There was a problem hiding this comment.
Fix OWNED BY target for ROWID sequence (wrong third identifier).
You’re passing the sequence name as the "column" in OWNED BY. ALTER SEQUENCE ... OWNED BY expects table.column; using the sequence name will mis-associate ownership. Use "rowid" (or whatever token your ROWID dependency handler expects) as the third component.
Apply this diff in both places:
- relnamelist = list_make3(makeString(snamespace),
- makeString(cxt->relation->relname),
- makeString(sname));
+ relnamelist = list_make3(makeString(snamespace),
+ makeString(cxt->relation->relname),
+ makeString("rowid"));Also applies to: 3835-3841
|
No comments, proceed. |
|
It appears that the feature implemented supports only syntax:
But not following syntax ( which works on oracle): |
|
yes, Ivorysql does not support following syntax: CREATE TABLE t2 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/access/transam/xlog.c (1)
5576-5612: Harden control-file reads: handle partial reads, close FDs on error, avoid getenv("PGDATA").
- Partial-read bug: only checks len < 0; must verify len == sizeof(ControlFileData) or treat as error.
- FD leak on read error: fd not closed before returning -1.
- Using getenv("PGDATA") can be NULL and diverges from server’s configured DataDir; prefer DataDir or guard NULL.
- Use MAXPGPATH instead of a raw 1024.
Apply this diff to both functions:
@@ -int GetDatabaseStyleFromControl(char* path) +int GetDatabaseStyleFromControl(char *path) { - int fd; - int len; - char pathname[1024]; - char *configdir; + int fd; + int len; + char pathname[MAXPGPATH]; + char *configdir = NULL; ControlFileData confile; - if (path) - configdir = make_absolute_path(path); - else - configdir = make_absolute_path(getenv("PGDATA")); + if (path) + configdir = make_absolute_path(path); + else + configdir = make_absolute_path(DataDir); @@ - fd = open(pathname, O_RDONLY | PG_BINARY, 0); + fd = BasicOpenFile(pathname, O_RDONLY | PG_BINARY); if (fd < 0) { - fprintf(stderr, _("could not open file \"%s\" for reading: %s\n"), - pathname, strerror(errno)); - return -1; + ereport(LOG, + (errcode_for_file_access(), + errmsg("could not open file \"%s\" for reading: %m", pathname))); + if (configdir) pfree(configdir); + return -1; } - len = read(fd, &confile, sizeof(ControlFileData)); - if (len < 0) + len = read(fd, &confile, sizeof(ControlFileData)); + if (len != sizeof(ControlFileData)) { - fprintf(stderr, _("could not read file \"%s\": %s\n"), - pathname, strerror(errno)); - - return -1; + int save_errno = errno; + close(fd); + if (len < 0) + ereport(LOG, + (errcode_for_file_access(), + errmsg("could not read file \"%s\": %m", pathname))); + else + ereport(LOG, + (errmsg("could not read file \"%s\": read %d of %zu", + pathname, len, (Size) sizeof(ControlFileData)))); + if (configdir) pfree(configdir); + return -1; } - close(fd); + close(fd); + if (configdir) pfree(configdir); return confile.dbmode; } @@ -int GetCaseSwitchModeFromControl(char* path) +int GetCaseSwitchModeFromControl(char *path) { - int fd; - int len; - char pathname[1024]; - char *configdir; + int fd; + int len; + char pathname[MAXPGPATH]; + char *configdir = NULL; ControlFileData confile; - if (path) - configdir = make_absolute_path(path); - else - configdir = make_absolute_path(getenv("PGDATA")); + if (path) + configdir = make_absolute_path(path); + else + configdir = make_absolute_path(DataDir); @@ - fd = open(pathname, O_RDONLY | PG_BINARY, 0); + fd = BasicOpenFile(pathname, O_RDONLY | PG_BINARY); if (fd < 0) { - fprintf(stderr, _("could not open file \"%s\" for reading: %s\n"), - pathname, strerror(errno)); - - return -1; + ereport(LOG, + (errcode_for_file_access(), + errmsg("could not open file \"%s\" for reading: %m", pathname))); + if (configdir) pfree(configdir); + return -1; } - len = read(fd, &confile, sizeof(ControlFileData)); - if (len < 0) + len = read(fd, &confile, sizeof(ControlFileData)); + if (len != sizeof(ControlFileData)) { - fprintf(stderr, _("could not read file \"%s\": %s\n"), - pathname, strerror(errno)); - - return -1; + int save_errno = errno; + close(fd); + if (len < 0) + ereport(LOG, + (errcode_for_file_access(), + errmsg("could not read file \"%s\": %m", pathname))); + else + ereport(LOG, + (errmsg("could not read file \"%s\": read %d of %zu", + pathname, len, (Size) sizeof(ControlFileData)))); + if (configdir) pfree(configdir); + return -1; } - close(fd); + close(fd); + if (configdir) pfree(configdir); return confile.casemode; }Also applies to: 5618-5655
🧹 Nitpick comments (3)
src/port/identifier_alpha_transfor.c (1)
17-17: Consider renaming the file for consistency.While the comment corrections are good, the filename itself still contains "transfor" (identifier_alpha_transfor.c). For complete consistency, consider renaming to
identifier_alpha_transform.cin a future change. This would require updating build scripts and any includes.Also applies to: 29-29
src/backend/access/transam/xlog.c (2)
5572-5576: Nit: typo in block banner comment.“indentifier” → “identifier”.
Also applies to: 5690-5692
5660-5691: Make GUC assignment authoritative and handle read errors.
- Use PGC_INTERNAL + PGC_S_OVERRIDE (consistent with ivorysql.database_mode) so control-file values cannot be overridden unintentionally.
- If GetDatabaseStyleFromControl()/GetCaseSwitchModeFromControl() return -1, handle gracefully.
Apply:
void SetCaseGucOption(char* path) { int dbstyle; dbstyle = GetDatabaseStyleFromControl(path); - /* PG mode leaves identifier_case_switch unchanged. */ - if (DB_ORACLE == dbstyle) + if (dbstyle == -1) + return; /* log already emitted in helper */ + + /* PG mode leaves identifier_case_switch unchanged. */ + if (DB_ORACLE == dbstyle) { int casemode; casemode = GetCaseSwitchModeFromControl(path); + if (casemode < 0) + { + ereport(WARNING, + (errmsg("could not read case conversion mode from control file"))); + return; + } if (casemode == NORMAL) { - SetConfigOption("ivorysql.identifier_case_switch", "normal", - PGC_USERSET, PGC_S_DEFAULT); + SetConfigOption("ivorysql.identifier_case_switch", "normal", + PGC_INTERNAL, PGC_S_OVERRIDE); } else if (casemode == INTERCHANGE) { - SetConfigOption("ivorysql.identifier_case_switch", "interchange", - PGC_USERSET, PGC_S_DEFAULT); + SetConfigOption("ivorysql.identifier_case_switch", "interchange", + PGC_INTERNAL, PGC_S_OVERRIDE); } else if (casemode == LOWERCASE) { - SetConfigOption("ivorysql.identifier_case_switch", "lowercase", - PGC_USERSET, PGC_S_DEFAULT); + SetConfigOption("ivorysql.identifier_case_switch", "lowercase", + PGC_INTERNAL, PGC_S_OVERRIDE); } else ereport(FATAL, - (errmsg("Incorrect case conversion mode value \"%d\"", casemode))); + (errmsg("incorrect case conversion mode value \"%d\"", casemode))); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/oracle_test/regress/expected/case_conversion.outis excluded by!**/*.outsrc/oracle_test/regress/expected/case_conversion_1.outis excluded by!**/*.outsrc/oracle_test/regress/expected/case_conversion_2.outis excluded by!**/*.out
📒 Files selected for processing (8)
src/backend/access/transam/xlog.c(5 hunks)src/backend/catalog/namespace.c(1 hunks)src/backend/parser/scansup.c(1 hunks)src/backend/tcop/backend_startup.c(2 hunks)src/backend/utils/misc/ivy_guc.c(2 hunks)src/bin/initdb/initdb.c(2 hunks)src/oracle_test/regress/sql/case_conversion.sql(1 hunks)src/port/identifier_alpha_transfor.c(6 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/backend/tcop/backend_startup.c
- src/oracle_test/regress/sql/case_conversion.sql
- src/backend/catalog/namespace.c
- src/backend/parser/scansup.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/utils/misc/ivy_guc.c
⏰ 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: contrib_regression
- GitHub Check: build (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
🔇 Additional comments (5)
src/port/identifier_alpha_transfor.c (2)
21-22: LGTM! Documentation typo fixed.The correction from "transfor" to "transform" improves the clarity and correctness of the function descriptions.
31-31: LGTM! Consistent documentation improvements.All comment updates improve clarity and grammatical correctness throughout the file.
Also applies to: 43-43, 59-59, 69-69, 85-85
src/bin/initdb/initdb.c (2)
3323-3323: LGTM! Formatting improvement enhances readability.The empty line appropriately separates the
make_postgres()andmake_ivorysql()database creation calls, improving visual organization of the code.
3694-3694: LGTM! Documentation improvement fixes typo and enhances clarity.The updated comment corrects the typo ("transfor" → "transform") and uses clearer, more grammatically correct wording to describe the Oracle username case transformation logic.
src/backend/access/transam/xlog.c (1)
5590-5590: Nice: safer path building with snprintf.Avoids potential overflows compared to sprintf. LGTM.
Also applies to: 5631-5631
Sorry, I gave wrong answer. CREATE TABLE t3 |
Summary by CodeRabbit
New Features
Documentation
Behavior