On Fri, Sep 30, 2016 at 10:12 PM, Daniel Verite <daniel@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