A bunch of additional comments, after looking at the patch a bit today.
All are mostly minor, and sometime perhaps a matter of preference.
1) There's a mismatch between the comment and actual function name for
jsonb_path_match_opr and jsonb_path_exists_opr(). The comments say
"_novars" instead.
2) In a couple of switches the "default" case does a return with a
value, following elog(ERROR). So it's practically unreachable, AFAICS
it's fine without it, and we don't do this elsewhere. And I don't get
any compiler warnings if I remove it either.
Examples:
JsonbTypeName
default:
elog(ERROR, "unrecognized jsonb value type: %d", jbv->type);
return "unknown";
jspOperationName
default:
elog(ERROR, "unrecognized jsonpath item type: %d", type);
return NULL;
compareItems
default:
elog(ERROR, "unrecognized jsonpath operation: %d", op);
return jpbUnknown;
3) jsonpath_send is using makeStringInfo() for a value that is not
returned - IMHO it should use regular stack-allocated variable and use
initStringInfo() instead
4) the version number should be defined/used as a constant, not as a
magic constant somewhere in the code
5) Why does jsonPathToCstring do this?
appendBinaryStringInfo(out, "strict ", 7);
Why not to use regular appendStringInfoString()? What am I missing?
6) comment typo: "Aling StringInfo"
7) alignStringInfoInt() should explain why we need this and why INTALIGN
is the right alignment.
8) I'm a bit puzzled by what flattenJsonPathParseItem does with 'next'
I don't quite understand what it's doing with 'next' value?
/*
* Actual value will be recorded later, after next and children
* processing.
*/
appendBinaryStringInfo(buf,
(char *) &next, /* fake value */
sizeof(next));
Perhaps a comment explaining it (why we need a fake value at all?) would
be a good idea here.
9) I see printJsonPathItem is only calling check_stack_depth while
flattenJsonPathParseItem also calls CHECK_INTERRUPTS. Why the
difference, considering they seem about equally expensive?
10) executeNumericItemMethod is missing a comment (unlike the other
executeXXX functions)
11) Wording of some of the error messages in the execute methods seems a
bit odd. For example executeNumericItemMethod may complain that it
... is applied to not a numeric value
but perhaps a more natural wording would be
... is applied to a non-numeric value
And similarly for the other execute methods. But I'm not a native
speaker, so perhaps the original wording is just fine.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
