Skip to content

Conversation

@ucko
Copy link
Contributor

@ucko ucko commented May 30, 2024

... i.e., of VARCHAR(MAX) and the like. Split from #555.

... i.e., of VARCHAR(MAX) and the like.

Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>
@freddy77
Copy link
Contributor

That's... interesting! Tests passes with both this commit and without, so I suppose this path it's not really tested. Worth to write a test, and check TDS documentation!

@ucko
Copy link
Contributor Author

ucko commented May 31, 2024

An analogous patch to 1.4 has been helpful for some of my usage. However, when I locally extended ctlib's long_binary test to incorporate BCP, I found that it succeeded under TDS 7.2+ only without this change. I'll take a closer look and get back to you.

@ucko
Copy link
Contributor Author

ucko commented May 31, 2024

AFAICT, whether this patch is helpful or harmful depends on runtime context TBD; I'll try to identify the relevant condition and adjust it accordingly.

@freddy77
Copy link
Contributor

freddy77 commented Jun 1, 2024

TDS protocol documentation seems to indicate PLP length is needed. Yes, it would be useful to extend a test to catch this. I vaguely remember this specific code (I wrote it) and it was kind of a bug in a specific server version, or a combination of server and protocol version. Wondering why I didn't write a test at that time.

@freddy77
Copy link
Contributor

freddy77 commented Jun 1, 2024

Added a test, see 3006113. Looks like it's working with current code, not with this patch applied.

@freddy77
Copy link
Contributor

freddy77 commented Jun 3, 2024

Could it be that this commit was here to fix an old bug handling empty strings?

@ucko
Copy link
Contributor Author

ucko commented Jun 3, 2024

Aha, the supporting logic for blk_textxfer in #558 doesn't account for possibly needing to send a final PLP length of zero. I'll fix that instead.

@ucko
Copy link
Contributor Author

ucko commented Jun 18, 2024

Withdrawn in favor of #558.

@ucko ucko closed this Jun 18, 2024
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.

2 participants