Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: oracleuserpriv #746

Merged
merged 4 commits into from
Feb 27, 2023
Merged

docs: oracleuserpriv #746

merged 4 commits into from
Feb 27, 2023

Conversation

slatawa
Copy link
Contributor

@slatawa slatawa commented Feb 22, 2023

docs: add instructions on privileges required by oracle user to connect to DVT

@slatawa
Copy link
Contributor Author

slatawa commented Feb 22, 2023

/gcbrun

@@ -155,6 +155,11 @@ Then `pip install cx_Oracle`.
}
```

### Oracle User permissions to run the validator tool:
* GRANT CREATE SESSION TO DVT-USER (Create Session Privilege)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be better to be consistent with the BigQuery section and simply list the privileges we require?
For example:

Oracle user permissions required to run this validator tool:

  • CREATE SESSION
  • READ or SELECT on any tables to be validated

Just a suggestion, not an essential change (I just like consistency :-) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nj1973 - Incorporated.

* GRANT CREATE SESSION TO DVT-USER (Create Session Privilege)
* GRANT READ ON SCHEMA_NAME.TABLE_NAME to DVT-USER; (Oracle Read table privilege)


Copy link
Contributor

Choose a reason for hiding this comment

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

I was recently pointed to an issue logged in the Oracle alert log that is caused by SQL Alchemy when the user doesn't have read privileges on V$TRANSACTION. I wonder if we should cover this too. For example:

If you encounter "WARNING: too many parse errors" messages in your Oracle alert log relating to a query from your DVT database account then a workaround is to grant READ privilege on SYS.V_$TRANSACTION. See here for background on this: https://docs.sqlalchemy.org/en/14/dialects/oracle.html#transaction-isolation-level-autocommit

Or do you think this is overkill?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nj1973 - Looking at the issue you shared ,I concur it should be a pre-requisite to have read on SYS.V_$TRANSACTION but don't see any reason to only give this privilege if the users sees the warning. from the doc

"The cx_Oracle dialect attempts to call the Connection.get_isolation_level() method when the dialect makes its first connection to the database in order to acquire the “default”isolation level."

so it seems to me that we should get a read priv on SYS.V_$TRANSACTION instead of the alternative behaviour which would be getting a background warning and setting to default.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that makes sense.
Because v$transaction is an unusual privilege to require I think we should include a comment or link to explain why we expect it. Also stating that it is optional (because code continues to function fine without it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nj1973 - have added the same to doc now.

@slatawa slatawa self-assigned this Feb 27, 2023
@slatawa
Copy link
Contributor Author

slatawa commented Feb 27, 2023

/gcbrun

@slatawa
Copy link
Contributor Author

slatawa commented Feb 27, 2023

/gcbrun

@slatawa slatawa requested a review from nj1973 February 27, 2023 10:11
Copy link
Contributor

@nj1973 nj1973 left a comment

Choose a reason for hiding this comment

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

LGTM

@slatawa slatawa merged commit a7889bf into develop Feb 27, 2023
@slatawa slatawa deleted the issue-705 branch February 27, 2023 11:37
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.

None yet

2 participants