Re: psql tab completion enhancements - Mailing list pgsql-patches

Neil,

thanks for your review. I accepted what you wrote for all items I don't
mention in this reply.

On Sat, Jan 07, 2006 at 10:59:27PM -0500, Neil Conway wrote:
> > + #define COMPLETE_WITH_MALLOCED_LIST(list) \
> > +       do { COMPLETE_WITH_LIST((const char**) list); free(list); list
> > = (char**) 0; } while(0)

> NULL is better style than 0 in a pointer context. Also, why are the
> casts necessary?

The cast "(const char**) list" is necessary because completion_charpp is
declared to be const. Removing the const means that you have to remove it
in a lot of other places as well, do you think this would be cleaner?


> > !               /* if we have only seen LOCK but not LOCK TABLE so
> > far, offer
> > !                * the TABLE keyword as well */

> Is this actually useful? The "TABLE" keyword is just noise.

Yeah, but it is valid SQL, so why not support it? I got used to use long
forms sometimes and get confused with psqls completion not supporting this
sometimes, for example in:

template1=# alter TABLE test ALTER column <TAB>
DROP DEFAULT    SET DEFAULT     SET STATISTICS  TYPE
DROP NOT NULL   SET NOT NULL    SET STORAGE



> > + static char**
> > + get_query_list(const char *text,
> > +                          const char *query,
> > +                          const char* completion_info)
> > + {
> > +       return _get_query_list(0, text, query, completion_info);
> > + }

> The difference between get_query_list() and _get_query_list() is not
> reflected in the names of the methods. An "_internal" suffix would be
> better, for example.

There are

static char *complete_from_query(const char *text, int state);
static char *complete_from_schema_query(const char *text, int state);
static char *_complete_from_query(int is_schema_query,
                     const char *text, int state);

already, so I made analogous versions for

static char **get_query_list(const char *text, const char *query,
                             const char *completion_info);
static char **get_schema_query_list(const char *text, const SchemaQuery* squery,
                                    const char *completion_info);
static char **_get_query_list(int is_schema_query,
                              const char *text, const char *query,
                              const char *completion_info);


> "bool" for boolean variables rather than "int", and consistent parameter
> declarations ("char *" not "char*").

See above, I used the "int is_schema_query" because of
_complete_from_query().

I could change both to bool in a new patch if you want to?


> > + static char**
> > + list_add_item(char **list, char *item)
> > + {
> > +       int size = 0;
> > +       while (list[size])
> > +               size++;
> > +       list = realloc(list, sizeof(char*) * (size + 1 + 1));
> > +       list[size] = item;
> > +       list[size+1] = NULL;
> > +       return list;
> > + }

> That's a terribly inefficient implementation.

It's not terribly inefficient, it just scales terribly. However psql limits
its queries to return 1000 items at most. At this size even a PII 300 adds
1000 items in

:/tmp$ time ./test
real    0m0.007s
user    0m0.000s
sys     0m0.000s

Program startup overhead for this example and doing the loop without any
work takes 0m0.004s already.

If the consensus is that it has to be better, I'll supply another version,
but I thought that it's not necessary.


Joachim