tds-fdw / tds_fdw

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

Trying to import `information_schema` leads to tds_fdw internal query crash #219

Open jcarnu opened 4 years ago

jcarnu commented 4 years ago

We are trying to import the information_schema from MSSQL.

IMPORT FOREIGN SCHEMA information_schema 
FROM SERVER mssql 
INTO foreign_information_schema;

ERROR:  syntax error at or near ")"
LINE 2: ) SERVER mssql        ^
QUERY:  SELECT 
   t.table_name,  
   c.column_name,   
   c.data_type,   
   c.column_default,   
   c.is_nullable,  
   c.character_maximum_length,   
   c.numeric_precision,   
   c.numeric_precision_radix,  
   c.numeric_scale,   
   c.datetime_precision 
FROM INFORMATION_SCHEMA.TABLES t
LEFT JOIN INFORMATION_SCHEMA.COLUMNS c 
        ON t.table_schema = c.table_schema 
          AND t.table_name = c.table_name 
WHERE 
t.table_type = 'BASE TABLE'
AND t.table_schema = 'information_schema' 
ORDER BY t.table_name, c.ordinal_position) 
SERVER mssql OPTIONS (schema_name 'information_schema', table_name '');

Note : the query runned directly on SQLServer returns no table because information_schema does not contain any table from its own schema.

The point is from the OPTION part and table_name being empty but the error seems to be confusing.

IMHO :

I should dig into the source code to help solve the problem. For the firs point, it should be quick, for the second @GeoffMontee I'd like to discuss about the possibility to query the system catalog and if tds indents to query MSSQLServer only or if there might be some other informations I should take care of.

This is a proposal.

jcarnu commented 4 years ago

One more word on this, maybe it should be checked in unit tests ?

GeoffMontee commented 4 years ago

Hi @jcarnu ,

the IMPORT SCHEMA should take care of the possible inexistance of tables in the corresponding schema

Yeah, I agree. We should probably add a check right around these two places, for both MS SQL Server and Sybase:

https://github.com/tds-fdw/tds_fdw/blob/d50c09f88f87ee842357211aa85441d47c93e27c/src/tds_fdw.c#L3044

https://github.com/tds-fdw/tds_fdw/blob/d50c09f88f87ee842357211aa85441d47c93e27c/src/tds_fdw.c#L3434

The code for the check can probably look like this:

erc = dbrows(dbproc);

if (erc == FAIL)
{
    ereport(NOTICE,
        (errmsg("tds_fdw: No tables were found in schema %s", stmt->remote_schema)
        ));

    return commands;
}

if information_schema does not provide the information of its own "tables" (that are actually views), maybe tds_fdw could query mssql system catalog for such a reason ?

It sounds good to me to use the system catalog in some cases. How would you suggest that should work?

jcarnu commented 4 years ago

Yeah, I agree. We should probably add a check right around these two places, for both MS SQL Server and Sybase:

https://github.com/tds-fdw/tds_fdw/blob/d50c09f88f87ee842357211aa85441d47c93e27c/src/tds_fdw.c#L3044

https://github.com/tds-fdw/tds_fdw/blob/d50c09f88f87ee842357211aa85441d47c93e27c/src/tds_fdw.c#L3434

The code for the check can probably look like this:

erc = dbrows(dbproc);

if (erc == FAIL)
{
  ereport(NOTICE,
      (errmsg("tds_fdw: No tables were found in schema %s", stmt->remote_schema)
      ));

  return commands;
}

Great, once the conference is over, I'll have a look at it and try to submit a patch for this.

if information_schema does not provide the information of its own "tables" (that are actually views), maybe tds_fdw could query mssql system catalog for such a reason ?

It sounds good to me to use the system catalog in some cases. How would you suggest that should work?

* Don't query `information_schema` at all for MS SQL Server, and instead, just query the system catalog tables?

* Query the system catalog tables as a fall-back if the `information_schema`  query returns 0 rows?

* Allow the user to specify an option if they would like to query the system catalog tables instead of the `information_schema` tables?

I only can say for MSSQL, maybe we could let the user choose with a default option. Can you check for sybase ? While information_schema is quite standard I cannot figure out if there will be the same columns in sys for both mssql and sybase.

Do you have any information on this ?

If sys is present on both platform maybe we can use only sys.

jcarnu commented 4 years ago

Well, it turns out, information_schema does not appear also in sysas a schema. So the only change needed is to check if any table is present in such a schema.

I still wonder whether information_schema is a virtual object hunder the hood.

OTOH `exec sp_helptext 'information_schema.columns' seems to return interesting entries...

As I'm far from being au SQLServer DBA, can someone help me in finding how that schema entry is handled ?

Thanks