bucardo / dbdpg

Perl Postgres driver DBD::Pg aka dbdpg
48 stars 35 forks source link

Result length fix 1 #73

Closed jonjensen closed 4 years ago

jonjensen commented 4 years ago

@turnstep Here's a new unit test that shows the length() bug and in a separate commit, one way I fixed it.

It seems like the string's length should be getting set here during each loop iteration:

sv_setpvn(sv, (char *)value, value_len);

Based on the earlier:

typeinfo->dequote(aTHX value, &value_len);

And dequote_string sure looks like it sets *value_len.

Something seems to be getting recycled behind the scenes.

perlguts(1) says:

The "sv_set*()" functions are not generic enough to operate on values that have "magic". See "Magic Virtual Tables" later in this document.

Also note that the "sv_set()" and "sv_cat()" functions described earlier do not invoke 'set' magic on their targets. This must be done by the user either by calling the "SvSETMAGIC()" macro after calling these functions, or by using one of the "sv_set_mg()" or "sv_cat_mg()" functions.

And that document always shows SvUTF8_off(sv); and SvUTF8_on(sv); followed immediately by SvSETMAGIC(sv);

Because of this?

PERL_MAGIC_utf8           vtbl_utf8      Cached UTF-8 information

So that's what I did and it works. That is done a few times in Pg.xs but seems like it may be needed in more places if this is the right fix.

I made another pull request in https://github.com/bucardo/dbdpg/pull/74 that solves the problem a different way, which may be more efficient, but I wonder if it may miss something because it wouldn't get run on all code paths, especially for future UTF-8 capable types.

Anyway, let me know what you think.

turnstep commented 4 years ago

Thanks for this @jonjensen - I like this pull version better. Matter of fact, I think we can move the SvSETMAGIC up a line so it is only called when we set call UTF8_on on the svm which seems to confuse things. It looks like without that magic, MG_LEN gets set once and stays sticky even though the sv is re-used. Once we tell Perl it is magic, MG_LEN goes to -1, which is what we want, as it forces things like length() to look it up itself, rather than rely on a bogus cached value.

Will look at this more later when I have more time, just wanted to drop some quick feedback.

jonjensen commented 4 years ago

@turnstep Makes sense. Thanks for the note!

turnstep commented 4 years ago

Closing - work has been released in version 3.13.0