Is anything happening with this? Seems like a no-brainer and would be really helpful to MSSQL users

@elhigu @kibertoad Patched my local knex with this and it seems to work

@dustryder Been testing it for a little bit and everything works well except for unique composite indexes and tables with triggers. I ended up just using a .del() then .insert in my case since it was just for a seed file but it seems to have been the issue with the original PR as well: #3763 (comment).

Another issue is that I seem to be getting The target table '<table name>' of the DML statement cannot have any enabled triggers if the statement contains an OUTPUT clause without INTO clause. errors which may or may not be related to #4152

The code in the commit linked related to triggers is only hit if you add the includeTriggerModifications option in your insert or update statement. This just bypasses the output using a slower methodology but it's a simple way to get around the trigger issues for return values. You could use the same ideology for the upset I suspect though. If you need further clarifications let me know.

@KalebMatthews Sorry for the late reply! I've tried a bunch of different fixes but none of them seem to work. Would you be able to provide some guidance on the exact changes to make?

@KrisLau So you're issue is that you are treating the output like a return statement. As the error indicates if a table has a trigger the output statement must be pushed into a temp table of some sort. The main knex maintainers did not like the weight that this put on the normal query (which I agree with) which is why I added a modification option to enable to trigger specific insert, update, del returns. You can add the return feature only if the return is being utilized and generate the temp table and separate select statement (and delete temp table statement) if a return is specified. If one is not used then you can just return the count or exclude the output completely.

That is my suggestion.

@KalebMatthews Sorry for all the trouble! I looked through the _insertWithMerge function but it seems to be doing the same thing that you had implemented in the insert methods unless I'm misunderstanding the code. Specifically this part seems to be replicating the behaviour you mentioned:

const returningSql = returning
  ? ` ${this._returning('insert', returning, options.includeTriggerModifications)}`
  : ''

// other code

sql += returningSql;

if (options && options.includeTriggerModifications) {
  sql = this._buildTempTable(returning) + sql + this._buildReturningSelect(returning);
}

So is the issue with my code have to do with me having to pass true to the includeTriggerModication option?

The only other difference I could see was the return statement returning just the sql like this in this PR: return sql instead of an object with both sql and returning like this:

return {
    sql,
    returning
}

I tried changing the return to match the latter but that still resulted in the same error. I also tried changing all the returningSql to empty strings (which I assume just means it doesn't return anything from the statement?), removing anything top do with the temp tables, calling _returning with the includeTriggerModifications set to true manually, and they all seemed to either result in the same error or nothing changing in the DB

Side note: I tried adding console.log to the functions but nothing seemed to be be printed to the console for some reason so I was having trouble trying to debug what exactly was changing.

According to your testing scripts you are expecting results like
merge into [upsert_tests] using (values (?, ?)) as tsource([email], [name]) on [upsert_tests].[email] = tsource.[email] when not matched then insert ([email], [name]) values (tsource.[email], tsource.[name]) output inserted.[email];

The last output is where your error string is referring to.

@KalebMatthews Managed to get rid of the error turns out I hadn't noticed that I fixed the error and was getting a different one related to something else (managed to fix that too), thanks so much for the help!

@KalebMatthews Managed to get rid of the error turns out I hadn't noticed that I fixed the error and was getting a different one related to something else (managed to fix that too), thanks so much for the help!

Glad I could be of some help.

coverage: 92.691% (-0.2%) from 92.843%
when pulling 4630953 on dustryder:knex-5646-mssql-merge
into b6507a7 on knex:master.

@kibertoad Saw that this was merged, thanks! Just wanted to add that I ended up not using the method awhile back when I was researching and thought I'd add some notes for anyone in case they might want to use the method.

It seems at least from some articles and StackOverflow posts I read that the usage of MERGE is not necessarily safe? From my understanding it causes locks and some other issues that would be bad for systems that have a lot of concurrent users as well as a number of other issues. Most articles seem to suggest against using it overall:

• https://michaeljswart.com/2021/08/what-to-avoid-if-you-want-to-use-merge/ (the most cited one when i read forums)
• https://www.mssqltips.com/sqlservertip/3074/use-caution-with-sql-servers-merge-statement/
• https://sqlblog.org/merge

I think the method would still be very useful to a subset of people but a note on the method somewhere in the documentation and maybe a comment on the method specifying that mssql's implementation has some drawbacks on the MSSQL side.

What I ended up implementing (using objectionjs's inside the BaseModel) instead in case this might be helpful to anyone else. Lmao forgot I I wrote something similar before. My code is here

@KrisLau would you recommend reverting feature overall, or adding documentation?

@kibertoad I dont think I have enough expertise to give great advice haha but imo probably just add documentation? Most of my current knowledge was just those forums & articles.
Ideally, would be an implementation that just uses insert and update somehow I'm guessing (one alternative implementation) but I've also seen some people say that the merge statement works for them in their use case so I'm not sure

This looks close. Looking forward to it.

will this be merged in sometime soon? would benefit from this functionality today.