refactor rowid codes#922
Conversation
WalkthroughAdds Oracle-compatible ROWID/UROWID composite types with implicit casts and installs uuid-ossp in preload_ora_misc.sql. Introduces a new boolean GUC ivorysql.default_with_rowids (USERSET, default false). Updates comments in heap.c, htup_details.h, tablefunc SQL, and Oracle test SQL. parse_utilcmd.c receives formatting-only edits. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
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 comments (6)
src/backend/catalog/heap.c (1)
231-233: LGTM! Documentation improvement.The enhanced comment provides better clarity about the Oracle ROWID pseudo-column's purpose and Oracle compatibility, which improves code maintainability.
src/backend/utils/misc/ivy_guc.c (1)
190-209: LGTM! Clear documentation for the new GUC parameter.The comment block and updated description effectively explain the purpose of
ivorysql.default_with_rowids. The grammatical improvements make the description more professional.src/oracle_test/regress/sql/ora_rowid.sql (2)
2-2: LGTM! Improved header comment.The updated header comment provides clearer terminology by using "Oracle-Compatible ROWID Pseudo-Column", which is more precise and consistent with the terminology used throughout the codebase.
114-114: LGTM! Typo corrections.Fixed spelling errors:
- "clearup" → "cleanup" (Line 114)
- "CTEATE TABLE" → "CREATE TABLE" (Line 119)
These corrections improve the test file's clarity and professionalism.
Also applies to: 119-119
contrib/ivorysql_ora/preload_ora_misc.sql (2)
9-17: LGTM! Well-structured ROWID/UROWID types.The composite type definitions and bidirectional implicit casts correctly implement Oracle-compatible ROWID functionality. The identical structure for both types with
WITH INOUTcasts is appropriate for Oracle compatibility where UROWID is a universal version of ROWID.
19-22: uuid-ossp addition appears unrelated to ROWID functionality.The grep search found no usage of uuid-ossp functions (uuid_generate_v1, uuid_generate_v5, sys_guid, etc.) in any ROWID-related code. The
CREATE EXTENSION IF NOT EXISTS "uuid-ossp"line in lines 19–22 appears logically disconnected from the Oracle compatibility casts for ROWID on lines 1–17.This extension should either be:
- Confirmed as required for Oracle compatibility (with clarification added to the PR description), or
- Moved to a separate commit/PR.
| 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 column in rowid sequence setup.
ALTER SEQUENCE schema.table_rowid_seq OWNED BY schema.table.table_rowid_seq will run because the third identifier comes from sname, i.e., the sequence name. The table has no column of that name, so the ALTER will error out and the table creation fails. The third element must be the ROWID column name (e.g., "rowid"), not the sequence name. Please adjust the relnamelist accordingly.
Apply this diff to correct the relnamelist:
- relnamelist = list_make3(makeString(snamespace),
- makeString(cxt->relation->relname),
- makeString(sname));
+ relnamelist = list_make3(makeString(snamespace),
+ makeString(cxt->relation->relname),
+ makeString("rowid"));📝 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.
| 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); | |
| relnamelist = list_make3(makeString(snamespace), | |
| makeString(cxt.relation->relname), | |
| makeString("rowid")); | |
| altseqstmt->options = list_make1(makeDefElem("owned_by", | |
| (Node *) relnamelist, -1)); | |
| cxt.alist = lappend(cxt.alist, altseqstmt); |
| relnamelist = list_make3(makeString(snamespace), | ||
| makeString(cxt.relation->relname), | ||
| makeString(sname)); | ||
| makeString(cxt.relation->relname), | ||
| makeString(sname)); | ||
|
|
||
| altseqstmt->options = list_make1(makeDefElem("owned_by", |
There was a problem hiding this comment.
Also fix OWNED BY clause in ALTER TABLE path.
The same issue occurs here: relnamelist uses the sequence name (sname) as the column identifier, so the generated ALTER SEQUENCE ... OWNED BY still targets a non-existent column. Update this block to pass the ROWID column name instead.
- relnamelist = list_make3(makeString(snamespace),
- makeString(cxt->relation->relname),
- makeString(sname));
+ relnamelist = list_make3(makeString(snamespace),
+ makeString(cxt->relation->relname),
+ makeString("rowid"));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/backend/parser/parse_utilcmd.c around lines 3834-3838, relnamelist is
currently built with makeString(sname) and the ALTER SEQUENCE OWNED BY option
also references the sequence name as the column; replace those uses with the
table's ROWID column name. Specifically, change the third element in relnamelist
from makeString(sname) to makeString(cxt.rowidcol) (or the exact field holding
the ROWID column name) and ensure the owned_by option is set to refer to that
same ROWID column name instead of sname so ALTER SEQUENCE ... OWNED BY targets
the correct column.
|
no comments, proceed. |
Summary by CodeRabbit
New Features
Documentation