create minimal SYS.V$SESSION view#1185
create minimal SYS.V$SESSION view#1185pierreforstmann wants to merge 9 commits intoIvorySQL:masterIvorySQL/IvorySQL:masterfrom pierreforstmann:patch_dyn_viewspierreforstmann/IvorySQL:patch_dyn_viewsCopy head branch name to clipboard
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:
📝 WalkthroughWalkthroughAdds three new public SYS dynamic views: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
🤖 Fix all issues with AI agents
In `@contrib/ivorysql_ora/src/sysview/sysview--1.0.sql`:
- Around line 1201-1214: The SYS.V$SESSION view is missing a post-create GRANT
and uses raw PG types; update the view definition for SYS.V$SESSION to cast
columns to Oracle-compatible types (e.g., USENAME::VARCHAR2(128) as USERNAME,
CLIENT_ADDR::VARCHAR2(64) or coalesce(hostname, CLIENT_ADDR::text)::VARCHAR2(64)
as MACHINE, CLIENT_PORT::NUMBER as PORT, QUERY::VARCHAR2(4000) as SQL_TEXT,
STATE/WAIT_EVENT/XACT_START/ BACKEND_START cast to appropriate
VARCHAR2/DATE/NUMBER types) and then add a GRANT SELECT ON SYS.V$SESSION TO
PUBLIC immediately after the CREATE OR REPLACE VIEW statement so non-superusers
can query it.
- Around line 1194-1195: Remove the duplicate grant statement: locate the
redundant SQL line "GRANT SELECT ON SYS.USER_VIEWS TO PUBLIC;" (the second
occurrence around the end of the file) and delete it so only the single original
grant remains; ensure no other identical GRANT statements are introduced
elsewhere in the file.
🧹 Nitpick comments (1)
contrib/ivorysql_ora/src/sysview/sysview--1.0.sql (1)
1205-1208: Column semantics diverge from OracleV$SESSION.A few mappings worth noting for Oracle compatibility:
- STATUS (
STATE): PostgreSQL returns values like'active','idle','idle in transaction', whereas OracleV$SESSION.STATUSuses'ACTIVE','INACTIVE','KILLED', etc. Consider usingUPPER()at minimum, or aCASEexpression to map PG states to Oracle equivalents.- TYPE (
BACKEND_TYPE): Oracle uses'USER'/'BACKGROUND'. PostgreSQL returns'client backend','autovacuum launcher', etc. ACASEmapping (e.g.,'client backend'→'USER', else'BACKGROUND') would improve compatibility.- MACHINE (
CLIENT_ADDR): Oracle provides the client hostname; PG provides the IP. This may be acceptable as a "minimal" view, but worth documenting the difference.
|
Thank you again for your contribution to IvorySQL. We willl review your codes soon. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@contrib/ivorysql_ora/src/sysview/sysview--1.0.sql`:
- Around line 1240-1241: Change the DEFAULT_VALUE column mapping from RESET_VAL
to BOOT_VAL: replace the reference mapping RESET_VAL::VARCHAR2(255) AS
DEFAULT_VALUE with BOOT_VAL::VARCHAR2(255) AS DEFAULT_VALUE so DEFAULT_VALUE
reflects the compiled-in bootstrap default (pg_settings.boot_val) rather than
the reset value; update any related aliases or comments that reference RESET_VAL
to avoid confusion with RESET_VAL/boot_val semantics.
- Around line 1224-1226: The BACKGROUND CASE expression returns an integer -1
and uses redundant WHENs; change it to produce a VARCHAR(1) matching Oracle
V$PROCESS.BACKGROUND semantics: return '1' for background processes and NULL
otherwise. Update the CASE that references BACKEND_TYPE (the current CASE
producing BACKGROUND) to use CASE WHEN BACKEND_TYPE = 'client backend' THEN NULL
ELSE '1' END AS BACKGROUND so the type and values match Oracle's
V$PROCESS.BACKGROUND and remove the redundant WHEN BACKEND_TYPE <> 'client
backend' branch.
- Around line 1247-1251: The CASE currently producing IMMEDIATE/DEFERRED/FALSE
is being assigned to AS ISSES_MODIFIABLE but those values actually correspond to
ISSYS_MODIFIABLE; update the SELECT so that AS ISSES_MODIFIABLE returns TRUE or
FALSE (e.g., CONTEXT IN ('user','superuser') or CONTEXT = 'postmaster' =>
appropriate TRUE/FALSE) and add a separate expression AS ISSYS_MODIFIABLE that
preserves the current IMMEDIATE/DEFERRED/FALSE mapping (or alternatively rename
the existing AS ISSES_MODIFIABLE to AS ISSYS_MODIFIABLE if you prefer a single
column); adjust the CASE(s) around the same CASE ... END expression to use the
exact identifiers ISSES_MODIFIABLE and ISSYS_MODIFIABLE.
🧹 Nitpick comments (1)
contrib/ivorysql_ora/src/sysview/sysview--1.0.sql (1)
1235-1239:TYPEcolumn missing::NUMBERcast.All other numeric columns in the file use
::NUMBER. Add a cast for consistency.Proposed fix
CASE WHEN VARTYPE = 'bool' THEN 1 WHEN VARTYPE = 'integer' THEN 3 ELSE 2 - END AS TYPE, + END::NUMBER AS TYPE,
There was a problem hiding this comment.
ISSES_MODIFIABLE returns ISSYS_MODIFIABLE values (IMMEDIATE/DEFERRED/FALSE).
Oracle's ISSES_MODIFIABLE is TRUE/FALSE (whether ALTER SESSION can change it). The values IMMEDIATE/DEFERRED/FALSE correspond to Oracle's ISSYS_MODIFIABLE column instead. Either rename the column to ISSYS_MODIFIABLE (and optionally add a separate ISSES_MODIFIABLE), or change the values to TRUE/FALSE.
Proposed fix — provide both columns
+ CASE
+ WHEN CONTEXT IN ('user', 'superuser') THEN 'TRUE'
+ ELSE 'FALSE'
+ END::VARCHAR2(5) AS ISSES_MODIFIABLE,
CASE
WHEN CONTEXT IN ('user', 'superuser') THEN 'IMMEDIATE'
WHEN CONTEXT = 'postmaster' THEN 'FALSE'
ELSE 'DEFERRED'
- END AS ISSES_MODIFIABLE,
+ END::VARCHAR2(9) AS ISSYS_MODIFIABLE,📝 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.
| CASE | |
| WHEN CONTEXT IN ('user', 'superuser') THEN 'IMMEDIATE' | |
| WHEN CONTEXT = 'postmaster' THEN 'FALSE' | |
| ELSE 'DEFERRED' | |
| END AS ISSES_MODIFIABLE, | |
| CASE | |
| WHEN CONTEXT IN ('user', 'superuser') THEN 'TRUE' | |
| ELSE 'FALSE' | |
| END::VARCHAR2(5) AS ISSES_MODIFIABLE, | |
| CASE | |
| WHEN CONTEXT IN ('user', 'superuser') THEN 'IMMEDIATE' | |
| WHEN CONTEXT = 'postmaster' THEN 'FALSE' | |
| ELSE 'DEFERRED' | |
| END::VARCHAR2(9) AS ISSYS_MODIFIABLE, |
🤖 Prompt for AI Agents
In `@contrib/ivorysql_ora/src/sysview/sysview--1.0.sql` around lines 1247 - 1251,
The CASE currently producing IMMEDIATE/DEFERRED/FALSE is being assigned to AS
ISSES_MODIFIABLE but those values actually correspond to ISSYS_MODIFIABLE;
update the SELECT so that AS ISSES_MODIFIABLE returns TRUE or FALSE (e.g.,
CONTEXT IN ('user','superuser') or CONTEXT = 'postmaster' => appropriate
TRUE/FALSE) and add a separate expression AS ISSYS_MODIFIABLE that preserves the
current IMMEDIATE/DEFERRED/FALSE mapping (or alternatively rename the existing
AS ISSES_MODIFIABLE to AS ISSYS_MODIFIABLE if you prefer a single column);
adjust the CASE(s) around the same CASE ... END expression to use the exact
identifiers ISSES_MODIFIABLE and ISSYS_MODIFIABLE.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@contrib/ivorysql_ora/src/sysview/sysview--1.0.sql`:
- Around line 1235-1246: The CASE expressions producing the TYPE and ISDEFAULT
columns return raw PostgreSQL types; update them to include explicit
Oracle-compatible casts: cast the numeric result of the TYPE CASE to an Oracle
NUMBER (e.g., NUMBER(1)) and cast the string result of the ISDEFAULT CASE to
VARCHAR2(5) (or appropriate VARCHAR2 length). Locate the CASE that checks
VARTYPE (the alias TYPE) and the CASE that compares setting = boot_val (the
alias ISDEFAULT) and append the corresponding ::NUMBER(...) and ::VARCHAR2(...)
style casts (or use TO_NUMBER/TO_CHAR as your style across the file) so they
match the explicit Oracle type casting used for other columns like
SETTING::VARCHAR2(80) and BOOT_VAL::VARCHAR2(255).
- Around line 1247-1251: The CREATE VIEW contains a duplicate, orphaned "END AS
ISSES_MODIFIABLE" after the completed CASE expression; remove the stray line so
the CASE ... END::VARCHAR2(5) AS ISSES_MODIFIABLE stays as the single, valid
expression (look for the CASE that sets ISSES_MODIFIABLE and delete the extra
trailing "END AS ISSES_MODIFIABLE").
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing Oracle-compatible type casts on TYPE and ISDEFAULT columns.
Every other column in this file is explicitly cast to an Oracle type. These two CASE expressions return raw PostgreSQL types (integer and text), breaking consistency.
Proposed fix
CASE
WHEN VARTYPE = 'bool' THEN 1
WHEN VARTYPE = 'integer' THEN 3
ELSE 2
- END AS TYPE,
+ END::NUMBER AS TYPE,
SETTING::VARCHAR2(80) AS VALUE,
BOOT_VAL::VARCHAR2(255) AS DEFAULT_VALUE,
-- Mapping update levels
CASE
WHEN setting = boot_val THEN 'TRUE'
ELSE 'FALSE'
- END AS ISDEFAULT,
+ END::VARCHAR2(5) AS ISDEFAULT,📝 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.
| CASE | |
| WHEN VARTYPE = 'bool' THEN 1 | |
| WHEN VARTYPE = 'integer' THEN 3 | |
| ELSE 2 | |
| END AS TYPE, | |
| SETTING::VARCHAR2(80) AS VALUE, | |
| BOOT_VAL::VARCHAR2(255) AS DEFAULT_VALUE, | |
| -- Mapping update levels | |
| CASE | |
| WHEN setting = boot_val THEN 'TRUE' | |
| ELSE 'FALSE' | |
| END AS ISDEFAULT, | |
| CASE | |
| WHEN VARTYPE = 'bool' THEN 1 | |
| WHEN VARTYPE = 'integer' THEN 3 | |
| ELSE 2 | |
| END::NUMBER AS TYPE, | |
| SETTING::VARCHAR2(80) AS VALUE, | |
| BOOT_VAL::VARCHAR2(255) AS DEFAULT_VALUE, | |
| -- Mapping update levels | |
| CASE | |
| WHEN setting = boot_val THEN 'TRUE' | |
| ELSE 'FALSE' | |
| END::VARCHAR2(5) AS ISDEFAULT, |
🤖 Prompt for AI Agents
In `@contrib/ivorysql_ora/src/sysview/sysview--1.0.sql` around lines 1235 - 1246,
The CASE expressions producing the TYPE and ISDEFAULT columns return raw
PostgreSQL types; update them to include explicit Oracle-compatible casts: cast
the numeric result of the TYPE CASE to an Oracle NUMBER (e.g., NUMBER(1)) and
cast the string result of the ISDEFAULT CASE to VARCHAR2(5) (or appropriate
VARCHAR2 length). Locate the CASE that checks VARTYPE (the alias TYPE) and the
CASE that compares setting = boot_val (the alias ISDEFAULT) and append the
corresponding ::NUMBER(...) and ::VARCHAR2(...) style casts (or use
TO_NUMBER/TO_CHAR as your style across the file) so they match the explicit
Oracle type casting used for other columns like SETTING::VARCHAR2(80) and
BOOT_VAL::VARCHAR2(255).
|
Hi @pierreforstmann,
|
|
Hi @yuanyl630
2/3/4: I have added BLOCKING_SESSION, WAIT_CLASS, SQL_ID, SQL_EXEC_START and tried to fix STATE and STATUS. |
|
Hi @pierreforstmann, That said, I think we still have a couple of must-fix items before we can merge, mainly around the blocking logic:
Nice-to-haves (not blocking this PR, but would improve compatibility): Let me know if you want me to point to a specific snippet for the pg_blocking_pids() approach — it should simplify the view a lot and avoid the tricky edge cases. |
|
Hi @pierreforstmann,
|
|
Hi @pierreforstmann, thanks for the updates on this PR. |
Hello,
I propose this patch to create a minimal view for Oracle SYS.V$SESSION.
Could you please confirm that this is right way to create a dynamic Oracle view.
Thanks and regards
Summary by CodeRabbit
New Features
Permissions