On Fri, Sep 30, 2016 at 10:12 PM, Daniel Verite <dan...@manitou-mail.org> wrote: > Tomas Vondra wrote: > >> A few minor comments regarding the patch: >> >> 1) CopyStartSend seems pretty pointless - It only has one function call >> in it, and is called on exactly one place (and all other places simply >> call allowLongStringInfo directly). I'd get rid of this function and >> replace the call in CopyOneRowTo(() with allowLongStringInfo(). >> >> 2) allowlong seems awkward, allowLong or allow_long would be better >> >> 3) Why does resetStringInfo reset the allowLong flag? Are there any >> cases when we want/need to forget the flag value? I don't think so, so >> let's simply not reset it and get rid of the allowLongStringInfo() >> calls. Maybe it'd be better to invent a new makeLongStringInfo() method >> instead, which would make it clear that the flag value is permanent. >> >> 4) HandleParallelMessage needs a tweak, as it uses msg->len in a log >> message, but with '%d' and not '%ld' (as needed after changing the type >> to Size). >> >> 5) The comment at allowLongStringInfo talks about allocLongStringInfo >> (i.e. wrong function name). > > Here's an updated patch. Compared to the previous version: > > - removed CopyStartSend (per comment #1 in review) > > - renamed flag to allow_long (comment #2) > > - resetStringInfo no longer resets the flag (comment #3). > > - allowLongStringInfo() is removed (comment #3 and #5), > makeLongStringInfo() and initLongStringInfo() are provided > instead, as alternatives to makeStringInfo() and initStringInfo(). > > - StringInfoData.len is back to int type, 2GB max. > (addresses comment #4 incidentally). > This is safer because many routines peeking > into StringInfoData use int for offsets into the buffer, > for instance most of the stuff in backend/libpq/pqformat.c > Altough these routines are not supposed to be called on long > buffers, this assertion was not enforced in the code, and > with a 64-bit length effectively over 2GB, they would misbehave > pretty badly.
The patch status is "Waiting on Author" in the CF app, but a new patch has been sent 2 days ago, so this entry has been moved to next CF with "Needs Review". -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
