feat(oracle_compat): add DBA_CONS_COLUMNS view for Oracle compatibility#1201
feat(oracle_compat): add DBA_CONS_COLUMNS view for Oracle compatibility#1201xiaoyu509 wants to merge 13 commits intoIvorySQL:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a new Oracle-compatible system view Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@xiaoyu509 Hi xiaoyu, thanks for your work on IvorySQL! |
|
@NotHimmel 已清理误提交的文件，并添加了测试用例，请重新审查。 |
感谢补充测试用例并重新提交。还有几个问题需要你再修改一下：
麻烦你按这几点调整后再重新提交，谢谢！ |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/bin/psql/psqlplusparse.c (3)
2018-2039: Consider parameterized query forget_guc_settings.While
gucnameis currently only passed known internal GUC names ("nls_length_semantics"), using string interpolation for SQL is a risky pattern. Consider usingPQexecParamsfor better safety:Using parameterized query
static char * get_guc_settings(const char *gucname) { - PQExpBuffer query = createPQExpBuffer(); PGresult *res; char *val = NULL; + const char *paramValues[1] = { gucname }; - appendPQExpBuffer(query, "SELECT setting from pg_settings where name = '%s';", gucname); - res = PQexec(pset.db, query->data); + res = PQexecParams(pset.db, + "SELECT setting FROM pg_settings WHERE name = $1", + 1, NULL, paramValues, NULL, NULL, 0); if (PQresultStatus(res) == PGRES_TUPLES_OK && PQntuples(res) == 1 && !PQgetisnull(res, 0, 0)) { val = pg_strdup(PQgetvalue(res, 0, 0)); } PQclear(res); - destroyPQExpBuffer(query); return val; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/psql/psqlplusparse.c` around lines 2018 - 2039, The get_guc_settings function constructs SQL with string interpolation which risks SQL injection; replace the appendPQExpBuffer + PQexec call with a parameterized call using PQexecParams (or PQprepare/PQexecPrepared) to pass gucname as a text parameter, i.e. build the query "SELECT setting FROM pg_settings WHERE name = $1", call PQexecParams with paramTypes/textFormats set appropriately, handle the returned PGresult the same way (check PQresultStatus, PQntuples, PQgetisnull), and ensure PQclear and destroyPQExpBuffer are still called; update variable names in the function (get_guc_settings) accordingly and remove direct concatenation of gucname into the SQL string.
2063-2084: Potential memory leak inmerge_print_list.After copying item pointers from
list2tolist1, the function returns without freeinglist2->itemsorlist2itself. This leaves thelist2structure and its items array orphaned.If this is intentional (e.g., both lists share ownership of items), add a comment clarifying the ownership model. Otherwise, consider freeing the orphaned structures:
Proposed fix to free orphaned list2 structures
static print_list * merge_print_list(print_list *list1, print_list *list2) { int newlen; int i; if (list1 == NULL) return list2; if (list2 == NULL) return list1; newlen = list1->length + list2->length; list1->items = (print_item **) pg_realloc(list1->items, sizeof(print_item *) * newlen); for (i = 0; i < list2->length; i++) { list1->items[list1->length] = list2->items[i]; list1->length++; } + /* Free list2 structure (items are now owned by list1) */ + pg_free(list2->items); + pg_free(list2); + return list1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/psql/psqlplusparse.c` around lines 2063 - 2084, The merge_print_list function currently appends pointers from print_list *list2 into list1 but never releases list2 or its items array, causing a memory leak; either document the ownership model with a clear comment in merge_print_list indicating that ownership of individual print_item pointers transfers to list1 and list2 and its items array must not be freed by callers, or free the orphaned structures after copying by freeing list2->items and then freeing list2 itself (while leaving the pointed-to print_item objects intact since ownership moved to list1); update or add the comment near merge_print_list and implement the free(list2->items) and free(list2) path if you choose to take ownership in merge_print_list.
2092-2096: Silent error handling is intentional but consider logging.The
psqlplus_yyerrorfunction silently discards parse errors. While the comment indicates this is intentional (the parser runs "quietly"), consider whether debug-level logging would be helpful for troubleshooting parse failures during development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/psql/psqlplusparse.c` around lines 2092 - 2096, The psqlplus_yyerror function currently swallows parse errors; update psqlplus_yyerror(yyscan_t yyscanner, const char *message) to emit a debug-level log containing the incoming message (and, if available from yyscanner, location or token/context info) instead of doing nothing—use the project's logging facility (or fprintf to stderr if none exists) to log a clear message prefixed with "psqlplus parse error" and include the message and scanner-derived details to aid troubleshooting while preserving the "quiet" behavior at higher log levels.contrib/ivorysql_ora/expected/test_dba_cons_columns.out (1)
6-8: Verify header divider alignment.Line 7 shows
------+-----------------+------------+-------------+----------but this appears to have inconsistent dash counts relative to the column headers on Line 6. Ensure the expected output file was generated from an actual test run with proper formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/ivorysql_ora/expected/test_dba_cons_columns.out` around lines 6 - 8, The header divider row under the column headers ("owner | constraint_name | table_name | column_name | position") is misaligned; regenerate the expected output from a real test run and replace the divider line so the number of dashes for each column matches the width of its header (ensuring separators " | " line up with "owner", "constraint_name", "table_name", "column_name", and "position"), updating the line currently showing "------+-----------------+------------+-------------+----------" in the expected output to the correctly formatted divider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/ivorysql_ora/expected/test_dba_cons_columns.out`:
- Around line 1-4: Update the expected test output so the column header matches
the SQL alias: change the header from "?column?" to "view_exists" in
contrib/ivorysql_ora/expected/test_dba_cons_columns.out to reflect the query
SELECT count(*) > 0 AS view_exists; ensure the spacing/underline width matches
the existing formatting so the file shows "view_exists" then the row with "t"
and the "(1 row)" footer.
In `@contrib/ivorysql_ora/sql/test_dba_cons_columns.sql`:
- Around line 1-7: Translate the two Chinese comments to English (the comment
before the first SELECT and the one before the second SELECT) in
contrib/ivorysql_ora/sql/test_dba_cons_columns.sql and expand the file to
include the three missing test cases mentioned in the PR: add a test that
creates an empty table and verifies sys.dba_cons_columns shows no constraints
for it, add a test that creates a table with multiple constraints (PRIMARY KEY,
UNIQUE, FOREIGN KEY) and asserts sys.dba_cons_columns returns the expected rows
for each constraint, and add a cross-user/schema test that creates a table in a
different schema/user and verifies visibility from sys.dba_cons_columns; keep
the existing view-existence check (first SELECT) and simple SELECT (second
SELECT) but update their comments to English to match project conventions.
In `@src/fe_utils/ora_kwlist_d.h`:
- Around line 1-1324: This PR accidentally includes generated parser/keyword
changes (the OraScanKeywords definitions such as OraScanKeywords_kw_string,
OraScanKeywords_kw_offsets, ORASCANKEYWORDS_NUM_KEYWORDS,
OraScanKeywords_hash_func and the const ScanKeywordList OraScanKeywords); remove
this file from the DBA_CONS_COLUMNS change set by reverting the changes to
src/fe_utils/ora_kwlist_d.h (or restore the file to the state on main) so the PR
only contains the sysview logic/tests, and move the regenerated keyword header
into a separate, dedicated parser/keyword PR for review.
---
Nitpick comments:
In `@contrib/ivorysql_ora/expected/test_dba_cons_columns.out`:
- Around line 6-8: The header divider row under the column headers ("owner |
constraint_name | table_name | column_name | position") is misaligned;
regenerate the expected output from a real test run and replace the divider line
so the number of dashes for each column matches the width of its header
(ensuring separators " | " line up with "owner", "constraint_name",
"table_name", "column_name", and "position"), updating the line currently
showing "------+-----------------+------------+-------------+----------" in the
expected output to the correctly formatted divider.
In `@src/bin/psql/psqlplusparse.c`:
- Around line 2018-2039: The get_guc_settings function constructs SQL with
string interpolation which risks SQL injection; replace the appendPQExpBuffer +
PQexec call with a parameterized call using PQexecParams (or
PQprepare/PQexecPrepared) to pass gucname as a text parameter, i.e. build the
query "SELECT setting FROM pg_settings WHERE name = $1", call PQexecParams with
paramTypes/textFormats set appropriately, handle the returned PGresult the same
way (check PQresultStatus, PQntuples, PQgetisnull), and ensure PQclear and
destroyPQExpBuffer are still called; update variable names in the function
(get_guc_settings) accordingly and remove direct concatenation of gucname into
the SQL string.
- Around line 2063-2084: The merge_print_list function currently appends
pointers from print_list *list2 into list1 but never releases list2 or its items
array, causing a memory leak; either document the ownership model with a clear
comment in merge_print_list indicating that ownership of individual print_item
pointers transfers to list1 and list2 and its items array must not be freed by
callers, or free the orphaned structures after copying by freeing list2->items
and then freeing list2 itself (while leaving the pointed-to print_item objects
intact since ownership moved to list1); update or add the comment near
merge_print_list and implement the free(list2->items) and free(list2) path if
you choose to take ownership in merge_print_list.
- Around line 2092-2096: The psqlplus_yyerror function currently swallows parse
errors; update psqlplus_yyerror(yyscan_t yyscanner, const char *message) to emit
a debug-level log containing the incoming message (and, if available from
yyscanner, location or token/context info) instead of doing nothing—use the
project's logging facility (or fprintf to stderr if none exists) to log a clear
message prefixed with "psqlplus parse error" and include the message and
scanner-derived details to aid troubleshooting while preserving the "quiet"
behavior at higher log levels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eca953b6-fe4e-432e-8df6-eaf1fcaca2f8
📒 Files selected for processing (12)
contrib/ivorysql_ora/expected/test_dba_cons_columns.outcontrib/ivorysql_ora/ivorysql_ora--1.0.sqlcontrib/ivorysql_ora/sql/test_dba_cons_columns.sqlsrc/bin/psql/ora_psqlscanslash.csrc/bin/psql/psqlplusparse.csrc/bin/psql/psqlplusparse.hsrc/bin/psql/psqlplusscan.csrc/fe_utils/ora_keywords.csrc/fe_utils/ora_kwlist_d.hsrc/include/catalog/postgres_oracle.bkisrc/oracle_fe_utils/ora_psqlscan.csrc/test/regress/ora_pg_regress
| ?column? | ||
| ---------- | ||
| t | ||
| (1 row) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the SQL file alias produces the expected column name
cat contrib/ivorysql_ora/sql/test_dba_cons_columns.sql
echo "---"
cat contrib/ivorysql_ora/expected/test_dba_cons_columns.outRepository: IvorySQL/IvorySQL
Length of output: 461
Update expected output to match SQL alias.
The SQL file contains SELECT count(*) > 0 AS view_exists with an explicit alias, but the expected output shows ?column? as the column header. PostgreSQL should display the aliased column name view_exists.
Update the expected output to:
view_exists
-------------
t
(1 row)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/ivorysql_ora/expected/test_dba_cons_columns.out` around lines 1 - 4,
Update the expected test output so the column header matches the SQL alias:
change the header from "?column?" to "view_exists" in
contrib/ivorysql_ora/expected/test_dba_cons_columns.out to reflect the query
SELECT count(*) > 0 AS view_exists; ensure the spacing/underline width matches
the existing formatting so the file shows "view_exists" then the row with "t"
and the "(1 row)" footer.
| -- 测试 DBA_CONS_COLUMNS 视图是否存在 | ||
| SELECT count(*) > 0 AS view_exists | ||
| FROM information_schema.views | ||
| WHERE table_schema = 'sys' AND table_name = 'dba_cons_columns'; | ||
|
|
||
| -- 测试查询是否能返回数据（至少不报错） | ||
| SELECT * FROM sys.dba_cons_columns LIMIT 1; |
There was a problem hiding this comment.
Translate comments to English per project conventions.
The reviewer explicitly requested that comments be in English to match project conventions. Lines 1 and 6 contain Chinese comments that should be translated.
Additionally, the PR description claims "three dedicated test cases covering empty tables, multiple constraints, and cross-user queries," but this file only contains 2 basic tests (view existence check and a simple SELECT). Consider adding tests that actually exercise:
- Empty table scenarios
- Multiple constraints on a table
- Cross-user/schema constraint visibility
Proposed translation of comments
--- 测试 DBA_CONS_COLUMNS 视图是否存在
+-- Test whether the DBA_CONS_COLUMNS view exists
SELECT count(*) > 0 AS view_exists
FROM information_schema.views
WHERE table_schema = 'sys' AND table_name = 'dba_cons_columns';
--- 测试查询是否能返回数据（至少不报错）
+-- Test that the query executes without error
SELECT * FROM sys.dba_cons_columns LIMIT 1;📝 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.
| -- 测试 DBA_CONS_COLUMNS 视图是否存在 | |
| SELECT count(*) > 0 AS view_exists | |
| FROM information_schema.views | |
| WHERE table_schema = 'sys' AND table_name = 'dba_cons_columns'; | |
| -- 测试查询是否能返回数据（至少不报错） | |
| SELECT * FROM sys.dba_cons_columns LIMIT 1; | |
| -- Test whether the DBA_CONS_COLUMNS view exists | |
| SELECT count(*) > 0 AS view_exists | |
| FROM information_schema.views | |
| WHERE table_schema = 'sys' AND table_name = 'dba_cons_columns'; | |
| -- Test that the query executes without error | |
| SELECT * FROM sys.dba_cons_columns LIMIT 1; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/ivorysql_ora/sql/test_dba_cons_columns.sql` around lines 1 - 7,
Translate the two Chinese comments to English (the comment before the first
SELECT and the one before the second SELECT) in
contrib/ivorysql_ora/sql/test_dba_cons_columns.sql and expand the file to
include the three missing test cases mentioned in the PR: add a test that
creates an empty table and verifies sys.dba_cons_columns shows no constraints
for it, add a test that creates a table with multiple constraints (PRIMARY KEY,
UNIQUE, FOREIGN KEY) and asserts sys.dba_cons_columns returns the expected rows
for each constraint, and add a cross-user/schema test that creates a table in a
different schema/user and verifies visibility from sys.dba_cons_columns; keep
the existing view-existence check (first SELECT) and simple SELECT (second
SELECT) but update their comments to English to match project conventions.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/ivorysql_ora/src/sysview/sysview_procedures.sql`:
- Line 284: The OWNER column currently calls pg_get_userbyid(n.oid) which passes
the namespace OID to a user lookup and yields wrong names; replace this with the
appropriate namespace-derived value by using n.nspname for Oracle-compatible
schema name or n.nspowner if you intend to show the schema's owning role (i.e.,
change the expression that references pg_get_userbyid(n.oid) to either
n.nspname::name or pg_get_userbyid(n.nspowner)::name depending on whether OWNER
should be the schema name or the schema owner).
- Around line 281-295: Rename the view to uppercase SYS.DBA_CONS_COLUMNS and
update its column expressions to follow file conventions: wrap identifier
expressions (pg_get_userbyid(n.oid), c.conname, cl.relname, a.attname) with
SYS.ORA_CASE_TRANS() and cast them to Oracle types VARCHAR2(128) instead of
::name, and cast the position (row_number() ...) to NUMBER instead of ::int; add
the pg_toast exclusion (n.nspname <> 'pg_toast') alongside existing namespace
filters; and add the matching grant statement GRANT SELECT ON
SYS.DBA_CONS_COLUMNS TO PUBLIC (or adjust per DBA-only access policy) so the
view follows the same naming, typing, case-wrapping, namespace filtering, and
GRANT patterns used by other views in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a4f3a15-e23a-4727-962d-f2e16cd60828
📒 Files selected for processing (1)
contrib/ivorysql_ora/src/sysview/sysview_procedures.sql
| -- Create DBA_CONS_COLUMNS view for Oracle compatibility | ||
| CREATE OR REPLACE VIEW sys.dba_cons_columns AS | ||
| SELECT | ||
| pg_get_userbyid(n.oid)::name AS owner, | ||
| c.conname::name AS constraint_name, | ||
| cl.relname::name AS table_name, | ||
| a.attname::name AS column_name, | ||
| (row_number() OVER (PARTITION BY c.oid ORDER BY a.attnum))::int AS position | ||
| FROM pg_constraint c | ||
| JOIN pg_class cl ON c.conrelid = cl.oid | ||
| JOIN pg_namespace n ON cl.relnamespace = n.oid | ||
| JOIN pg_attribute a ON a.attrelid = cl.oid AND a.attnum = ANY(c.conkey) | ||
| WHERE cl.relkind = 'r' | ||
| AND a.attisdropped = false | ||
| AND n.nspname NOT IN ('pg_catalog', 'information_schema'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Multiple consistency issues with existing sysview conventions.
The new view deviates from established patterns in this file:
- Naming: Uses lowercase
sys.dba_cons_columnswhile all other views use uppercaseSYS.DBA_PROCEDURES, etc. - Missing GRANT: All other views include
GRANT SELECT ON SYS.<view_name> TO PUBLIC; - Missing Oracle types: Should use
VARCHAR2(128)/NUMBERinstead of::name/::int - Missing
SYS.ORA_CASE_TRANS(): Existing views wrap identifiers for Oracle case compatibility - Missing pg_toast filter: Other views exclude
PG_TOASTnamespace
Proposed fix aligning with existing conventions
--- Create DBA_CONS_COLUMNS view for Oracle compatibility
-CREATE OR REPLACE VIEW sys.dba_cons_columns AS
-SELECT
- pg_get_userbyid(n.oid)::name AS owner,
- c.conname::name AS constraint_name,
- cl.relname::name AS table_name,
- a.attname::name AS column_name,
- (row_number() OVER (PARTITION BY c.oid ORDER BY a.attnum))::int AS position
-FROM pg_constraint c
-JOIN pg_class cl ON c.conrelid = cl.oid
-JOIN pg_namespace n ON cl.relnamespace = n.oid
-JOIN pg_attribute a ON a.attrelid = cl.oid AND a.attnum = ANY(c.conkey)
-WHERE cl.relkind = 'r'
- AND a.attisdropped = false
- AND n.nspname NOT IN ('pg_catalog', 'information_schema');
+/* DBA_CONS_COLUMNS */
+CREATE OR REPLACE VIEW SYS.DBA_CONS_COLUMNS AS
+SELECT
+ SYS.ORA_CASE_TRANS(N.NSPNAME::VARCHAR2)::VARCHAR2(128) AS OWNER,
+ SYS.ORA_CASE_TRANS(C.CONNAME::VARCHAR2)::VARCHAR2(128) AS CONSTRAINT_NAME,
+ SYS.ORA_CASE_TRANS(CL.RELNAME::VARCHAR2)::VARCHAR2(128) AS TABLE_NAME,
+ SYS.ORA_CASE_TRANS(A.ATTNAME::VARCHAR2)::VARCHAR2(128) AS COLUMN_NAME,
+ (ROW_NUMBER() OVER (PARTITION BY C.OID ORDER BY A.ATTNUM))::NUMBER AS POSITION
+FROM PG_CONSTRAINT C
+JOIN PG_CLASS CL ON C.CONRELID = CL.OID
+JOIN PG_NAMESPACE N ON CL.RELNAMESPACE = N.OID
+JOIN PG_ATTRIBUTE A ON A.ATTRELID = CL.OID AND A.ATTNUM = ANY(C.CONKEY)
+WHERE CL.RELKIND = 'r'
+ AND A.ATTISDROPPED = FALSE
+ AND N.NSPNAME != 'PG_CATALOG'::REGNAMESPACE::OID
+ AND N.NSPNAME != 'PG_TOAST'::REGNAMESPACE::OID
+ AND N.NSPNAME != 'INFORMATION_SCHEMA'::REGNAMESPACE::OID
+;
+
+/* GRANT SELECT PRIVILEGE TO PUBLIC */
+GRANT SELECT ON SYS.DBA_CONS_COLUMNS TO PUBLIC;Note: The PR objectives state DBA views should be "visible only to users with DBA privileges." If that's the intended access model, you may need to restrict the GRANT or use a different privilege mechanism rather than granting to PUBLIC.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/ivorysql_ora/src/sysview/sysview_procedures.sql` around lines 281 -
295, Rename the view to uppercase SYS.DBA_CONS_COLUMNS and update its column
expressions to follow file conventions: wrap identifier expressions
(pg_get_userbyid(n.oid), c.conname, cl.relname, a.attname) with
SYS.ORA_CASE_TRANS() and cast them to Oracle types VARCHAR2(128) instead of
::name, and cast the position (row_number() ...) to NUMBER instead of ::int; add
the pg_toast exclusion (n.nspname <> 'pg_toast') alongside existing namespace
filters; and add the matching grant statement GRANT SELECT ON
SYS.DBA_CONS_COLUMNS TO PUBLIC (or adjust per DBA-only access policy) so the
view follows the same naming, typing, case-wrapping, namespace filtering, and
GRANT patterns used by other views in this file.
|
已删除所有自动生成文件及编译目录，添加了正确的视图实现代码，并确保所有新增注释为英文。请重新审查。 |
还有编译的中间文件未清除，只提交你修改过的文件，其余都不要提交到你的PR中，谢谢。 |
|
@NotHimmel 已按指示清理完毕，现在 PR 仅包含视图实现代码和测试用例。请重新审查。 |
|
Hi, Xiaoyu, I'm taking over the review of this PR. My first comments is, could you put the code into a new file sysview_constrains.sql ? This way, it can be consistent with other implementations. |
| GRANT SELECT ON SYS.USER_PROCEDURES TO PUBLIC; | ||
| /* End - ReqID:SRS-SQL-SYSVIEW */ | ||
| -- Create DBA_CONS_COLUMNS view for Oracle compatibility | ||
| CREATE OR REPLACE VIEW sys.dba_cons_columns AS |
There was a problem hiding this comment.
-
Oracle query results are uppercase by default, whereas IvorySQL uses lowercase by default. To maintain consistency with Oracle, IvorySQL provides the case-conversion function ORA_CASE_TRANS. It is recommended to use this function for automatic case conversion to match Oracle's behavior.
-
Should a NOT NULL constraint be added as well? This constraint is recorded in pg_attribute and indicated by the attnotnull flag.
There was a problem hiding this comment.
- yes, agree that ORA_CASE_TRANS should be used.
- also agree that NOT NULL constraint should be added as well.
|
It's recommended to squash the commits into a single one for a cleaner and more concise history. |
|
One more comment: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/ivorysql_ora/sql/ora_sysview.sql`:
- Around line 95-99: Expand the test for sys.dba_cons_columns to verify the
feature contract instead of just existence: create a non-DBA test user and a DBA
user, create a table with constraints under the non-DBA user, then run queries
against DBA_CONS_COLUMNS as both DBA and non-DBA to assert DBA-only visibility
(DBA sees the constraint rows, non-DBA does not); also validate the returned
column set and types (e.g., owner, constraint_name, table_name, column_name,
position) and specific expected values for the created constraint to ensure
content correctness and cross-user behavior rather than only confirming SELECT
succeeds.
- Line 99: The SELECT against sys.dba_cons_columns is nondeterministic; update
the query in the ora_sysview.sql test so results are ordered before applying
LIMIT. Specifically, modify the SELECT * FROM sys.dba_cons_columns LIMIT 5;
statement to include an ORDER BY on stable columns (for example owner,
constraint_name, and column_name or similar deterministic keys present in
sys.dba_cons_columns) so the first 5 rows are reproducible across runs.
🪄 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: fb938c65-6cda-4c9a-a5b8-95a228a95ed7
📒 Files selected for processing (1)
contrib/ivorysql_ora/sql/ora_sysview.sql
|
@bigplaice @scmysxb-git
I have also cleaned up the commits, but I am not able to squash them myself. Could you please help squash them? Thank you! Please review again. |
OK, we will squash merge them. |
|
issue #1200 |
c3dc4d0 to
2fcca30
Compare
Fixes #1200
功能实现
DBA_CONS_COLUMNS系统视图，实现全局约束列的查询能力；pg_constraint/pg_attribute/pg_class系统表关联实现，与 Oracle 官方逻辑对齐；ALL_CONS_COLUMNS/USER_CONS_COLUMNS视图逻辑统一；测试验证
make check-world全量回归测试，无新增报错；make oracle-check-worldOracle 兼容测试，视图查询结果符合预期；关联信息
Summary by CodeRabbit
New Features
sys.dba_cons_columnsOracle-compatible system view, enabling users to query constraint column information including owner, constraint name, table name, column name, and nullable status.Tests