fix MSVC2015 build & mssql instance name support & fix fails on IMPORT FOREIGN SCHEMA... #162
Conversation
|
Can one of the admins verify this patch? |
|
Test PASSed. |
|
@4321ip maybe you can add a new test to check the queries that were crashing? |
|
Test PASSed. |
|
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! |
|
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.
|
4321ip
changed the title
|
Test FAILed. |
| @@ -1407,7 +1414,8 @@ void tdsGetColumnMetadata(ForeignScanState *node, TdsFdwOptionSet *option_set) | |||
| (errmsg("tds_fdw: Comparing retrieved column name to the following local column name: %s", local_name) | |||
| )); | |||
|
|
|||
| if (strncmp(local_name, column->name, NAMEDATALEN) == 0) | |||
| //! \PPV local_name always in lowcase. But column->name's register defined in sql server. so you should ignore case | |||
| if (strnicmp(local_name, column->name, NAMEDATALEN) == 0) | |||
There was a problem hiding this comment.
It seems this is crashing tds_fdw on loading: https://jenkins.juliogonzalez.es/job/tds_fdw-build-pr/44/DISTRO=ubuntu14.04,PG_VER=9.2,label=docker/console
Could it be that strnicmp is not part of the standard library and MS stuff only?
|
@4321ip, can we get this PR fixed? If it helps with the Windows support, it could be useful. But for that we need:
|
some fixes