search

tds-fdw / tds_fdw

A PostgreSQL foreign data wrapper to connect to TDS databases (Sybase and Microsoft SQL Server)
Other
381 stars 102 forks source link

fix MSVC2015 build & mssql instance name support & fix fails on IMPORT FOREIGN SCHEMA... #162

Open 4321ip opened 6 years ago

4321ip commented 6 years ago

some fixes

jenkins-juliogonzalez commented 6 years ago

Can one of the admins verify this patch?

jenkins-juliogonzalez commented 6 years ago

Test PASSed.

juliogonzalez commented 6 years ago

@4321ip maybe you can add a new test to check the queries that were crashing?

jenkins-juliogonzalez commented 6 years ago

Test PASSed.

GeoffMontee commented 6 years ago

Hi @4321ip,

Thanks for the PR!

Unfortunately, I don't think that I can accept the PR, as it currently is. Here are some comments:

Please let me know if you have any questions.

Thanks again for the PR!

4321ip commented 6 years ago

Hello! Sorry for my first pull. Maybe its not fully correct.

8b76c8e  has some code commented out. It is unclear to me why this code is commented out. I am also not entirely sure that I agree with the assertion in the comment that DEFAULT clauses are "useless". I suspect that DEFAULT clauses might be important when/if tds_fdw supports INSERT.

tds_fdw  INSERT - I'm thinking of realizing it )) if base table in MSSQL have default values for column, then you can miss it in pgsql insert query. What do you need else?

418b140  also has some code commented out. If you move a function call to a new location, it would probably be better to delete the redundant instance of the code, rather than just commenting it out. However, I am unsure why this change is necessary. Can you please provide an example of when the old code had problems, so we know what problem the new code fixes? It looks like the problem involved a case where relname == NULL. When would that happen in this context? Shouldn't all queries handled by tds_fdw involve a foreign table?

this code occurs segfault in tds_fdw.dll: CREATE FOREIGN TABLE fdw_server.xxx (dt timestamp) SERVER fdw_mssql OPTIONS (query 'select SYSDATETIME() as dt'); select * from fdw_server.xxx; also you can use query  code like this:  .....OPTIONS (query 'exec sp_linkedservers ');  its legal code for mssql

b6b8665  also has code commented out. I'm also not sure if the change is a good one, because it forces tds_fdw to use local statistics by default, and I'm not actually sure how well ANALYZE works with tds_fdw's foreign tables, so I'm not entirely confident that the local statistics are meaningful. I should also add that if you are using MS SQL Server, then this change isn't even necessary because there is already a solution to the problem of the query running multiple times--set row_estimate_method=showplan_all. See the documentation for more information -  https://github.com/tds-fdw/tds_fdw/blob/master/ForeignTableCreation.md

this solution  is not complete. its not working for IMPORT FOREIGN SCHEMA ... Postgresql docs say that default value for  DEFAULT_USE_REMOTE_ESTIMATE is 0. ( https://www.postgresql.org/docs/9.3/static/postgres-fdw.html ) But you redefine this. its not very good.

821c6cb  - It is not clear to me why the case of column names should be ignored. I also think the assertion that "local_name always in lowcase" is false. People can create upper or mixed-case column names by enclosing the names in quotes when they create the table. Am I misunderstanding something? CREATE FOREIGN TABLE fdw_server.xxx (FIELD_NAME timestamp) SERVER fdw_mssql OPTIONS (query 'select SYSDATETIME() as FIELD_NAME ');  select * from fdw_server.xxx; if (strnicmp(local_name, column->name, NAMEDATALEN) == 0)

in this case local_name == "field_name". (postgre fdw make this), but column->name still equal "FIELD_NAME"

cd56e0a  also has code commented out. It looks like you added include directives for 2 FreeTDS headers, but then commented out the new directives? I like the added support for an "instance" name. It also seems to include a fix for an unrelated crash? What crash did this fix?

I add  fredts header because of i want edit login manually in this code if ((*dbproc = dbopen(login, conn_string)) == NULL) But it seems private structure. So i cancel this bruteforce, but forget clear commented code. by the way connect with inctance name is working. I saw code in "dbopen" func in freeDTS. Its not very good. But its working.

t also seems to include a fix for an unrelated crash? What crash did this fix?

this code occurs segfault in tds_fdw.dll: CREATE FOREIGN TABLE fdw_server.xxx (srv_name text ,srv_providername text, SRV_PRODUCT text ,SRV_DATASOURCE text) SERVER fdw_mssql OPTIONS (query 'exec sp_linkedservers');  select * from fdw_server.xxx;

exec sp_linkedservers return more columns, then  CREATE FOREIGN TABLE syntax.

jenkins-juliogonzalez commented 6 years ago

Test FAILed.

juliogonzalez commented 5 years ago

@4321ip, can we get this PR fixed? If it helps with the Windows support, it could be useful.

But for that we need: