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

Show vector parameters values in error messages #1200

Merged
merged 12 commits into from
Feb 4, 2025

Conversation

vadz
Copy link
Member

@vadz vadz commented Jan 30, 2025

This addresses the problem of not knowing which vector element resulted in the error when using bulk operations. For now this is implemented for some backends only, but it's already better than nothing.

@vadz vadz force-pushed the error-msg-bulk-insert branch from bd3b395 to a4188c8 Compare January 30, 2025 18:29
vadz added 9 commits January 30, 2025 20:22
This was the only backend which didn't implement retrieving parameter
names by position, so this commit makes it possible to rely on this
working for all backends and will simplify writing upcoming tests.

Note that Oracle always returns names in upper case and there doesn't
seem to be any way to avoid it.
Check for the exact fragment expected and not just a single word.

Also use REQUIRE_THAT() instead of CAPTURE() + CHECK() combination, this
is simpler and provides the same information (arguably more clearly).
Check that the values of the vectors elements that resulted in the error
appear in the output too, at least for the backends that have been
already updated to do it (i.e. none yet, but some of them will be soon).
This requires backend cooperation, but it is now possible for a backend
to override get_row_to_dump() function to return the row whose values
would appear in the error message.

This commit is best viewed using Git --color-moved option.
Override get_row_to_dump() to return the row that was being processed
when an exception happened.
Refactor the code to replace all calls to OCIAttrGet(OCI_HTYPE_STMT)
with calls to the new function. This is simpler and shorter.

Note that this is still unsafe as the correspondence between the
attribute and the corresponding type is not checked, but it doesn't seem
worth to complicate things further as long as we're using only a handful
of attributes (and each of them is used only once).

This commit is best viewed ignoring whitespace-only changes.
This free function will be generalized to work with other OCI handles in
the next commit.

No real changes yet.
Use OCI_BATCH_ERRORS mode to retrieve information about the errors
during bulk operations and use it to show the parameter values for the
first failing row.

Also add a generally useful RAII helper wrapping OCIHandle{Alloc,Free}
and use it for temporary OCIError allocation.
As we currently don't use libpq support for bulk operations (via
pipeline mode), we simply have to remember the row which resulted in an
error, if any.
@vadz vadz force-pushed the error-msg-bulk-insert branch from a4188c8 to 2e5e8fc Compare January 30, 2025 19:22
vadz added 3 commits January 30, 2025 23:13
We already have this information there, provided by the status array, so
just override get_row_to_dump() to return the first error row.
Factor out get_last_row_count() from get_affected_rows() and use the new
function while performing the bulk operation, as it doesn't depend on
the value of rowsAffectedBulk_ and so we can update it safely.

No real changes, but the new version is more clear and robust.
This is similar to the other backends without special support for bulk
operations: just remember the current row during bulk operations, so
that we could refer to its values if an error happens.
@vadz vadz force-pushed the error-msg-bulk-insert branch from faa31cd to 0ea3281 Compare January 30, 2025 22:22
@vadz vadz marked this pull request as ready for review January 30, 2025 22:22
@vadz
Copy link
Member Author

vadz commented Jan 30, 2025

This is ready now, I've implemented this for all backends except DB/2 (which I can't test) and MySQL (which I could but don't use, so I'm leaving this for somebody who cares about it).

If there are no comments/objections, I'll merge this soon.

vadz added a commit that referenced this pull request Jan 30, 2025
Update to SOCI branch showing row values for errors during bulk
operations.

See #1200
@vadz vadz merged commit b7b470e into SOCI:master Feb 4, 2025
16 checks passed
@vadz vadz deleted the error-msg-bulk-insert branch February 4, 2025 14:17
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.

1 participant