-1

I am trying to create a stored procedure to which is passed a TVP and then some data from the TVP is inserted into two tables.

I have already implemented the stored procedure, but only the second insert (the only one that does not read from the TVP) is working. The other two are not working (do not insert anything) and I can't seem to figure out why.

I have tried to create a dummy TVP in SQL Server and run the procedure there, but that also did not work. Is this being caused by the fact TVPs are readonly? I would assume not, since I am not actually inserting or updating data inside the TVP.

Is there a way to make this work?

Thank you for your assistance!

Table-valued parameter definition:

CREATE TYPE dbo.Ingredients 
AS TABLE 
( 
  Quantity int,
  Measure nvarchar(50),
  Ingredient nvarchar(50),
)
GO

Stored procedure:

ALTER PROCEDURE uspCreateRecipe 
    (@IDUser int, 
     @RecipeName nvarchar(50), 
     @Category nvarchar(50), 
     @Difficulty nvarchar(50), 
     @Duration nvarchar(50), 
     @ING dbo.Ingredients READONLY, 
     @Execution text)
AS
BEGIN
    INSERT INTO dbo.Ingredients 
    VALUES ((SELECT Ingredient FROM @ING WHERE NOT EXISTS (SELECT Ingredient FROM @ING WHERE Ingredient IN (SELECT IngredientName FROM dbo.Ingredients))), 2)

    INSERT INTO dbo.Recipes 
    VALUES (@IDUser, @RecipeName, NULL, 
            (SELECT IDDifficulty FROM dbo.Difficulty WHERE Difficulty = @Difficulty), 
            (SELECT IDDuration FROM dbo.Duration  WHERE Duration = @Duration ), 
            NULL, 
            (SELECT IDCategory FROM dbo.Category WHERE CategoryName = @Category ), 
            @Execution , NULL, 2, GETDATE())

    INSERT INTO dbo.Recipes_Ingredients 
    VALUES (SCOPE_IDENTITY(), 
            (SELECT Quantity FROM @ING), 
            (SELECT IDMeasure FROM dbo.Measure WHERE Measure IN (SELECT Measure FROM @ING)), 
            (SELECT IDIngredient FROM dbo.Ingredients WHERE IngredientName IN (SELECT Ingredient FROM @ING)))
END
marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
Martunis99
  • 21
  • 1
  • 8
  • 5
    Show us your [MRE] i.e. where you provide some values in the TVP and nothing happens. And for debugging purposes put some selects in your SP to work out what is coming in. Also, as best practice you should always list the columns your are inserting into. And don't use a combination of `values` and `select` to obtain the values to insert, use one or the other. – Dale K Jun 27 '21 at 22:26
  • 6
    `@Execution text)` NO NO NO NO NO! This datatype has been deprecated for almost 20 years. It should not be used for new development. – SMor Jun 27 '21 at 22:28
  • 4
    Whitespace doesn't cost, you know – Charlieface Jun 27 '21 at 23:22
  • 3
    My eyes aren't wide enough to tell, but it doesn't look like you've used [correlated subqueries](https://learn.microsoft.com/en-us/sql/relational-databases/performance/subqueries?view=sql-server-ver15#correlated) in your `not exists` subquery. Without correlation it simply turns into an all-or-nothing switch for _all_ rows in the outer query. – HABO Jun 28 '21 at 02:30
  • 1
    Hey @SMor! Thank you for the input, I changed all text variables in my DB to nvarchar(max), like Dale said. – Martunis99 Jun 28 '21 at 22:15
  • 1
    @Charlieface ahah yeah I really need to structure my SQL better! Thank you – Martunis99 Jun 28 '21 at 22:15

1 Answers1

5
  1. Rather than using VALUES with sub-queries, just use SELECT.
  2. Always list the columns you are inserting into. Its clearer and will reduce errors especially if you modify the table structure in future,
  3. Your first query appeared to be overcomplicated - if indeed it worked at all.
  4. Your third query should have thrown an error because you have multiple IN sub-queries which should have resulted in a "sub-query returned multiple results" error.
  5. The text datatype is depreciated use varchar(max).
  6. Normally you want to SET NOCOUNT, XACT_ABORT ON.
  7. Always RETURN a status so your calling app knows whether it succeeded or not. 0 will be returned by default by I prefer to be explicit.
  8. Semi-colon terminate all statements.
ALTER PROCEDURE uspCreateRecipe
(
  @IDUser int
  , @RecipeName nvarchar(50)
  , @Category nvarchar(50)
  , @Difficulty nvarchar(50)
  , @Duration nvarchar(50)
  , @ING dbo.Ingredients READONLY
  , @Execution nvarchar(max) -- text is depreciated
)
AS
BEGIN
    SET NOCOUNT, XACT_ABORT ON;

    INSERT INTO dbo.Ingredients ([Name], Col2)
        SELECT Ingredient, 2
        FROM @ING
        WHERE Ingredient NOT IN (SELECT IngredientName FROM dbo.Ingredients);

    INSERT INTO dbo.Recipes (IDUser, RecipeName, Col3, IDDifficulty, IDDuration, Col6, IDCategory, Col8, Col9, Col10, Co11)
        SELECT @IDUser, @RecipeName, NULL, IDDifficulty
            , (SELECT IDDuration FROM dbo.Duration WHERE Duration = @Duration)
            , NULL
            , (SELECT IDCategory FROM dbo.Category WHERE CategoryName = @Category)
            , @Execution, NULL, 2, GETDATE()
        FROM dbo.Difficulty
        WHERE Difficulty = @Difficulty;

    INSERT INTO dbo.Recipes_Ingredients (IDRecipe, Quantity, IDMeasureid, IDIngredient)
        SELECT SCOPE_IDENTITY(), Quantity
            , (SELECT IDMeasure FROM dbo.Measure WHERE Measure = I.Measure)
            , (SELECT IDIngredient FROM dbo.Ingredients WHERE IngredientName = I.Ingredient)
        FROM @ING I;

    RETURN 0;
END;
Dale K
  • 25,246
  • 15
  • 42
  • 71
  • Why `RETURN 0;` what else do you expect it to return, and why do you care anyway? – Charlieface Jun 27 '21 at 23:22
  • 2
    @Charlieface I like to be explicit about these things :) – Dale K Jun 27 '21 at 23:24
  • 1
    Hi @DaleK! Sorry for not getting back to you sooner but I was unavailable. To answer your question shortly: yes, the procedure is now working like a charm! But also thank you for all the tips and best practices you shared! I will keep them in mind in the future. They all make perfect sense except one: why the need to return a status? What does it actually do? I am quite new to this world and it is always important to learn to do things the right way from early on. It is not the first time we run into each other, so thank you for the availability and knowledge-sharing! :) – Martunis99 Jun 28 '21 at 21:39
  • 1
    @Martunis99 the return value of a SP is an int status intended to communicate to the calling context whether the SP succeeded or not. 0 is returned by default and means OK, however I prefer to be explicit. And if a failure condition arises, then you can return some other number to communicate that to your app/calling context. – Dale K Jun 28 '21 at 21:54
  • 1
    @DaleK I see, that makes sense. Again, thank you for the world of knowledge you have shared and for helping me learn the ropes around here! – Martunis99 Jul 02 '21 at 20:32