Conversation
Now correctly handles empty (len=0) parameter length..
For text parameters (format=0), respect paramLengths[i] if provided, otherwise fall back to strlen().
Summary of ChangesHello @rahim-kanji, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an "Invalid UTF-8 byte sequence error" that occurred when a text parameter was immediately followed by a null parameter during data relay. The core issue stemmed from Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with handling PostgreSQL parameters, particularly when mixing text and null types, which could lead to UTF-8 errors. The fix involves two main parts: patching the underlying libpq to correctly use provided parameter lengths for text-formatted data, and correcting a potential overflow and pointer arithmetic bug in ProxySQL's own protocol parser for extended query messages. A regression test is also added to validate the fix.
My review identifies a bug in the new regression test that involves an out-of-bounds column access, and a potential logic issue in the parsing code that could conflate empty strings with NULL values. Both issues come with suggestions for fixes.
| bool isnull_c2 = PQgetisnull(res, 0, 2); | ||
|
|
||
| ok(std::string(c1) == "ABCDEFGHIJKLMN", "col1 length 14 parsed correctly"); | ||
| ok(isnull_c2, "col3 NULL parsed correctly"); |
There was a problem hiding this comment.
There appears to be an out-of-bounds access in the test. The test table reg_test_5140 has two columns, col1 and col2, so valid column indices are 0 and 1. However, PQgetisnull(res, 0, 2) attempts to access the column with index 2, which does not exist. This should be 1 to check col2. Additionally, the test message in the ok() call refers to col3, which is also incorrect.
| bool isnull_c2 = PQgetisnull(res, 0, 2); | |
| ok(std::string(c1) == "ABCDEFGHIJKLMN", "col1 length 14 parsed correctly"); | |
| ok(isnull_c2, "col3 NULL parsed correctly"); | |
| bool isnull_c2 = PQgetisnull(res, 0, 1); | |
| ok(std::string(c1) == "ABCDEFGHIJKLMN", "col1 length 14 parsed correctly"); | |
| ok(isnull_c2, "col2 NULL parsed correctly"); |
8b9ed25 to
c4a9e40
Compare
c4a9e40 to
ee4cd98
Compare
|
Closes #5140