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
Hi,
thank you for the patch, I personally think it is a useful addition and thus it
gets my vote. However, I also think that the proposed code will need some
changes.
On a high level I will recommend the addition of tests. There are similar tests
present in:
./src/test/regress/sql/psql.sql
I will also inquire as to the need for renaming the function `do_lo_list` to
`listLargeObjects` and its move to describe.c. from large_obj.c. In itself it is
not necessarily a blocking point, though it will require some strong arguments
for doing so.
Applying the patch, generates several whitespace warnings. It will be helpful
if those warnings are removed.
The patch contains:
case 'l':
- success = do_lo_list();
+ success = listLargeObjects(show_verbose);
It might be of some interest to consider in the above to check the value of the
next character in command or emit an error if not valid. Such a pattern can be
found in the same switch block as for example:
switch (cmd[2])
{
case '\0':
case '+':
<snip>
success = ...
</snip>
break;
default:
status = PSQL_CMD_UNKNOWN;
break;
}
The patch contains:
else if (strcmp(cmd + 3, "list") == 0)
- success = do_lo_list();
+ success = listLargeObjects(false);
+
+ else if (strcmp(cmd + 3, "list+") == 0)
+ success = listLargeObjects(true);
In a fashion similar to `exec_command_list`, it might be interesting to consider
expressing the above as:
show_verbose = strchr(cmd, '+') ? true : false;
<snip>
else if (strcmp(cmd + 3, "list") == 0
success = do_lo_list(show_verbose);
Thoughts?
Cheers,
//Georgios
