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

Introduce new server option ansi_mode. #348

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

psoo
Copy link
Contributor

@psoo psoo commented Dec 1, 2023

Follow up PR for #347 with suggested changes, squashed commits.

This mode currently enables the following settings when connecting to SQL Servers:

CONCAT_NULLS_YIELDS_NULL ON
ANSI_NULLS ON
ANSI_WARNINGS ON
ANSI_PADDING ON
QUOTED_IDENTIFER ON
ANSI_NULL_DFLT_ON

Since we support the new option `sqlserver_ansi_mode` only for SQL Server, error out in case we don't find such a backend.
This also moves the check for the backend vendor into a separate function `tdsIsSqlServer()`, additionally used by `tdsImportForeignSchema()`.
Documentation for `sqlserver_ansi_mode` updated accordingly.
@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 PASSed.

@GeoffMontee
Copy link
Collaborator

Hi @psoo,

Thanks for the squashed patch! It looks like the tdsSetSqlServerAnsiMode() function has two declarations:

/*
 * Internal helper to set ANSI compatible server-side settings for SQL Server
 * in case foreign server was configured with sqlserver_ansi_mode 'true'.
 */
static void tdsSetSqlServerAnsiMode(DBPROCESS **dbproc);

/*
 * Internal helper to set ANSI compatible server-side settings for SQL Server
 * in case foreign server was configured with sqlserver_ansi_mode 'true'.
 */
static void tdsSetSqlServerAnsiMode(DBPROCESS **dbproc);

Shouldn't one of these be removed? If you fix this issue, then I will merge the patch.

Thanks!

@psoo
Copy link
Contributor Author

psoo commented Dec 4, 2023

Oh yes, looks like a copy&past mistake...sorry for this, didn't see this before pushing. Fixed.

@GeoffMontee
Copy link
Collaborator

Test this, please

@jenkins-juliogonzalez
Copy link
Member

Test PASSed.

@GeoffMontee GeoffMontee merged commit 14b147f into tds-fdw:master Dec 4, 2023
1 check passed
@GeoffMontee
Copy link
Collaborator

This has been merged. Thanks again for the patch!

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.

3 participants