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

[Oracle] Sqitch does not detect missing schema #668

Closed
vectro opened this issue Aug 9, 2022 · 15 comments · Fixed by #762
Closed

[Oracle] Sqitch does not detect missing schema #668

vectro opened this issue Aug 9, 2022 · 15 comments · Fixed by #762
Assignees

Comments

@vectro
Copy link
Contributor

vectro commented Aug 9, 2022

Steps to reproduce:

  1. Run sqitch deploy --registry xxx, where xxx is a schema that does not exist on the target database; and where there is a sqitch registry in the connecting user's schema.

Expected behavior:

  • Sqitch would report an error and take no action.

Actual behavior:

  • Sqitch will query the registry in the connecting user's schema instead.
@theory
Copy link
Collaborator

theory commented Aug 10, 2022

Odd. Can you apply this patch, try again, and send me the output?

--- a/lib/App/Sqitch/Command/deploy.pm
+++ b/lib/App/Sqitch/Command/deploy.pm
@@ -114,6 +114,7 @@ sub execute {
 
     # Warn on multiple targets.
     my $target = shift @{ $targets };
+    say 'Registry: ', $target->registry;
     $self->warn(__x(
         'Too many targets specified; connecting to {target}',
         target => $target->name,

@theory theory self-assigned this Aug 10, 2022
@theory theory added bug engine waiting Waiting on feedback labels Aug 10, 2022
@vectro
Copy link
Contributor Author

vectro commented Aug 11, 2022

with --registry does_not_exist, I see

Registry: does_not_exist
Nothing to deploy (already at "first-change")

@theory
Copy link
Collaborator

theory commented Aug 11, 2022

Oh interesting!

@theory
Copy link
Collaborator

theory commented Aug 12, 2022

Oh I see what's happening. Sqitch attempts to set the schema path on connect, but doesn't fail if the schema doesn't exist, on the assumption that it will be created and set on the first deploy.

if (my $schema = $self->registry) {
try {
$dbh->do("ALTER SESSION SET CURRENT_SCHEMA = $schema");
# https://www.nntp.perl.org/group/perl.dbi.dev/2013/11/msg7622.html
$dbh->set_err(undef, undef) if $dbh->err;
};
}

And since you're not doing a deploy, it just defaults to the current schema. It seems as though you previously used the same schema for your own database object and for Sqitch, yes? This usually doesn't fail because the ideal is to use a separate schema for the registry, and you don't specify (or default to) the registry for your target.

So my short answer is "don't use your own schema for the registry".

The longer response is…I'm not sure this is something that'd be easy to change.

@vectro
Copy link
Contributor Author

vectro commented Jan 11, 2023

Hmm, in Oracle at least I wouldn't expect Sqitch to automatically create the registry schema.

That is because in Oracle there is an isomorphism between schemas and users, and to me creating users is something that shouldn't be done implicitly.

Do we rely on this error-swallowing functionality?

@theory
Copy link
Collaborator

theory commented Jan 12, 2023

Yep:

if (my $schema = $self->registry) {
try {
$dbh->do("ALTER SESSION SET CURRENT_SCHEMA = $schema");
# https://www.nntp.perl.org/group/perl.dbi.dev/2013/11/msg7622.html
$dbh->set_err(undef, undef) if $dbh->err;
};
}

It gets created here:

(my $file = file(__FILE__)->dir->file('oracle.sql')) =~ s/"/""/g;
$self->_run_with_verbosity($file);
$self->dbh->do("ALTER SESSION SET CURRENT_SCHEMA = $schema") if $schema;
$self->_register_release;

But the Oracle registry script doesn't create the schema (user). Instead, according to the tutorial, it just uses the current schema. From the tutorial:

First Sqitch created the registry tables used to track database changes. The structure and name of the registry varies between databases, but in Oracle they are simply stored in the current schema -- that is, the schema with the same name as the user you've connected as. In this example, that schema is scott. Ideally, only Sqitch data will be stored in this schema, so it probably makes the most sense to create a superuser named sqitch or something similar and use it to deploy changes.

@theory theory removed the waiting Waiting on feedback label May 20, 2023
@theory
Copy link
Collaborator

theory commented May 20, 2023

I just worked through it all again, committing 8be3608 to improve the consistency of handling the registry username. But I still can't work out how this is happening, unless it's not capturing some errors somewhere. Would you try again with this patch, please?

--- a/lib/App/Sqitch/Engine/oracle.pm
+++ b/lib/App/Sqitch/Engine/oracle.pm
@@ -459,8 +459,11 @@ sub initialize {
 
     # Load up our database.
     (my $file = file(__FILE__)->dir->file('oracle.sql')) =~ s/"/""/g;
+    say "Initialize $schema";
     $self->_run_with_verbosity($file);
+    say "Set session to $schema";
     $self->dbh->do("ALTER SESSION SET CURRENT_SCHEMA = $schema") if $schema;
+    say "Register release in $schema";
     $self->_register_release;
 }
 
@@ -742,6 +745,7 @@ sub _script {
 sub _run {
     my $self = shift;
     my $script = $self->_script(@_);
+    say "Run $script";
     open my $fh, '<:utf8_strict', \$script;
     return $self->sqitch->spool( $fh, $self->sqlplus );
 }
@@ -749,6 +753,7 @@ sub _run {
 sub _capture {
     my $self = shift;
     my $conn = $self->_script(@_);
+    say "Capture $conn";
     my @out;
 
     require IPC::Run3;

@theory theory added the waiting Waiting on feedback label May 20, 2023
@vectro
Copy link
Contributor Author

vectro commented Jun 1, 2023

So, this might be frustrating, but that patch does not result in any extra output. Presumably because Sqitch does not detect that schema initialization is needed?

@theory
Copy link
Collaborator

theory commented Jun 2, 2023

No output at all? Weird. What happens if you try to print the schema name in initialized()?

@vectro
Copy link
Contributor Author

vectro commented Jun 2, 2023

Okay so I dug into this a bit today and there is an extra condition required to trigger the behavior documented above.

I would amend the "steps to reproduce" above as follows:

  1. Run sqitch deploy --registry xxx, where xxx is a schema that does not exist on the target database; and where there is a sqitch registry in the connecting user's schema, and where that registry contains an entry in the changes table whose project corresponds to the current project name.

What happens is that we query the changes table (in _select_state()) for this project; and if it is found, then deploy() will never call initialized().

@theory
Copy link
Collaborator

theory commented Jun 3, 2023

IIRC, it currently defaults to the current database if the sqitch registry doesn't exist, but it shouldn't fall back if the registry schema name is explicitly named, as in your example. Will have to think about how to deal with that…

theory added a commit that referenced this issue Jun 4, 2023
Instead of ignoring them, record when the schema can't be added to the
search path. Then use that value to avoid querying the database in
`_sync_plan`. This prevents Sqitch from finding a valid project record
when the search path contains a valid changes table and record for the
project, which can happen when one starts using the current schema for
the registry (the default on Oracle) and then tries to use a new
registry. Resolves #668.

To best make use of the new `_no_registry` attribute of Engine, rename
the `initialized()` method to `_initialized()` and add a new
`initialized()` method to check it before calling the engine-specific
`_initialized()` method. Also rename the `initialize()` method to
`_initialize()` and provide an `initialize()` implementation that sets
`_no_registry` to false after a successful call to the engine-specific
`_initialize()` method.

Update the DBI `connected` callbacks to return anytime `$dbh->do`
returns false, thereby always propagating errors. Previously errors were
ignored in MySQL and Snowflake (and we had an invalid warehouse name in
the tests that needed fixing!).

For search path errors, call the new DBIEngine role
`_handle_no_registry` method, which sets `_no_registry` to true and
discards the error so that the DBI won't throw it once the callback
function returns. Since the error handling is boolean-based, here, and
the error handler is never called from inside the callback, remove the
`try {}` block. It wasn't doing anything.

While at it, require DBI 1.631 to ensure consistent behavior of error
handling in callbacks and to eliminate a workaround for older versions
in the mysql engine.
@theory
Copy link
Collaborator

theory commented Jun 4, 2023

#762 should fix the issue. It detects when the registry schema does not exist at connection time, and doesn't bother to call _select_state() if it doesn't exist.

@theory theory added patched Fixed in a branch but not yet merged. and removed waiting Waiting on feedback labels Jun 4, 2023
@vectro
Copy link
Contributor Author

vectro commented Jun 5, 2023

This seems reasonable, but maybe there is one more case that slipped through the cracks? What if:

  • The default schema contains a changes table whose project corresponds to the current project name
  • The registry schema exists, but is empty

It seems like in this case we will check if the registry schema exists but then still end up querying the default schema instead. What do you think?

@theory
Copy link
Collaborator

theory commented Jun 24, 2023

Ugh, yeah, that's quite the edge case, but a very real possibility. The only solution I can think of offhand is to schema-qualify the changes table in that query — for those databases that use schemas, that is. Will have to give that some thought, but perhaps in a separate PR.

@theory
Copy link
Collaborator

theory commented Jun 25, 2023

Him, the docs suggest that ALTER SESSION SET CURRENT_SCHEMA changes the default schema. I don't see anything about it falling back on the previous default schema, or a schema path. Am I missing something?

@theory theory closed this as completed in c6ccc30 Jul 1, 2023
@theory theory removed the patched Fixed in a branch but not yet merged. label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants