Skip to content

feat(mssql): Microsoft SQL Server destination#6417

Merged
kodiakhq[bot] merged 2 commits intomainfrom
feat/dst/ms-sql-server
Jan 18, 2023
Merged

feat(mssql): Microsoft SQL Server destination#6417
kodiakhq[bot] merged 2 commits intomainfrom
feat/dst/ms-sql-server

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

@candiduslynx candiduslynx commented Jan 6, 2023

@candiduslynx candiduslynx requested review from a team and shimonp21 and removed request for a team January 6, 2023 08:04
@candiduslynx
Copy link
Copy Markdown
Contributor Author

This PR still required adding test workflow for destination testing + docs. Code is almost ready, though.

@candiduslynx candiduslynx self-assigned this Jan 6, 2023
@candiduslynx candiduslynx linked an issue Jan 6, 2023 that may be closed by this pull request
@yevgenypats yevgenypats requested review from yevgenypats and removed request for shimonp21 January 6, 2023 13:20
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the plugin incorrectly implements multiple interfaces and it needs to only implement the unmanaged plugin.

@candiduslynx candiduslynx force-pushed the feat/dst/ms-sql-server branch 4 times, most recently from 8e20e18 to b13bca1 Compare January 9, 2023 19:23
@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Jan 10, 2023

TODO: add test runner like in microsoft/mssql-docker#133 (comment)

@erezrokah
Copy link
Copy Markdown
Member

Hi @candiduslynx can we make sure to incorporate the fix from #6519?

@candiduslynx
Copy link
Copy Markdown
Contributor Author

Hi @candiduslynx can we make sure to incorporate the fix from #6519?

@erezrokah I think I already included those

@candiduslynx candiduslynx force-pushed the feat/dst/ms-sql-server branch 5 times, most recently from c6a42d9 to d4a6a98 Compare January 11, 2023 15:27
@candiduslynx candiduslynx force-pushed the feat/dst/ms-sql-server branch 5 times, most recently from cb80ff5 to 44763c2 Compare January 11, 2023 16:18
@candiduslynx candiduslynx force-pushed the feat/dst/ms-sql-server branch from b8ea7a0 to 0dd4cc3 Compare January 13, 2023 09:45
@candiduslynx candiduslynx force-pushed the feat/dst/ms-sql-server branch 3 times, most recently from c02ba22 to 8dd0886 Compare January 13, 2023 20:37
@candiduslynx candiduslynx added no automerge and removed automerge Automatically merge once required checks pass labels Jan 14, 2023
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I really like the bulk-insert. In overwrite I think we can do something similar otherwise it will be quite slow / open many goroutines and so on.

I think in overwrite we should use a temptable and then do a merge on the serverside - it's quite common approach in databases that don't have good built-in batch support.

https://blog.jonschneider.com/2016/08/bulk-upsert-into-sql-server.html
https://stackoverflow.com/questions/4889123/any-way-to-sqlbulkcopy-insert-or-update-if-exists

@candiduslynx
Copy link
Copy Markdown
Contributor Author

@yevgenypats I tried using temporary tables and the overhead was enormous (see fae6b7a).
The code was creating tmp table, doing bulk insert and only then performing merge.

@candiduslynx
Copy link
Copy Markdown
Contributor Author

@erezrokah seems that some base links (not touched by this PR) were broken

@candiduslynx
Copy link
Copy Markdown
Contributor Author

@yevgenypats I used table value parameters along with stored procedure to achieve better results.

Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @candiduslynx !

This one is a complex one for sure as it's an "old" db :)

I played with and overall it looks good. I've a few comments but I'd like to wait for some real world usage to see if users hit any snags.

For example:

  1. TVP is really cool but also it adds complexity, which means harder to debug bugs. Can we achieve the same thing with temp tables? maybe and maybe it wont be better as temp tables also have their snags as not everything is happy flow in MSSQL world.

So let's put it out there and once we release tmrw we can update the users that requested that.

@candiduslynx
Copy link
Copy Markdown
Contributor Author

@yevgenypats thanks for the review!
There was an attempt to use temporary tables + bulk insert + merge (see my comment above: #6417 (comment)).
Overall, creating a temporary table added a huge overhead, so the only feasible way of performing the overwrite would be to use staging tables or do the TVP approach I implementetd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add SQL Server as a supported destination

5 participants