On 2020/02/29 0:46, Hamid Akhtar wrote: > The following review has been posted through the commitfest application: > make installcheck-world: not tested > Implements feature: not tested > Spec compliant: not tested > Documentation: not tested > > First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it is still better to fix this problem. > > I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on with detailed testing.
Thanks for the review and comments!
> 1. pgindent is showing a few issues with formatting. Please have a look and resolve those.
Yes.
> 2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables.
Basically I don't want to use the same variable for several purposes because which would decrease the code readability.
> Also, I would choose a more appropriate name for "tmp" variable.
Yeah, so what about "rest" as the variable name?
> I believe if you move the following lines before the conditional statement and simply and change the if statement to "if (len >= sizeof(buf) - 1)", it will serve the purpose.
ISTM that this doesn't work correctly when the "buf" contains trailing carriage returns but not newlines (i.e., this line is too long so the "buf" doesn't include newline). In this case, pg_strip_crlf() shorten the "buf" and then its return value "len" should become less than sizeof(buf). So the following condition always becomes false unexpectedly in that case even though there is still rest of the line to eat.
Per code comments for pg_strip_crlf:
"pg_strip_crlf -- Remove any trailing newline and carriage return"
If the buf read contains a newline or a carriage return at the end, then clearly the line
is not exceeding the sizeof(buf). If alternatively, it doesn't, then pg_strip_crlf will have
no effect on string length and for any lines exceeding sizeof(buf), the following conditional
