Binding output one-character parameter

[reflexator]

Hi,

reposting reported not solved bug https://rt.cpan.org/Public/Bug/Display.html?id=94232

[mjegh]

I really have no time to look at this right now. Is it all possible you could look at the diffs between 1.68 and 1.70 and try and highlight the culprit. Or perhaps someone else can do that.

[yanick]

I tried it on my machine, and there are trailing spaces for v1.68 as well. I went down to v1.58, and witnessed the spaces there too.

[yanick]

Okay, commit 6aa9541e0a8ca42a72e761e9603803efd2d9f396 seems to be causing problems.

Before the commit, we get '1 ' instead of '1', but after, we get ' '. Which I'd say is worse. :-)

[mjegh]

OK, this is obviously my fault. I was attempting to fix RT91698 (which I did) but it obviously had unintended consequences. The diff looks a lot larger that it really is as I added comments and some tracing. All it resolves down to is incrementing maxlen in bind code rather than the rebind code because incrementing in the latter increases the buffer every time you run execute. I cannot as yet see how this has caused the reported problem.

[mjegh]

And this makes it work again but reintroduces tRT91698:

diff --git a/dbdimp.c b/dbdimp.c
index a94cbe5..7c159ae 100644
--- a/dbdimp.c
+++ b/dbdimp.c
@@ -3289,7 +3289,7 @@ dbd_bind_ph(SV *sth, imp_sth_t *imp_sth, SV *ph_namesv, SV *newvalue, IV sql_typ
         }
         /* Add space for NUL - do it now rather than in rebind as it cause problems
            in rebind where maxlen continually grows. */
-        phs->maxlen = phs->maxlen + 1;
+        /*phs->maxlen = phs->maxlen + 1;*/
        }

        return dbd_rebind_ph(sth, imp_sth, phs);
diff --git a/oci8.c b/oci8.c
index 46a69e8..0249c3f 100644
--- a/oci8.c
+++ b/oci8.c
@@ -1226,7 +1226,7 @@ dbd_phs_out(dvoid *octxp, OCIBind *bindp,
                                sv_setpv(sv,"");
                }

-        *bufpp = SvGROW(sv, (size_t)(((phs->maxlen < 28) ? 28 : phs->maxlen)));
+        *bufpp = SvGROW(sv, (size_t)(((phs->maxlen < 28) ? 28 : phs->maxlen+1)));
                phs->alen = SvLEN(sv);  /* max buffer size now, actual data len later */

        }

[mjegh]

ok, I see what is happening. The original logic is flawed but works in this case. It grows the buffer to 16 when the buffer is 0 then adds 1 for the trailing null chr. Then when you run execute again, rebind adds 1 again and so on so it grows repeatedly.

The original change I made stops the buffer growing but breaks the rebind code which now the buffer is 1, it fails to grow it to 16 so there is no room for a single chr + a null chr.

This is a PITA.

[mjegh]

I think I see the problem now. I think it is simply that the phs->maxlen+1 needs removing from dbd_phs_out however, this would need some substantial testing. The problem seems to be by incrementing maxlen at the end of the bind code (as I originally did) OCIBindByName gets called with a non zero valuep (which is incorrect in the case of out params). I made this change and ran the test code and nothing failed and I believe it also keeps the original RT fixed.

I'm not at all sure why the '1' comes back as '1 ' (one followed by 2 spaces) but it did that before this change too so that is a different issue.

I'd suggest a test is added as per the provided example code but with examples which return '1' with a buffer of size 1, then '11' with a size of 2, '111' with a size of 3 etc at least up to 17 chrs (I say 17 as SvGROW seems to default to 16). Obviously they should fail right now but when commit 6aa9541 is reverted (which the change below does) and the change as per below is applied I'd hope they work and I'll try and verify the original RT i was trying to fix is also fixed.

diff --git a/dbdimp.c b/dbdimp.c
index a94cbe5..6d7684f 100644
--- a/dbdimp.c
+++ b/dbdimp.c
@@ -3287,9 +3287,6 @@ dbd_bind_ph(SV *sth, imp_sth_t *imp_sth, SV *ph_namesv, SV *newvalue, IV sql_typ

             phs->sv = SvREFCNT_inc(newvalue);  /* point to live var    */
         }
-        /* Add space for NUL - do it now rather than in rebind as it cause problems
-           in rebind where maxlen continually grows. */
-        phs->maxlen = phs->maxlen + 1;
        }

        return dbd_rebind_ph(sth, imp_sth, phs);

[mjegh]

BTW, if you don't specify ora_type => ORA_CHAR (which I don't think you need to anyway) it works before and after the change I originally made.

[yanick]

Just want to say that you, my friend, are an OCI/XS wizard. Hat off to you.

[mjegh]

Thanks Yanick but in this case I balls it up in the first place. I've written a lot of C in my past life (don't do much these days) but nothing prepares you for Perl C/XS internal macros or the complexity of DBD::Oracle with its many contributors over time, massively nested code, lack of comments and use of OCI which is a "magic" API that is constantly surprising me.

If only I had more time I might be able to improve things but the reality is that any refactoring of this code is bound to break things in the short term so in this case it is probably better to leave things as they are and provide fixes on the edge when they seem important enough. I seriously doubt that anyone is going to get to grips with this codeset in any meaningful way in the future as it contains too many special cases that are undocumented. So, we tweak on the edges for now.