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

Support server-side ANSI settings #347

Closed psoo closed 10 months ago

psoo commented 10 months ago

Specific feature in SQL Server forces us to set ANSI-compliant options during database sessions.

One example are XML features, especially when exposed when using views like this example:

SQL Server:

CREATE VIEW bernd.test_xml AS 
SELECT CAST('<name>Bernd</name>' as xml).exist('/name') AS xml_val;

Map/Import the view as a FOREIGN TABLE in PostgreSQL:

import foreign schema bernd LIMIT TO(test_xml) from server sqlserver_ubuntu INTO sqlserver ;
IMPORT FOREIGN SCHEMA

The view gets correctly mapped (the exist() XML function returns 1 to indicate it found a matching key):

\d sqlserver.test_xml 
                      Foreign table "sqlserver.test_xml"
 Column  │   Type   │ Collation │ Nullable │ Default │       FDW options       
─────────┼──────────┼───────────┼──────────┼─────────┼─────────────────────────
 xml_val │ smallint │           │          │         │ (column_name 'xml_val')
Server: sqlserver_ubuntu
FDW options: (schema_name 'bernd', table_name 'test_xml')

Though a SELECT from this view with its single column returning the result of the XMLQuery yields the following ERROR (msg_handler set to notice so the details of the error can be seen):

SELECT * FROM sqlserver.test_xml ;

NOTICE:  DB-Library notice: Msg #: 5701, Msg state: 2, Msg: Changed database context to 'bernd'., Server: 9327c78cf76b, Process: , Line: 1, Level: 0
NOTICE:  DB-Library notice: Msg #: 5703, Msg state: 1, Msg: Changed language setting to us_english., Server: 9327c78cf76b, Process: , Line: 1, Level: 0
NOTICE:  DB-Library notice: Msg #: 1934, Msg state: 1, Msg: SELECT failed because the following SET options have incorrect settings: 'ANSI_NULLS, QUOTED_IDENTIFIER, CONCAT_NULL_YIELDS_NULL, ANSI_WARNINGS, ANSI_PADDING'. Verify that SET options are correct for use with indexed views and/or indexes on computed columns and/or filtered indexes and/or query notifications and/or XML data type methods and/or spatial index operations., Server: 9327c78cf76b, Process: , Line: 1, Level: 16
ERROR:  DB-Library error: DB #: 20018, DB Msg: General SQL Server error: Check messages from the SQL Server, OS #: -1, OS Msg: , Level: 16

SQL Server forces ANSI compliant settings when using specific functions/features, like documented here (and the notice message also gives a hint regarding the XML datatype used):

https://learn.microsoft.com/en-us/sql/t-sql/statements/set-ansi-defaults-transact-sql?view=sql-server-ver16

There is a single ANSI_DEFAULTS setting, which can be used to make such constructs usable with TDS clients, according to the documentation it aggregates the following settings:

I've decided not to use ANSI_DEFAULTS directly, since i am not sure CURSOR_CLOSE_ON_COMMIT and especially IMPLICIT_TRANSACTIONS is something we want in tds_fdw. In fact, according to the documentation above, the native ODBC and OLEDB drivers are setting this two parameters back to OFF by default after having set ANSI_DEFAULTS ON.

ANSI_DEFAULTS ON also doesn't set CONCAT_NULL_YIELD_NULL, too. So i decided to set all required ANSI parameters explicitly by the new helper function tdsSetSqlServerAnsiMode(). Please note that SQL Server will force CONCAT_NULL_YIELDS_NULL ON in future releases, documented here:

https://learn.microsoft.com/en-us/sql/t-sql/statements/set-concat-null-yields-null-transact-sql?view=sql-server-ver16

So after all i've decided to set all options explicitly as we need.

The parameters need to be set at connection start, so i made it a new foreign server option sqlserver_ansi_mode, used e.g. like this:

CREATE SERVER sqlserver_ubuntu FOREIGN DATA WRAPPER tds_fdw OPTIONS (
    database 'bernd',
    port '1433',
    servername 'sqlserver.somewhere',
    sqlserver_ansi_mode 'true',
    tds_version '7.4'
);

First i used to name it ansi_mode, but research revealed that Sybase seems to have a completely different behavior here, thus i renamed it to sqlserver_ansi_mode to reflect its purpose better. Please see the updated documentation in ForeignServerCreation.md.

Since these properties are session specific i am not sure it make sense to make them available on table-level, too.

With all this in place, selecting such views just works:

ALTER SERVER sqlserver OPTIONS(ADD sqlserver_ansi_mode 'true');
ALTER SERVER

SELECT * FROM sqlserver.test_xml ;
NOTICE:  tds_fdw: Query executed correctly
NOTICE:  tds_fdw: Getting results
 xml_val 
─────────
       1
(1 row)
jenkins-juliogonzalez commented 10 months ago

Can one of the admins verify this patch?

GeoffMontee commented 10 months ago

Test this, please

GeoffMontee commented 10 months ago

Hi @psoo,

Thanks for the patch!

If this new option is only supported for MS SQL Server, should a condition be put in-place to determine if the server is MS SQL server before the call to tdsSetSqlServerAnsiMode()? There is some code in tdsImportForeignSchema() that shows how to do this. For example, see here: https://github.com/tds-fdw/tds_fdw/blob/2323efe2007d012b043fe91ea97a736b85eddce3/src/tds_fdw.c#L3874

psoo commented 10 months ago

Yeah, probably a good idea to protect against misconfinguration...

I'd go forward and make this a separate function, i don't want to repeat that stuff in tdsSetSqlServerAnsiMode() and tdsImportForeignSchema() code-wise.

What do you think about moving that check into a separate function, like

int tdsIsSqlServer()

and calling that.

GeoffMontee commented 10 months ago

That sounds like a great plan. Thanks!

jenkins-juliogonzalez commented 10 months ago

Test PASSed.

GeoffMontee commented 10 months ago

Also, would you be willing to squash the commits into a single commit to make the changes easier to review?

psoo commented 10 months ago

I am going to create a new PR, since squashing the already pushed changes into a single commit for this existing branch doesn't seem to work.

psoo commented 10 months ago

Pls see https://github.com/tds-fdw/tds_fdw/pull/348 for review.

GeoffMontee commented 10 months ago

This has been merged via #348.