Thread: psql tab completion enhancements
Hi, psql's tab completion has the following problem: If we have the following syntax for example: SET SESSION AUTHORIZATION <user>; SET SESSION AUTHORIZATION DEFAULT; After "SET SESSION AUTHORIZATION", the tab completion can offer a list of roles or the string constant "DEFAULT". However it can't offer both because it can't get a list of roles and add a string constant to this list. The appended patch adds the functionality of lists that can be extended with constants. Then you get: template1=# SET session AUTHORIZATION <tab> DEFAULT fred joe john I did proof-of-concept examples to add a constant to a - list from a query - list from a schema query - list of table attributes Joachim
Attachment
A few minor stylistic gripes...
On Fri, 2006-01-06 at 20:27 +0100, Joachim Wieland wrote:
> *** 150,155 ****
> --- 151,171 ----
> do {completion_charp = Query_for_list_of_attributes;
> completion_info_charp = table; matches = completion_matches(text,
> complete_from_query); } while(0)
>
> /*
> + * Keep the "malloced" keyword in all the names such that we
> remember that
> + * memory got allocated here. COMPLETE_WITH_MALLOCED_LIST frees this
> memory.
> + */
> + #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?
> /* Forward declaration of functions */
> + static char **get_empty_list();
Should be "static char **get_empty_list(void);", as C89 doesn't check
the parameters passed to a function declared with an empty parameter
list.
> *** 754,760 ****
> else if (pg_strcasecmp(prev3_wd, "TABLE") == 0 &&
> (pg_strcasecmp(prev_wd, "ALTER") == 0 ||
> pg_strcasecmp(prev_wd, "RENAME") == 0))
> ! COMPLETE_WITH_ATTR(prev2_wd);
>
> /* ALTER TABLE xxx RENAME yyy */
> else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
> --- 780,796 ----
> else if (pg_strcasecmp(prev3_wd, "TABLE") == 0 &&
> (pg_strcasecmp(prev_wd, "ALTER") == 0 ||
> pg_strcasecmp(prev_wd, "RENAME") == 0))
> ! {
> ! char** list = GET_MALLOCED_LIST_WITH_ATTR(prev2_wd);
Should be "char **list = ..." -- similarly in several other places.
> *** 1454,1461 ****
> else if (pg_strcasecmp(prev_wd, "LOCK") == 0 ||
> (pg_strcasecmp(prev_wd, "TABLE") == 0 &&
> pg_strcasecmp(prev2_wd, "LOCK") == 0))
> ! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables,
> NULL);
> !
> /* For the following, handle the case of a single table only
> for now */
>
> /* Complete LOCK [TABLE] <table> with "IN" */
> --- 1497,1510 ----
> else if (pg_strcasecmp(prev_wd, "LOCK") == 0 ||
> (pg_strcasecmp(prev_wd, "TABLE") == 0 &&
> pg_strcasecmp(prev2_wd, "LOCK") == 0))
> ! {
> ! char** list =
> GET_MALLOCED_LIST_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL);
> ! /* 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.
> *** 2315,2320 ****
> --- 2368,2437 ----
>
> }
>
> + /* LIST HELPER FUNCTIONS */
> +
> + static char**
> + get_empty_list() {
> + char** list = malloc(sizeof(char*));
> + list[0] = NULL;
> + return list;
> + }
Brace style ("{" on newline), "char **", and "(void"), as above.
> + 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.
> + 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*").
> + 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.
-Neil
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
I will adjust this patch based on later feedback emails.
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Joachim Wieland wrote:
> Hi,
>
> psql's tab completion has the following problem:
>
> If we have the following syntax for example:
>
> SET SESSION AUTHORIZATION <user>;
> SET SESSION AUTHORIZATION DEFAULT;
>
> After "SET SESSION AUTHORIZATION", the tab completion can offer a list of
> roles or the string constant "DEFAULT". However it can't offer both because
> it can't get a list of roles and add a string constant to this list.
>
> The appended patch adds the functionality of lists that can be extended with
> constants.
>
> Then you get:
>
> template1=# SET session AUTHORIZATION <tab>
> DEFAULT fred joe john
>
> I did proof-of-concept examples to add a constant to a
>
> - list from a query
> - list from a schema query
> - list of table attributes
>
>
> Joachim
>
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
On Fri, Feb 03, 2006 at 01:59:19PM -0500, Bruce Momjian wrote:
> I will adjust this patch based on later feedback emails.
I have another version of the patch. Minor style errors are corrected in it.
As far as I can see, open items are:
- Should all the
const char* foocompletion = { "BAR", "BAZ" };
be changed to
char* foocompletion = { "BAR", "BAZ" };
(including all functions that deal with that and take const char*'s now)
or should a cast be used only for the growable-list-case.
- is the simple list_add_item() algorithm sufficient for the psql use case
or not?
I append the version of the patch that I think is the latest I have.
Joachim
Attachment
Joachim Wieland <joe@mcknight.de> writes:
> - Should all the
> const char* foocompletion = { "BAR", "BAZ" };
> be changed to
> char* foocompletion = { "BAR", "BAZ" };
Removing const decoration sure seems like the wrong direction to be
going in. Why can't you still have const on the functions that expect
to take non-modifiable lists?
regards, tom lane
On Fri, Feb 03, 2006 at 04:15:32PM -0500, Tom Lane wrote:
> > - Should all the
> > const char* foocompletion = { "BAR", "BAZ" };
> > be changed to
> > char* foocompletion = { "BAR", "BAZ" };
> Removing const decoration sure seems like the wrong direction to be
> going in. Why can't you still have const on the functions that expect
> to take non-modifiable lists?
ok, one could duplicate the functionality of complete_from_list() and add a
function complete_from_malloced_list() or similar (or refactor both such that
they call a third function that does the actual work - if possible). I will
do that if this is what you're proposing.
Joachim
On Fri, Feb 03, 2006 at 04:15:32PM -0500, Tom Lane wrote:
> Joachim Wieland <joe@mcknight.de> writes:
> > - Should all the
> > const char* foocompletion = { "BAR", "BAZ" };
> > be changed to
> > char* foocompletion = { "BAR", "BAZ" };
> Removing const decoration sure seems like the wrong direction to be
> going in. Why can't you still have const on the functions that expect
> to take non-modifiable lists?
Ok, next try. Patch attached.
Joachim
Attachment
Modified patch attached and applied. Rather than create the lists in psql, I used UNION SELECT 'KEYWORD' to pass the keyword to the backend to be added to the query list. This was already being done for schema names, and was easy and efficient to add. My addition is even simpler because it just concatenates two adjacent strings. If you have more modifications, please use this system. If you find a macro that needs an 'addon', you can add it as a macro parameter to all calls and just use "" if you don't need it. If any macro call uses a non-constant string, you have to make a new *_ADDON macro version. --------------------------------------------------------------------------- Joachim Wieland wrote: > Hi, > > psql's tab completion has the following problem: > > If we have the following syntax for example: > > SET SESSION AUTHORIZATION <user>; > SET SESSION AUTHORIZATION DEFAULT; > > After "SET SESSION AUTHORIZATION", the tab completion can offer a list of > roles or the string constant "DEFAULT". However it can't offer both because > it can't get a list of roles and add a string constant to this list. > > The appended patch adds the functionality of lists that can be extended with > constants. > > Then you get: > > template1=# SET session AUTHORIZATION <tab> > DEFAULT fred joe john > > I did proof-of-concept examples to add a constant to a > > - list from a query > - list from a schema query > - list of table attributes > > > Joachim > [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 6: explain analyze is your friend -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/bin/psql/tab-complete.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/tab-complete.c,v retrieving revision 1.146 diff -c -c -r1.146 tab-complete.c *** src/bin/psql/tab-complete.c 12 Feb 2006 03:22:19 -0000 1.146 --- src/bin/psql/tab-complete.c 12 Feb 2006 07:18:33 -0000 *************** *** 140,153 **** */ #define COMPLETE_WITH_QUERY(query) \ do { completion_charp = query; matches = completion_matches(text, complete_from_query); } while(0) ! #define COMPLETE_WITH_SCHEMA_QUERY(query,addon) \ do { completion_squery = &(query); completion_charp = addon; matches = completion_matches(text, complete_from_schema_query);} while(0) #define COMPLETE_WITH_LIST(list) \ do { completion_charpp = list; matches = completion_matches(text, complete_from_list); } while(0) #define COMPLETE_WITH_CONST(string) \ do { completion_charp = string; matches = completion_matches(text, complete_from_const); } while(0) ! #define COMPLETE_WITH_ATTR(table) \ ! do {completion_charp = Query_for_list_of_attributes; completion_info_charp = table; matches = completion_matches(text,complete_from_query); } while(0) /* * Assembly instructions for schema queries --- 140,155 ---- */ #define COMPLETE_WITH_QUERY(query) \ do { completion_charp = query; matches = completion_matches(text, complete_from_query); } while(0) ! #define COMPLETE_WITH_QUERY_ADDON(query, addon) \ ! do { completion_charp = query addon; matches = completion_matches(text, complete_from_query); } while(0) ! #define COMPLETE_WITH_SCHEMA_QUERY(query, addon) \ do { completion_squery = &(query); completion_charp = addon; matches = completion_matches(text, complete_from_schema_query);} while(0) #define COMPLETE_WITH_LIST(list) \ do { completion_charpp = list; matches = completion_matches(text, complete_from_list); } while(0) #define COMPLETE_WITH_CONST(string) \ do { completion_charp = string; matches = completion_matches(text, complete_from_const); } while(0) ! #define COMPLETE_WITH_ATTR(table, addon) \ ! do {completion_charp = Query_for_list_of_attributes addon; completion_info_charp = table; matches = completion_matches(text,complete_from_query); } while(0) /* * Assembly instructions for schema queries *************** *** 754,767 **** else if (pg_strcasecmp(prev3_wd, "TABLE") == 0 && (pg_strcasecmp(prev_wd, "ALTER") == 0 || pg_strcasecmp(prev_wd, "RENAME") == 0)) ! COMPLETE_WITH_ATTR(prev2_wd); /* ALTER TABLE xxx RENAME yyy */ else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 && pg_strcasecmp(prev2_wd, "RENAME") == 0 && pg_strcasecmp(prev_wd, "TO") != 0) COMPLETE_WITH_CONST("TO"); /* If we have TABLE <sth> DROP, provide COLUMN or CONSTRAINT */ else if (pg_strcasecmp(prev3_wd, "TABLE") == 0 && pg_strcasecmp(prev_wd, "DROP") == 0) --- 756,783 ---- else if (pg_strcasecmp(prev3_wd, "TABLE") == 0 && (pg_strcasecmp(prev_wd, "ALTER") == 0 || pg_strcasecmp(prev_wd, "RENAME") == 0)) ! COMPLETE_WITH_ATTR(prev2_wd, " UNION SELECT 'COLUMN'"); + /* If we have TABLE <sth> ALTER COLUMN|RENAME COLUMN, provide list of columns */ + else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 && + (pg_strcasecmp(prev2_wd, "ALTER") == 0 || + pg_strcasecmp(prev2_wd, "RENAME") == 0) && + pg_strcasecmp(prev_wd, "COLUMN") == 0) + COMPLETE_WITH_ATTR(prev3_wd, ""); + /* ALTER TABLE xxx RENAME yyy */ else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 && pg_strcasecmp(prev2_wd, "RENAME") == 0 && pg_strcasecmp(prev_wd, "TO") != 0) COMPLETE_WITH_CONST("TO"); + /* ALTER TABLE xxx RENAME COLUMN yyy */ + else if (pg_strcasecmp(prev5_wd, "TABLE") == 0 && + pg_strcasecmp(prev3_wd, "RENAME") == 0 && + pg_strcasecmp(prev2_wd, "COLUMN") == 0 && + pg_strcasecmp(prev_wd, "TO") != 0) + COMPLETE_WITH_CONST("TO"); + /* If we have TABLE <sth> DROP, provide COLUMN or CONSTRAINT */ else if (pg_strcasecmp(prev3_wd, "TABLE") == 0 && pg_strcasecmp(prev_wd, "DROP") == 0) *************** *** 775,781 **** else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 && pg_strcasecmp(prev2_wd, "DROP") == 0 && pg_strcasecmp(prev_wd, "COLUMN") == 0) ! COMPLETE_WITH_ATTR(prev3_wd); /* ALTER TABLE ALTER [COLUMN] <foo> */ else if ((pg_strcasecmp(prev3_wd, "ALTER") == 0 && pg_strcasecmp(prev2_wd, "COLUMN") == 0) || --- 791,797 ---- else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 && pg_strcasecmp(prev2_wd, "DROP") == 0 && pg_strcasecmp(prev_wd, "COLUMN") == 0) ! COMPLETE_WITH_ATTR(prev3_wd, ""); /* ALTER TABLE ALTER [COLUMN] <foo> */ else if ((pg_strcasecmp(prev3_wd, "ALTER") == 0 && pg_strcasecmp(prev2_wd, "COLUMN") == 0) || *************** *** 1021,1038 **** pg_strcasecmp(prev2_wd, "ON") == 0) { if (find_open_parenthesis(end)) ! COMPLETE_WITH_ATTR(prev_wd); else COMPLETE_WITH_CONST("("); } else if (pg_strcasecmp(prev5_wd, "INDEX") == 0 && pg_strcasecmp(prev3_wd, "ON") == 0 && pg_strcasecmp(prev_wd, "(") == 0) ! COMPLETE_WITH_ATTR(prev2_wd); /* same if you put in USING */ else if (pg_strcasecmp(prev4_wd, "ON") == 0 && pg_strcasecmp(prev2_wd, "USING") == 0) ! COMPLETE_WITH_ATTR(prev3_wd); /* Complete USING with an index method */ else if (pg_strcasecmp(prev_wd, "USING") == 0) { --- 1037,1054 ---- pg_strcasecmp(prev2_wd, "ON") == 0) { if (find_open_parenthesis(end)) ! COMPLETE_WITH_ATTR(prev_wd, ""); else COMPLETE_WITH_CONST("("); } else if (pg_strcasecmp(prev5_wd, "INDEX") == 0 && pg_strcasecmp(prev3_wd, "ON") == 0 && pg_strcasecmp(prev_wd, "(") == 0) ! COMPLETE_WITH_ATTR(prev2_wd, ""); /* same if you put in USING */ else if (pg_strcasecmp(prev4_wd, "ON") == 0 && pg_strcasecmp(prev2_wd, "USING") == 0) ! COMPLETE_WITH_ATTR(prev3_wd, ""); /* Complete USING with an index method */ else if (pg_strcasecmp(prev_wd, "USING") == 0) { *************** *** 1420,1426 **** else if (rl_line_buffer[start - 1] == '(' && pg_strcasecmp(prev3_wd, "INSERT") == 0 && pg_strcasecmp(prev2_wd, "INTO") == 0) ! COMPLETE_WITH_ATTR(prev_wd); /* * Complete INSERT INTO <table> with "VALUES" or "SELECT" or "DEFAULT --- 1436,1442 ---- else if (rl_line_buffer[start - 1] == '(' && pg_strcasecmp(prev3_wd, "INSERT") == 0 && pg_strcasecmp(prev2_wd, "INTO") == 0) ! COMPLETE_WITH_ATTR(prev_wd, ""); /* * Complete INSERT INTO <table> with "VALUES" or "SELECT" or "DEFAULT *************** *** 1452,1461 **** /* LOCK */ /* Complete LOCK [TABLE] with a list of tables */ ! else if (pg_strcasecmp(prev_wd, "LOCK") == 0 || ! (pg_strcasecmp(prev_wd, "TABLE") == 0 && ! pg_strcasecmp(prev2_wd, "LOCK") == 0)) ! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL); /* For the following, handle the case of a single table only for now */ --- 1468,1479 ---- /* LOCK */ /* Complete LOCK [TABLE] with a list of tables */ ! else if (pg_strcasecmp(prev_wd, "LOCK") == 0) ! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, ! " UNION SELECT 'TABLE'"); ! else if (pg_strcasecmp(prev_wd, "TABLE") == 0 && ! pg_strcasecmp(prev2_wd, "LOCK") == 0) ! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, ""); /* For the following, handle the case of a single table only for now */ *************** *** 1498,1504 **** else if (pg_strcasecmp(prev4_wd, "FROM") == 0 && pg_strcasecmp(prev2_wd, "ORDER") == 0 && pg_strcasecmp(prev_wd, "BY") == 0) ! COMPLETE_WITH_ATTR(prev3_wd); /* PREPARE xx AS */ else if (pg_strcasecmp(prev_wd, "AS") == 0 && --- 1516,1522 ---- else if (pg_strcasecmp(prev4_wd, "FROM") == 0 && pg_strcasecmp(prev2_wd, "ORDER") == 0 && pg_strcasecmp(prev_wd, "BY") == 0) ! COMPLETE_WITH_ATTR(prev3_wd, ""); /* PREPARE xx AS */ else if (pg_strcasecmp(prev_wd, "AS") == 0 && *************** *** 1639,1645 **** else if (pg_strcasecmp(prev3_wd, "SET") == 0 && pg_strcasecmp(prev2_wd, "SESSION") == 0 && pg_strcasecmp(prev_wd, "AUTHORIZATION") == 0) ! COMPLETE_WITH_QUERY(Query_for_list_of_roles); /* Complete RESET SESSION with AUTHORIZATION */ else if (pg_strcasecmp(prev2_wd, "RESET") == 0 && pg_strcasecmp(prev_wd, "SESSION") == 0) --- 1657,1663 ---- else if (pg_strcasecmp(prev3_wd, "SET") == 0 && pg_strcasecmp(prev2_wd, "SESSION") == 0 && pg_strcasecmp(prev_wd, "AUTHORIZATION") == 0) ! COMPLETE_WITH_QUERY_ADDON(Query_for_list_of_roles, " UNION SELECT 'DEFAULT'"); /* Complete RESET SESSION with AUTHORIZATION */ else if (pg_strcasecmp(prev2_wd, "RESET") == 0 && pg_strcasecmp(prev_wd, "SESSION") == 0) *************** *** 1706,1712 **** * make a list of attributes. */ else if (pg_strcasecmp(prev_wd, "SET") == 0) ! COMPLETE_WITH_ATTR(prev2_wd); /* UPDATE xx SET yy = */ else if (pg_strcasecmp(prev2_wd, "SET") == 0 && --- 1724,1730 ---- * make a list of attributes. */ else if (pg_strcasecmp(prev_wd, "SET") == 0) ! COMPLETE_WITH_ATTR(prev2_wd, ""); /* UPDATE xx SET yy = */ else if (pg_strcasecmp(prev2_wd, "SET") == 0 && *************** *** 1763,1769 **** /* WHERE */ /* Simple case of the word before the where being the table name */ else if (pg_strcasecmp(prev_wd, "WHERE") == 0) ! COMPLETE_WITH_ATTR(prev2_wd); /* ... FROM ... */ /* TODO: also include SRF ? */ --- 1781,1787 ---- /* WHERE */ /* Simple case of the word before the where being the table name */ else if (pg_strcasecmp(prev_wd, "WHERE") == 0) ! COMPLETE_WITH_ATTR(prev2_wd, ""); /* ... FROM ... */ /* TODO: also include SRF ? */
I found it was easier to concatenate the "UNION" at the macro call site, rather than the macro body. This eliminates the extra macro. Patch attached and applied. --------------------------------------------------------------------------- pgman wrote: > > Modified patch attached and applied. > > Rather than create the lists in psql, I used UNION SELECT 'KEYWORD' to > pass the keyword to the backend to be added to the query list. > > This was already being done for schema names, and was easy and efficient > to add. My addition is even simpler because it just concatenates two > adjacent strings. If you have more modifications, please use this > system. > > If you find a macro that needs an 'addon', you can add it as a macro > parameter to all calls and just use "" if you don't need it. If any > macro call uses a non-constant string, you have to make a new *_ADDON > macro version. > > --------------------------------------------------------------------------- > > Joachim Wieland wrote: > > Hi, > > > > psql's tab completion has the following problem: > > > > If we have the following syntax for example: > > > > SET SESSION AUTHORIZATION <user>; > > SET SESSION AUTHORIZATION DEFAULT; > > > > After "SET SESSION AUTHORIZATION", the tab completion can offer a list of > > roles or the string constant "DEFAULT". However it can't offer both because > > it can't get a list of roles and add a string constant to this list. > > > > The appended patch adds the functionality of lists that can be extended with > > constants. > > > > Then you get: > > > > template1=# SET session AUTHORIZATION <tab> > > DEFAULT fred joe john > > > > I did proof-of-concept examples to add a constant to a > > > > - list from a query > > - list from a schema query > > - list of table attributes > > > > > > Joachim -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/bin/psql/tab-complete.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/tab-complete.c,v retrieving revision 1.147 diff -c -c -r1.147 tab-complete.c *** src/bin/psql/tab-complete.c 12 Feb 2006 07:21:40 -0000 1.147 --- src/bin/psql/tab-complete.c 12 Feb 2006 15:05:09 -0000 *************** *** 140,147 **** */ #define COMPLETE_WITH_QUERY(query) \ do { completion_charp = query; matches = completion_matches(text, complete_from_query); } while(0) - #define COMPLETE_WITH_QUERY_ADDON(query, addon) \ - do { completion_charp = query addon; matches = completion_matches(text, complete_from_query); } while(0) #define COMPLETE_WITH_SCHEMA_QUERY(query, addon) \ do { completion_squery = &(query); completion_charp = addon; matches = completion_matches(text, complete_from_schema_query);} while(0) #define COMPLETE_WITH_LIST(list) \ --- 140,145 ---- *************** *** 1657,1663 **** else if (pg_strcasecmp(prev3_wd, "SET") == 0 && pg_strcasecmp(prev2_wd, "SESSION") == 0 && pg_strcasecmp(prev_wd, "AUTHORIZATION") == 0) ! COMPLETE_WITH_QUERY_ADDON(Query_for_list_of_roles, " UNION SELECT 'DEFAULT'"); /* Complete RESET SESSION with AUTHORIZATION */ else if (pg_strcasecmp(prev2_wd, "RESET") == 0 && pg_strcasecmp(prev_wd, "SESSION") == 0) --- 1655,1661 ---- else if (pg_strcasecmp(prev3_wd, "SET") == 0 && pg_strcasecmp(prev2_wd, "SESSION") == 0 && pg_strcasecmp(prev_wd, "AUTHORIZATION") == 0) ! COMPLETE_WITH_QUERY(Query_for_list_of_roles " UNION SELECT 'DEFAULT'"); /* Complete RESET SESSION with AUTHORIZATION */ else if (pg_strcasecmp(prev2_wd, "RESET") == 0 && pg_strcasecmp(prev_wd, "SESSION") == 0)
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I found it was easier to concatenate the "UNION" at the macro call site,
> rather than the macro body. This eliminates the extra macro. Patch
> attached and applied.
Yeah, that looks like a much safer idea --- avoids making assumptions
about the behavior of the macro expander.
regards, tom lane