On Thu, Jan 21, 2021 at 6:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Simon Riggs <simon.riggs@enterprisedb.com> writes: > > JSON parsing reports the line number and relevant context info > > incorrectly when the JSON contains newlines. Current code mostly just > > says "LINE 1" and is misleading for error correction. There were no > > tests for this previously. > > Couple thoughts: > > * I think you are wrong to have removed the line number bump that > happened when report_json_context advances context_start over a > newline. The case is likely harder to get to now, but it can still > happen can't it? If it can't, we should remove that whole stanza.
OK, I'm playing around with this to see what is needed.
> * I'd suggest naming the new JsonLexContext field "pos_last_newline"; > "linefeed" is not usually the word we use for this concept. (Although > actually, it might work better if you make that point to the char > *after* the newline, in which case "last_linestart" might be the > right name.)
Yes, OK
> * I'm not enthused about back-patching. This behavior seems like an > improvement, but that doesn't mean people will appreciate changing it > in stable branches.
OK, as you wish.
Thanks for the review, will post again soon with an updated patch.
I agree with Tom's feedback.. Whether you change pos_last_linefeed to point to the character after the linefeed or not, we can still simplify the for loop within the "report_json_context" function to:
=================
context_start = lex->input + lex->pos_last_linefeed; context_start += (*context_start == '\n'); /* Let's move beyond the linefeed */ context_end = lex->token_terminator; line_start = context_start; while (context_end - context_start >= 50 && context_start < context_end) { /* Advance to next multibyte character */ if (IS_HIGHBIT_SET(*context_start)) context_start += pg_mblen(context_start); else context_start++; } =================
IMHO, this should work as pos_last_linefeed points to the position of the last linefeed before the error occurred, hence we can safely skip it and move the start_context forward.