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

Import foreign schema #91

Merged
merged 1 commit into from
Sep 2, 2016
Merged

Import foreign schema #91

merged 1 commit into from
Sep 2, 2016

Conversation

za-arthur
Copy link
Contributor

Hello,

This pull request fixes the issue #75
I added tests and some documentation. I added test postgresql/003_import_schema.sql, so other tests was renamed.

If you have notes about this I will fix it.

@jenkins-juliogonzalez
Copy link
Member

Can one of the admins verify this patch?

@GeoffMontee
Copy link
Collaborator

Test this, please

@jenkins-juliogonzalez
Copy link
Member

Test FAILed.

@juliogonzalez
Copy link
Member

As IMPORT FOREIGN SCHEMA is only available for PostgreSQL >= 9.5, I will need to modify the script and the tests to support different PostgreSQL versions.

@select-artur, let me think a little about it and I'll make a patch so you can rebase it into this branch. It won't take long.

Meanwhile, may I suggest you squash your commits into a single one?

@za-arthur
Copy link
Contributor Author

za-arthur commented Sep 2, 2016

Meanwhile, may I suggest you squash your commits into a single one?

Done.

@select-artur, let me think a little about it and I'll make a patch so you can rebase it into this branch. It won't take long.

Thank you.

@juliogonzalez
Copy link
Member

@select-artur, the patch is ready and merged.

Now all the SQL files have a corresponding JSON file with, among other things, the minimum and maximum version required for each of the tests

Please rebase changes from master into your branch, fix the conflicts and just add a file tests/tests/postgresql/003_import_schema.json with the following content:

{
    "test_desc" : "foreign schema import",
    "server" : {
        "version" : {
            "min" : "9.5.0",
            "max" : ""
        }
    }
}

As the foreign tables for all other tests are deleted before running the actual test (check tests/tests/postgresql/003_tinyintmin.sql for example), you could also create your test as number 032, to avoid renaming all the previous tests (if it's more comfortable for you)

@juliogonzalez
Copy link
Member

I see that there are no conflicts, so it should be enough if you just add the JSON file as "tests/tests/postgresql/003_import_schema.json" :-)

@za-arthur
Copy link
Contributor Author

Thanks! I will add it.

@juliogonzalez
Copy link
Member

Great! @GeoffMontee or me will run the tests as soon as one of us see the commit.

@za-arthur
Copy link
Contributor Author

I suppose I did it.

you could also create your test as number 032, to avoid renaming all the previous tests (if it's more comfortable for you)

I've create import test as number 003 because I concern if IMPORT FOREIGN SCHEMA will be executed by the last query, then an error can be throwed. Because all foreign table will be exist. I cant test it now, because I will have the MS SQL environment only on monday.

Of course I can create import test as number 032 if I'm wrong.

@GeoffMontee
Copy link
Collaborator

Test this, please

@juliogonzalez
Copy link
Member

@select-artur, good point!

I can't see any mention about what will happen if one of the tables already exist, so most probably it will fail as you correctly guessed.

@jenkins-juliogonzalez
Copy link
Member

Test FAILed.

@juliogonzalez
Copy link
Member

juliogonzalez commented Sep 2, 2016

retest this, please (test faileds because of a problem starting PostgreSQL, not because of the PR itself)

@jenkins-juliogonzalez
Copy link
Member

Test PASSed.

@GeoffMontee GeoffMontee merged commit 8c2c3f9 into tds-fdw:master Sep 2, 2016
@GeoffMontee
Copy link
Collaborator

The tests passed, so this has been merged. Thanks a lot for the contribution, @select-artur!

@za-arthur
Copy link
Contributor Author

Thanks!

@ellmout
Copy link

ellmout commented Sep 20, 2016

Hi,

Not working on Sybase (12.5), since Sybase use sysobjects to store Tables informations.

@za-arthur
Copy link
Contributor Author

Hello @elliotVR ,
unfortunately I haven't Sybase server instance. So I cant implement and test it.

Maybe the solution is to add new option to IMPORT FOREIGN SCHEMA. Depening on this option it is necessary use various queries to retreive information about objects. Or maybe there is a possibility to get from tds_fdw is server Sybase or SQL Server.

@GeoffMontee
Copy link
Collaborator

Hi @elliotVR,

I just created issue #104 to track that we do not have a Sybase implementation of IMPORT FOREIGN SCHEMA yet.

@select-artur,

If you are interested in trying to develop the Sybase implementation of IMPORT FOREIGN SCHEMA as well, it looks like there is a free version of Sybase ASE available called SAP Adaptive Server Enterprise Express Edition. (Note: Sybase was purchased by SAP, so the name of the software has changed.)

@za-arthur
Copy link
Contributor Author

Hi,
@GeoffMontee, I think I could develop it in a couple weeks.

@GeoffMontee
Copy link
Collaborator

That would be awesome, @select-artur! tds_fdw's Sybase users would probably appreciate having that functionality. Thanks!

@za-arthur za-arthur deleted the import-schema branch November 19, 2016 21:17
@za-arthur
Copy link
Contributor Author

@elliotVR can you investigate the pull request #108 ?

It seems that there is not schemas in SAP ASE. So IMPORT FOREIGN SCHEMA imports tables of specific owner. So for SAP ASE table owner is equal to schema. Isn't wrong?

@GeoffMontee
Copy link
Collaborator

@elliotVR,

PR #108 from @select-artur has just been merged into the master branch if you want to test it out.

Thanks again for the great patch, @select-artur!

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.

6 participants