Re: psql: \dl+ to list large objects privileges

Поиск
Список
Период
Сортировка
От Georgios Kokolatos
Тема Re: psql: \dl+ to list large objects privileges
Дата
Msg-id 163067194414.1167.8140856874411625212.pgcf@coridan.postgresql.org
обсуждение исходный текст
Ответ на Re: psql: \dl+ to list large objects privileges  (Pavel Luzanov <p.luzanov@postgrespro.ru>)
Ответы Re: psql: \dl+ to list large objects privileges  (Georgios Kokolatos <gkokolatos@protonmail.com>)
Re: psql: \dl+ to list large objects privileges  (Pavel Luzanov <p.luzanov@postgrespro.ru>)
Re: psql: \dl+ to list large objects privileges  (Pavel Luzanov <p.luzanov@postgrespro.ru>)
Список pgsql-hackers
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

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Amit Kapila
Дата:
Сообщение: Re: Added schema level support for publication.
Следующее
От: Georgios Kokolatos
Дата:
Сообщение: Re: psql: \dl+ to list large objects privileges