From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, Erik Rijkers <er(at)xs4all(dot)nl>, Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>, David Steele <david(at)pgmasters(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Michael Paquier <michael(at)paquier(dot)xyz>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Oleg Bartunov <obartunov(at)gmail(dot)com>, Zhihong Yu <zyu(at)yugabyte(dot)com>
Date: 2021-07-26 14:12:41
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Below are a few small comments from a casual read-through. I noticed that
there was a new version posted after I had finished perusing, but it seems to
address other aspects.

+ Gerenates a column and inserts a composite SQL/JSON

+ into both child and parrent columns for all missing values.

- objectname = "xmltable";
+ objectname = rte->tablefunc ?
+ rte->tablefunc->functype == TFT_XMLTABLE ?
+ "xmltable" : "json_table" : NULL;
In which case can rte->tablefunc be NULL for a T_TableFuncScan? Also, nested
ternary operators are confusing at best, I think this should be rewritten as
plain if statements.

In general when inspecting functype I think it's better to spell it out with if
statements rather than ternary since it allows for grepping the code easier.
Having to grep for TFT_XMLTABLE to find json_table isn't all that convenient.
That also removes the need for comments stating why a ternary operator is Ok in
the first place.

+ errmsg("JSON_TABLE() is not yet implemented for json type"),
I can see this being potentially confusing to many, en errhint with a reference
to jsonb seems like a good idea.

+/* Recursively register column name in the path name list. */
+static void
+registerJsonTableColumn(JsonTableContext *cxt, char *colname)
This comment is misleading since the function isn't actually recursive, but a
helper function for a recursive function.

+ switch (get_typtype(typid))
+ {
+ return true;
+ return typeIsComposite(getBaseType(typid));
+ }
switch statements without a default runs the risk of attracting unwanted
compiler warning attention, or make static analyzers angry. This one can
easily be rewritten with two if-statements on a cached get_typtype()

+ * Returned false at the end of a scan, true otherwise.
s/Returned/Returns/ (applies at two places)

+ /* state->ordinal--; */ /* skip current outer row, reset counter */
Is this dead code to be removed, or left in there as a reminder to fix

Daniel Gustafsson

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2021-07-26 14:13:09 Re: when the startup process doesn't (logging startup delays)
Previous Message Masahiko Sawada 2021-07-26 14:01:46 Re: [PoC] Improve dead tuple storage for lazy vacuum