vef run query#243
Conversation
dbentley-vsql
left a comment
dbentley-vsql
left a comment
There was a problem hiding this comment.
Where is this on the capabilities that Adam put together?
| typedef struct { | ||
| bool is_null; | ||
| const char *str; // null-terminated; valid only during the row callback | ||
| size_t str_len; |
There was a problem hiding this comment.
Worth commenting:
if str is empty, because it's null-terminated, that means that str[0] == 0, correct? Is str_len == 0 or str_len == 1 ? I.e., is it the length of the string or the memory?
There was a problem hiding this comment.
str_len is the string length — i.e. strlen(str), not including the null terminator. Clarified in the comment: // number of bytes in str, not including the null terminator (i.e. str_len == strlen(str)); 0 when
is_null is true
| // ctx: opaque pointer forwarded to meta_cb and row_cb. | ||
| // error_msg: on VEF_QUERY_ERROR, a null-terminated description is written | ||
| // here; must point to a buffer of at least VEF_MAX_ERROR_LEN | ||
| // bytes; may be NULL if the caller does not need the message. |
There was a problem hiding this comment.
Tiniest of nits: if it may be NULL, then it is not the case that it "must point to a buffer"
There was a problem hiding this comment.
Good catch. Fixed to: "if non-NULL, must point to a buffer of at least VEF_MAX_ERROR_LEN bytes".
| inline vef_run_query_result_t run_query(std::string_view sql, | ||
| RunQueryMetaCb meta_cb, | ||
| RunQueryRowCb row_cb, | ||
| char *error_msg = nullptr) { |
There was a problem hiding this comment.
This is a header-only implementation (the function body lives in the .h file). inline is required to avoid ODR violations when the header is included in multiple translation units. Same pattern as keyring.h and
sys_var_builder.h in the SDK. Added a comment to explain this.
| inline vef_run_query_result_t run_query(std::string_view sql, | ||
| RunQueryMetaCb meta_cb, | ||
| RunQueryRowCb row_cb, | ||
| char *error_msg = nullptr) { |
There was a problem hiding this comment.
Why allow nullptr error_msg? Won't this encourage people to not check status?
There was a problem hiding this comment.
The return value (VEF_QUERY_OK/VEF_QUERY_ERROR/VEF_QUERY_ABORTED) is always available regardless, so callers can check the result without needing a message buffer. nullptr is mainly for the no-callbacks
convenience overload (e.g. run_query("SET SESSION sql_mode = ''")) where the caller just wants to know success/failure. The error description is bonus detail. That said, the doc comment now makes clear that
callers should still check the return value.
| } | ||
|
|
||
| // Propagate the caller's current database into the admin session. | ||
| if (current_thd != nullptr) { |
There was a problem hiding this comment.
Two questions:
-
This makes sense if it's called from within a UDF, but what if it's called in some other context...
IDK enough about current_thd and how it's maintained. -
Does it always make sense to thread through the DB? Are there any analogous cases to guide?
There was a problem hiding this comment.
You're right to flag this. run_query is intended for use from background threads, not from inside VDFs — calling SQL from within a SQL execution context is generally unsafe. We should add a clear warning to the
documentation that calling run_query from a VDF is unsupported. The current_thd propagation was added to make the test work, but the test itself demonstrates the wrong usage pattern. We should rewrite the test
to call run_query from a background thread, and update the API docs accordingly.
tomas-villagesql
force-pushed
the
tomas/vef-run-query
branch
2 times, most recently
from
1596f61 to
a02480c
Compare
|
I think this is an extension that's worth more of a review of the surface area we're adding that we'll have to support, and is worth moving to review a doc instead of a specific implementation. E.g., why support just this case instead of the broader case? |
tomas-villagesql
force-pushed
the
tomas/vef-run-query
branch
from
a02480c to
655e704
Compare
2 participants
No description provided.