Re: SQL/JSON: JSON_TABLE

For new files introduced in the patches:

+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group

2021 is a few days ahead. It would be good to update the year range.

For transformJsonTableColumn:

+ jfexpr->op =
+ jtc->coltype == JTC_REGULAR ? IS_JSON_VALUE :
+ jtc->coltype == JTC_EXISTS ? IS_JSON_EXISTS : IS_JSON_QUERY;

Should IS_JSON_EXISTS be mentioned in the comment preceding the method ?

For JsonTableDestroyOpaque():

+ state->opaque = NULL;

Should the memory pointed to by opaque be freed ?

+ l2 = list_head(tf->coltypes);
+ l3 = list_head(tf->coltypmods);
+ l4 = list_head(tf->colvalexprs);

Maybe the ListCell pointer variables can be named corresponding to the
lists they iterate so that the code is easier to understand.

+typedef enum JsonTablePlanJoinType
+{
+ JSTP_INNER = 0x01,
+ JSTP_OUTER = 0x02,
+ JSTP_CROSS = 0x04,

Since plan type enum starts with JSTP_, I think the plan join type should
start with JSTPJ_ or JSTJ_. Otherwise the following would be a bit hard to
read:

+ else if (plan->plan_type == JSTP_JOINED)
+ {
+ if (plan->join_type == JSTP_INNER ||
+ plan->join_type == JSTP_OUTER)

since different fields are checked in the two if statements but the
prefixes don't convey that.

+ Even though the path names are not incuded into the <literal>PLAN
DEFAULT</literal>

Typo: incuded -> included

+ int nchilds = 0;

nchilds -> nchildren

+#if 0 /* XXX it' unclear from the standard whether root path name is
mandatory or not */
+ if (plan && plan->plan_type != JSTP_DEFAULT && !rootPathName)

Do you plan to drop the if block ?

Cheers

On Fri, Dec 25, 2020 at 12:32 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
wrote:

>
> On 03.08.2020 10:55, Michael Paquier wrote:
>
> On Sun, Jul 05, 2020 at 12:15:58PM -0500, Justin Pryzby wrote:
>
> It looks like this needs to be additionally rebased - I will set cfbot to
> "Waiting".
>
> ... Something that has not happened in four weeks, so this is marked
> as returned with feedback. Please feel free to resubmit once a rebase
> is done.
> --
> Michael
>
>
> Atatched 44th version of the pacthes rebased onto current master
> (#0001 corresponds to v51 of SQL/JSON patches).
>
>
> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>