Most grateful for you input tye++ - this is why I love perlmonks. I obviously need to work on this more before releasing it.

Thanks again tye. I ended up with the following:

SV *
odbc_lob_read(sth, colno, bufsv, length, attr = NULL)
     SV  *sth
     int colno
     SV  *bufsv
     UV  length
     SV  *attr;
    PROTOTYPE: $$$$;$
    PREINIT:
     char *buf;
     UV   ret_len;
     IV   sql_type = 0;
    INIT:
     if (length == 0) {
         croak("Cannot retrieve 0 length lob");
     }
    CODE:
     if (attr) {
         SV **svp;
         DBD_ATTRIBS_CHECK("odbc_lob_read", sth, attr);
         DBD_ATTRIB_GET_IV(attr, "Type", 4, svp, sql_type);
     }
     if (SvROK(bufsv)) {
        bufsv = SvRV(bufsv);
     }
     sv_setpvn(bufsv, "", 0);                   /* ensure we can grow 
+ok */

     buf = SvGROW(bufsv, length + 1);
     ret_len = odbc_st_lob_read(sth, colno, bufsv, length, sql_type);
     if (ret_len > 0) {
         SvCUR_set(bufsv, ret_len);      /* set length in SV */
         *SvEND(bufsv) = '\0';           /* NUL terminate */
         SvSETMAGIC(bufsv);
         RETVAL = newSViv(ret_len);
     } else {
         XSRETURN_UNDEF;
     }
     OUTPUT:
        RETVAL
[download]

Thanks again for your suggestions.

I encourage you to replace if (ret_len > 0) { with if (ret_len >= 0) {.

Your code as written causes undef to be returned on EOF (I believe). Although there is prior art for this in Perl, it also means that your callers can't distinguish between EOF and an error. A lot of the time this doesn't matter (obviously, or else Perl would have changed this a long time ago).

But sometimes it is important to distinguish between EOF and an error. This is especially true when reading from a socket (which I assume is the typical case for something like DBD::pg). (This is another reason why one shouldn't use <$SOCK> with sockets.)

If you use <= 0, then the easy case is no more difficult (you just read until you get a false value: 0 or undef). But a caller can go to a little extra effort to detect undef and report a failure -- including $! at the time of the failure which will likely explain why the read attempt failed, to some level. With the code as-is, you have quite effectively hidden whether an error has occurred and made it impossible to reliably report such errors.

Having tried to write tests around "errors while reading" in Perl, I know well how hard it is to try to guess that an error actually happened and how easy those difficult guesses end being wrong.

I would hope that something like DBD::pg would report read failures such that $! can explain what went wrong rather than having to guess what went wrong when an invalid (actually, incomplete) packet/response can't be parsed (or even risking that an incomplete response could still be parsed).

- tye        

tye thanks again for the comments. Originally I wanted to add a lob_read function to DBD::ODBC and started by looking at DBD::Pg for inspiration. My original post was because I thought the code in DBD::Pg (which I was copying) looked wrong. Just to clear things up, the last code I posted here is for DBD::ODBC and not DBD::Pg.

I understand your point on detecting the difference between errors and EOF and in general agree with you but in this case if an error occurs in odbc_st_lob_read it calls DBI's error registration which will lead to a die if RaiseError is in force. On the other hand if RaiseError is disabled you are correct, you cannot tell the difference between an error and EOF so I'll look at that.

Thanks for your helpful comments.