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

Add batchError option to connection.executeMany() for PL/SQL #1232

Open
wattry opened this issue Mar 24, 2020 · 14 comments
Open

Add batchError option to connection.executeMany() for PL/SQL #1232

wattry opened this issue Mar 24, 2020 · 14 comments

Comments

@wattry
Copy link

wattry commented Mar 24, 2020

  1. I was not able to find anything that matches this request in the issues.

  2. We have applications that allow users to update multiple data objects in one request. Currently, this only works with INSERT, UPDATE, DELETE and MERGE SQL. However, we have large legacy DBs with error handling on the oracle side and use a PL/SQL package. We need to capture which executions were successful and which were not.

Personally, I think an array of success or error ids would be most useful. Or something containing details for each failure.

1.  [{ success: [id,...n] }, { error: [id,...n ] }]

2. [{ success: [id,...n] }, { error: { id: error, id...n: error...n } }]

Currently, we could use the autoCommit option to make sure successful requests are committed. However, if more than one commit fails we will only get an error for the first error message. Our frontend is expecting data about the success and failed executions to update state.

  1. We're using oracledb v4.2.0 in docker containers running node.js 12
@wattry wattry changed the title Add batchError option to conection.executeMany() for PL/SQL Add batchError option to connection.executeMany() for PL/SQL Mar 24, 2020
@anthony-tuininga
Copy link
Member

This is a restriction put in place by the Oracle Client libraries and not something that can be changed by the node-oracledb driver. I'll ask internally whether this restriction can be lifted in the Oracle Client libraries at some point.

@wattry
Copy link
Author

wattry commented Mar 24, 2020

This is a restriction put in place by the Oracle Client libraries and not something that can be changed by the node-oracledb driver. I'll ask internally whether this restriction can be lifted in the Oracle Client libraries at some point.

@anthony-tuininga very much appreciated.

@dmcghan
Copy link

dmcghan commented Mar 24, 2020

@09wattry Just to be sure, you're able to do what you need via PL/SQL for the moment correct? You're just looking for a way to make it easier going forward?

@wattry
Copy link
Author

wattry commented Mar 24, 2020

@dmcghan yes, we will be able to execute the PL/SQL like this. We are using the async/await syntax to make our code more readable so it creates some nuances. An eslint rule disallows awaits in loops, which makes sense to me.

if I use the following code, I will still only get one error even if both clients in the array don't exist.

let connection = null;
try {
  connection = await pool.getConnection();

  // accept a single client or client array
  const clients = Array.isArray(iClient) ? iClient : [iClient];

  const promises = [];
  for (let i = 0; i < clients.length; i += 1) {
    const client = clients[i];
    const bindDefs = {
      id: { dir: oracledb.BIND_INOUT, type: oracledb.NUMBER, val: client.id },
      address: { dir: oracledb.BIND_INOUT, type: oracledb.STRING, val: client.address, maxSize: 200 },
      username: { dir: oracledb.BIND_INOUT, type: oracledb.STRING, val: client.username, maxSize: 200 },
      service: { dir: oracledb.BIND_INOUT, type: oracledb.STRING, val: client.service, maxSize: 200 },
      type: { dir: oracledb.BIND_INOUT, type: oracledb.STRING, val: client.type, maxSize: 200 },
      comments: { dir: oracledb.BIND_INOUT, type: oracledb.STRING, val: client.comments, maxSize: 200 },
      updatedBy: { dir: oracledb.BIND_INOUT, type: oracledb.STRING, val: client.updatedBy, maxSize: 200 },
      group: { dir: oracledb.BIND_INOUT, type: oracledb.STRING, val: client.group, maxSize: 200 }
    };

    promises.push(connection.execute(UPDATE_CLIENT, bindDefs, {}));
  }

  return await Promise.all(promises);
} catch (error) {
  throw dbError(error);
} finally {
  if (connection) {
    await connection.close();
  }
}

The result if both client ids are invalid. So it creates. the same problem trying to get information to the user.

ORA-20001: Client does not exist and cannot be updated. id=12345
ORA-06512: at "REG.REGISTRATION_P", line 96 
ORA-06512: at "REG.REGISTRATION_P", line 261 
ORA-06512: at line 1 

@wattry
Copy link
Author

wattry commented Mar 24, 2020

I should add that if it succeeds, it will also create another small annoyance. The output will be an array which will need to be iterated over to remove the outBinds before it is returned.

[ { outBinds: { ... }  }, { outBinds: { ... } }  ]

@dmcghan
Copy link

dmcghan commented Mar 24, 2020

  1. I worry some when I see code like return await Promise.all(promises); with code that uses this driver (or any other module that uses the thread pool). A connection can only do one thing at a time, so while this code looks like it's doing things in parallel, it's not. It is, however, the type of code that can lead to issues.

    Rather than do a promise.push, you'd be best off simply running the execute in the loop. You could even wrap the call in a try/catch to catch the result of the operation and add the id/pk to an "errors array" as you described in your request.

    See this for more info.

  2. The problem with the current implementation and what I described above is that both involve a round trip for every execute call. The more iClient elements you're passed, the worse the performance will be. What you want to do is reduce the number of round trips by making fewer calls to execute. This is what executeMany was designed to do. However, because you're calling a PL/SQL stored procedure rather than executing SQL AND because you have a special requirement for error handling, you're likely best using execute with arrays.

    You'll pass execute one array for each field rather than scalar values. Once the arrays have been transferred, you can execute an anonymous PL/SQL block that goes into a loop and invokes the stored procedure as needed. You can save the results in "success" and "error" arrays and transfer the results back out as an "out bind" arrays.

    See this section of the doc to get the idea and this blog post for ideas.

Let me know if you have questions or still need help after reading the links. If so, I may be able to put together an example, but I'm not sure exactly when.

@wattry
Copy link
Author

wattry commented Mar 24, 2020

@dmcghan thanks for taking the time to reply.

  1. That is how our initial implementation was written, however we added ESlinting and got a whole new series of errors. However, with your comment in mind, I can argue that we can move back to loosen that rule.

  2. I'll give that a try.

Thanks again.

@dmcghan
Copy link

dmcghan commented Mar 24, 2020

I'd be curious to know what you saw in ESLint that caused you to make the change...

@wattry
Copy link
Author

wattry commented Mar 24, 2020

@dmcghan
Copy link

dmcghan commented Mar 24, 2020

Wow, thanks for pointing that out, it may explain why we see this from time to time. :)

@cjbj
Copy link
Member

cjbj commented Mar 25, 2020

I will explicitly mention that rule in the manual section which advises against using Promise.all.

There was some internal discussion lead by @anthony-tuininga and I opened an Oracle enhancement request 31077203 against the underlying Oracle libraries. I've also noted your request in my big node-oracledb todo list.

@cjbj
Copy link
Member

cjbj commented Mar 25, 2020

@09wattry one question: do you want to capture just the top level exceptions from each execution of the PL/SQL block, or do you want errors from individual SQL statements executed in each block?

@wattry
Copy link
Author

wattry commented Mar 25, 2020

@cjbj, honestly, I'm impressed with the responsiveness of this team. I believe that the PL/SQL is used to create custom errors and validation rules so the top level exceptions would be more interesting in our use cases.

@cjbj
Copy link
Member

cjbj commented Mar 25, 2020

@09wattry thank you and thank you.

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

No branches or pull requests

4 participants