Hello
2010/7/23 Jan Urbański <[email protected]>:
> On 21/07/10 14:43, Pavel Stehule wrote:
>> Hello
>>
>> I am sending a actualised patch.
>
> Hi, thanks!
>
>> I understand to your criticism about line numbering. I have to
>> agree. With line numbering the patch is longer. I have a one
>> significant reason for it.
>
>> **** CREATE OR REPLACE FUNCTION public.foo() **** RETURNS integer
>> **** LANGUAGE plpgsql **** AS $function$ 1 begin 2 return
>> 10/0; 3 end; **** $function$
>>
>> postgres=# select foo(); ERROR: division by zero CONTEXT: SQL
>> statement "SELECT 10/0" PL/pgSQL function "foo" line 2 at RETURN
>> postgres=#
>
> OK, that convinced me, and also others seem to like the feature. So I'll
> just make code remarks this time.
>
>
> In the \e handling code, if the file name was given and there is no line
> number, an uninitialised variable will be passed to do_edit. I see that
> you want to avoid passing a magic number to do_edit, but I think you
> should just treat anything <0 as that magic value, initialise lineno
> with -1 and simplify all these nested ifs.
>
fixed uninitialised variable. Second is little bit different - there
is three states, not only two - lineno is undefined, lineno is wrong
and lineno is correct. I would not to ignore a wrong lineno.
> Typo in a comment:
> when wirst row of function -> when first row of function
fixed
>
> I think the #define for DEFAULT_BODY_SEPARATOR should either be at the
> beginning of the file (and then also used in \ef handling) or just
> hardcoded in both places.
this means some like only local constant - see PARAMS_ARRAY_SIZE in
same file. It is used only inside a first_row_is_empty function. It's
not used outside this function.
>
> Any reason why DEFAULT_NAVIGATION_COMMAND has a space in front of it?
>
no it is useless - fixed
> Some lines have >80 characters.
there are lot of longer lines - but I can't to modify other lines only
for formating. My code has max line about 90 characters (when it
doesn't respect more common form for some parts).
>
> If you agree that a -1 parameter do do_edit will mean "no lineno", then
> I think you can change get_lineno_for_navigation to not take a
> backslashResult argument and signal errors by returning -1.
there are a too much magic constants, so I prefer a cleaner form with
backslashResult. The code isn't longer and it reacts better on wrong
entered value - negative value is nonsense for this purpose.
>
> In get_lineno_for_navigation you will have to protect the large comment
> block with /*------ otherwise pgindent will reflow it.
done
>
> PSQL_NAVIGATION_COMMAND needs to be documented in the SGML docs.
>
I changed PSQL_NAVIGATION_COMMAND to PSQL_EDITOR_NAVIGATION_OPTION and
append a few lines - as I can - some one who can speak English has to
correct it.
>
> Uff, that's all from me, sorry for bringing up all these small issues, I
> hope this will go in soon!
>
It is your job :)
> I'm going to change it to waiting on author again, mainly because of the
> uninitialised variable in \d handling, the rest are just stylistic nitpicks.
>
> Cheers,
> Jan
>
attached updated patch
Thank you very much
Pavel Stehule
*** ./doc/src/sgml/ref/psql-ref.sgml.orig 2010-07-20 05:54:19.000000000 +0200
--- ./doc/src/sgml/ref/psql-ref.sgml 2010-07-23 20:46:49.746690828 +0200
***************
*** 1339,1345 ****
<varlistentry>
! <term><literal>\edit</literal> (or <literal>\e</literal>) <literal><optional> <replaceable class="parameter">filename</replaceable> </optional></literal></term>
<listitem>
<para>
--- 1339,1345 ----
<varlistentry>
! <term><literal>\edit</literal> (or <literal>\e</literal>) <literal><optional> <replaceable class="parameter">filename</replaceable> </optional> <optional> linenumber </optional></literal></term>
<listitem>
<para>
***************
*** 1369,1380 ****
systems, <filename>notepad.exe</filename> on Windows systems.
</para>
</tip>
</listitem>
</varlistentry>
<varlistentry>
! <term><literal>\ef <optional> <replaceable class="parameter">function_description</replaceable> </optional></literal></term>
<listitem>
<para>
--- 1369,1386 ----
systems, <filename>notepad.exe</filename> on Windows systems.
</para>
</tip>
+
+ <para>
+ If <replaceable class="parameter">linenumber</replaceable> is
+ specified, then cursor is moved on this line after start of
+ editor.
+ </para>
</listitem>
</varlistentry>
<varlistentry>
! <term><literal>\ef <optional> <replaceable class="parameter">function_description</replaceable> </optional> <optional> linenumber </optional> </literal></term>
<listitem>
<para>
***************
*** 1397,1402 ****
--- 1403,1415 ----
If no function is specified, a blank <command>CREATE FUNCTION</>
template is presented for editing.
</para>
+
+ <para>
+ If <replaceable class="parameter">linenumber</replaceable> is
+ specified, then cursor is moved on this line after start of
+ editor. It count lines from start of function body, not from
+ start of text.
+ </para>
</listitem>
</varlistentry>
***************
*** 2116,2121 ****
--- 2129,2146 ----
<varlistentry>
+ <term><literal>\sf[+] <replaceable class="parameter">function_description</replaceable> <optional> linenumber </optional> </literal></term>
+
+ <listitem>
+ <para>
+ This command fetches and shows the definition of the named function,
+ in the form of a <command>CREATE OR REPLACE FUNCTION</> command.
+ If the form <literal>\sf+</literal> is used, then lines are numbered.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>\t</literal></term>
<listitem>
<para>
***************
*** 2123,2128 ****
--- 2148,2159 ----
footer. This command is equivalent to <literal>\pset
tuples_only</literal> and is provided for convenience.
</para>
+
+ <para>
+ If <replaceable class="parameter">linenumber</replaceable> is
+ specified, then cursor is moved on this line after start of
+ editor.
+ </para>
</listitem>
</varlistentry>
***************
*** 3066,3071 ****
--- 3097,3117 ----
</varlistentry>
<varlistentry>
+ <term><envar>PSQL_EDITOR_NAVIGATION_OPTION</envar></term>
+
+ <listitem>
+ <para>
+ Option used for navigation (go to line command) in external
+ editor. When it isn't defined, then it uses <option>+</option>
+ on Unix systems and <option>/</option> on Windows systems. For
+ wide used KDE editors is necessary to set this option to
+ <option>--line </option>. The space after <literal>line</literal>
+ is required.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><envar>SHELL</envar></term>
<listitem>
*** ./src/bin/psql/command.c.orig 2010-07-23 16:56:54.000000000 +0200
--- ./src/bin/psql/command.c 2010-07-23 20:50:29.189688832 +0200
***************
*** 57,63 ****
PsqlScanState scan_state,
PQExpBuffer query_buf);
static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
! bool *edited);
static bool do_connect(char *dbname, char *user, char *host, char *port);
static bool do_shell(const char *command);
static bool lookup_function_oid(PGconn *conn, const char *desc, Oid *foid);
--- 57,63 ----
PsqlScanState scan_state,
PQExpBuffer query_buf);
static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
! bool *edited, int lineno);
static bool do_connect(char *dbname, char *user, char *host, char *port);
static bool do_shell(const char *command);
static bool lookup_function_oid(PGconn *conn, const char *desc, Oid *foid);
***************
*** 65,70 ****
--- 65,72 ----
static void minimal_error_message(PGresult *res);
static void printSSLInfo(void);
+ static bool first_row_is_empty(char *src);
+ static int get_lineno_for_navigation(char *func, backslashResult *status);
#ifdef WIN32
static void checkWin32Codepage(void);
***************
*** 513,528 ****
else
{
char *fname;
!
fname = psql_scan_slash_option(scan_state,
! OT_NORMAL, NULL, true);
! expand_tilde(&fname);
if (fname)
! canonicalize_path(fname);
! if (do_edit(fname, query_buf, NULL))
! status = PSQL_CMD_NEWEDIT;
! else
! status = PSQL_CMD_ERROR;
free(fname);
}
}
--- 515,552 ----
else
{
char *fname;
! char *ln;
! int lineno = -1;
!
fname = psql_scan_slash_option(scan_state,
! OT_NORMAL, NULL, true);
!
! /* try to get lineno */
if (fname)
! {
! ln = psql_scan_slash_option(scan_state,
! OT_NORMAL, NULL, true);
! if (ln)
! {
! if (atoi(ln) < 1)
! {
! psql_error("line number is unacceptable\n");
! status = PSQL_CMD_ERROR;
! }
! else
! lineno = atoi(ln);
! }
! }
! if (status != PSQL_CMD_ERROR)
! {
! expand_tilde(&fname);
! if (fname)
! canonicalize_path(fname);
! if (do_edit(fname, query_buf, NULL, lineno))
! status = PSQL_CMD_NEWEDIT;
! else
! status = PSQL_CMD_ERROR;
! }
free(fname);
}
}
***************
*** 533,538 ****
--- 557,564 ----
*/
else if (strcmp(cmd, "ef") == 0)
{
+ int lineno;
+
if (!query_buf)
{
psql_error("no query buffer\n");
***************
*** 545,580 ****
func = psql_scan_slash_option(scan_state,
OT_WHOLE_LINE, NULL, true);
! if (!func)
{
! /* set up an empty command to fill in */
! printfPQExpBuffer(query_buf,
! "CREATE FUNCTION ( )\n"
! " RETURNS \n"
! " LANGUAGE \n"
! " -- common options: IMMUTABLE STABLE STRICT SECURITY DEFINER\n"
! "AS $function$\n"
! "\n$function$\n");
! }
! else if (!lookup_function_oid(pset.db, func, &foid))
! {
! /* error already reported */
! status = PSQL_CMD_ERROR;
! }
! else if (!get_create_function_cmd(pset.db, foid, query_buf))
! {
! /* error already reported */
! status = PSQL_CMD_ERROR;
}
- if (func)
- free(func);
}
if (status != PSQL_CMD_ERROR)
{
bool edited = false;
! if (!do_edit(0, query_buf, &edited))
status = PSQL_CMD_ERROR;
else if (!edited)
puts(_("No changes"));
--- 571,615 ----
func = psql_scan_slash_option(scan_state,
OT_WHOLE_LINE, NULL, true);
! lineno = get_lineno_for_navigation(func, &status);
!
! if (status != PSQL_CMD_ERROR)
{
! if (!func)
! {
! /* set up an empty command to fill in */
! printfPQExpBuffer(query_buf,
! "CREATE FUNCTION ( )\n"
! " RETURNS \n"
! " LANGUAGE \n"
! " -- common options: IMMUTABLE STABLE STRICT SECURITY DEFINER\n"
! "AS $function$\n"
! "\n$function$\n");
! }
! else if (!lookup_function_oid(pset.db, func, &foid))
! {
! /* error already reported */
! status = PSQL_CMD_ERROR;
! }
! else if (!get_create_function_cmd(pset.db, foid, query_buf))
! {
! /* error already reported */
! status = PSQL_CMD_ERROR;
! }
! if (func)
! free(func);
}
}
if (status != PSQL_CMD_ERROR)
{
bool edited = false;
! /* correct lineno when first row of function's body is empty */
! if (!first_row_is_empty(query_buf->data))
! lineno--;
!
! if (!do_edit(0, query_buf, &edited, lineno))
status = PSQL_CMD_ERROR;
else if (!edited)
puts(_("No changes"));
***************
*** 969,974 ****
--- 1004,1097 ----
free(fname);
}
+ /*
+ * \sf -- show the named function
+ */
+ else if (strcmp(cmd, "sf") == 0 || strcmp(cmd, "sf+") == 0)
+ {
+ int skip_lines = -1;
+ bool with_lno;
+
+ with_lno = (strcmp(cmd, "sf+") == 0);
+
+ if (!query_buf)
+ {
+ psql_error("no query buffer\n");
+ status = PSQL_CMD_ERROR;
+ }
+ else
+ {
+ char *func;
+ Oid foid = InvalidOid;
+
+ func = psql_scan_slash_option(scan_state,
+ OT_WHOLE_LINE, NULL, true);
+ skip_lines = get_lineno_for_navigation(func, &status) - 1;
+
+ if (status != PSQL_CMD_ERROR)
+ {
+ if (!func)
+ {
+ /* show error for empty command */
+ psql_error("missing a function name\n");
+ status = PSQL_CMD_ERROR;
+ }
+ else if (!lookup_function_oid(pset.db, func, &foid))
+ {
+ /* error already reported */
+ status = PSQL_CMD_ERROR;
+ }
+ else if (!get_create_function_cmd(pset.db, foid, query_buf))
+ {
+ /* error already reported */
+ status = PSQL_CMD_ERROR;
+ }
+ }
+ if (func)
+ free(func);
+ }
+
+ if (status != PSQL_CMD_ERROR)
+ {
+ int lineno = 0;
+ char *c = query_buf->data;
+ char *ptr;
+
+ /*
+ * PL doesn't calculate first row of function's body
+ * when first row is empty. So checks first row, and
+ * correct lineno when it is necessary.
+ */
+ if (first_row_is_empty(query_buf->data))
+ lineno--;
+
+ while (*c)
+ {
+ /* find next end of line */
+ for (ptr = c; *c != '\n'; c++);
+ *c++ = '\0';
+ lineno++;
+
+ /* skip first n lines */
+ if (skip_lines > 0 && (skip_lines > lineno))
+ continue;
+
+ if (!with_lno)
+ printf("%s\n", ptr);
+ else
+ {
+ /* don't show lineno for first three rows and last row */
+ if ((*c == '\0' && lineno != 3) || lineno < 4)
+ printf("**** %s\n", ptr);
+ else
+ printf("%4d %s\n", lineno - 3, ptr);
+ }
+ }
+ printf("\n");
+ fflush(stdout);
+ }
+ }
+
/* \set -- generalized set variable/option command */
else if (strcmp(cmd, "set") == 0)
{
***************
*** 1550,1558 ****
*/
static bool
! editFile(const char *fname)
{
const char *editorName;
char *sys;
int result;
--- 1673,1682 ----
*/
static bool
! editFile(const char *fname, int lineno)
{
const char *editorName;
+ const char *navigation_cmd;
char *sys;
int result;
***************
*** 1566,1571 ****
--- 1690,1699 ----
editorName = getenv("VISUAL");
if (!editorName)
editorName = DEFAULT_EDITOR;
+
+ navigation_cmd = getenv("PSQL_EDITOR_NAVIGATION_OPTION");
+ if (!navigation_cmd)
+ navigation_cmd = DEFAULT_EDITOR_NAVIGATION_OPTION;
/*
* On Unix the EDITOR value should *not* be quoted, since it might include
***************
*** 1574,1584 ****
* severe brain damage in their command shell plus the fact that standard
* program paths include spaces.
*/
! sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
#ifndef WIN32
! sprintf(sys, "exec %s '%s'", editorName, fname);
#else
! sprintf(sys, SYSTEMQUOTE "\"%s\" \"%s\"" SYSTEMQUOTE, editorName, fname);
#endif
result = system(sys);
if (result == -1)
--- 1702,1722 ----
* severe brain damage in their command shell plus the fact that standard
* program paths include spaces.
*/
! sys = pg_malloc(strlen(editorName) + strlen(fname) + 20 + 1);
#ifndef WIN32
! if (lineno > 0)
! sprintf(sys, "exec %s %s%d '%s'", editorName, navigation_cmd, lineno, fname);
! else
! sprintf(sys, "exec %s '%s'", editorName, fname);
#else
! if (lineno > 0)
! sprintf(sys, SYSTEMQUOTE "\"%s\" %s%d \"%s\"" SYSTEMQUOTE,
! editorName,
! navigation_cmd,
! lineno,
! fname);
! else
! sprintf(sys, SYSTEMQUOTE "\"%s\" \"%s\"" SYSTEMQUOTE, editorName, fname);
#endif
result = system(sys);
if (result == -1)
***************
*** 1593,1599 ****
/* call this one */
static bool
! do_edit(const char *filename_arg, PQExpBuffer query_buf, bool *edited)
{
char fnametmp[MAXPGPATH];
FILE *stream = NULL;
--- 1731,1737 ----
/* call this one */
static bool
! do_edit(const char *filename_arg, PQExpBuffer query_buf, bool *edited, int lineno)
{
char fnametmp[MAXPGPATH];
FILE *stream = NULL;
***************
*** 1685,1691 ****
/* call editor */
if (!error)
! error = !editFile(fname);
if (!error && stat(fname, &after) != 0)
{
--- 1823,1829 ----
/* call editor */
if (!error)
! error = !editFile(fname, lineno);
if (!error && stat(fname, &after) != 0)
{
***************
*** 2181,2186 ****
--- 2319,2347 ----
return result;
}
+
+ /*
+ * When first row of prosrc has only new-line char, then is ignored.
+ * This functions returns true, when first row is empty, else returns
+ * false.
+ */
+ static bool
+ first_row_is_empty(char *src)
+ {
+ char *ptr;
+
+ #define DEFAULT_BODY_SEPARATOR "$function$"
+
+ ptr = strstr(src, DEFAULT_BODY_SEPARATOR);
+
+ psql_assert(ptr != NULL);
+
+ ptr += strlen(DEFAULT_BODY_SEPARATOR);
+
+ return *ptr == '\n';
+ }
+
+
/*
* Fetches the "CREATE OR REPLACE FUNCTION ..." command that describes the
* function with the given OID. If successful, the result is stored in buf.
***************
*** 2213,2218 ****
--- 2374,2380 ----
return result;
}
+
/*
* Report just the primary error; this is to avoid cluttering the output
* with, for instance, a redisplay of the internally generated query
***************
*** 2241,2243 ****
--- 2403,2480 ----
destroyPQExpBuffer(msg);
}
+
+
+ /*
+ * Returns lineno used in \sf and \ef commands.
+ *
+ * These commands can be completed with number used as line
+ * number for navigation in showed lines / open file. The most
+ * simple method for parsing is reading isolated digits from
+ * right - \ef foo nn, \ef foo(..)nn. Returns -1 when
+ * lineno isn't defined.
+ */
+ static int
+ get_lineno_for_navigation(char *func, backslashResult *status)
+ {
+ char *endfunc;
+ char *c;
+ int lineno = -1;
+
+ if (!func)
+ return lineno;
+
+ endfunc = func + strlen(func) - 1;
+ c = endfunc;
+
+ /* skip useles whitespaces */
+ while (c >= func)
+ if (isblank(*c))
+ c--;
+ else
+ break;
+
+ /* search the most left digit of continuously number */
+ while (c >= func)
+ if (!isdigit(*c))
+ break;
+ else
+ c--;
+
+ /*
+ * when left char isn't blank and isn't a right parenthesis
+ * then command hasn't a lineno.
+ */
+ if (c < endfunc && c > func)
+ {
+ if (isblank(*c) || *c == ')')
+ {
+ c++;
+
+ if (atoi(c) < 1)
+ {
+ psql_error("line number is unacceptable\n");
+ *status = PSQL_CMD_ERROR;
+ }
+ else
+ {
+ /*----------
+ * Function get_create_function_cmd appends a few lines
+ * to function's body. But we would to like use a line
+ * numbers use a PL parsers - so add three lines to
+ * lineno:
+ * CREATE OR REPLACE FUNCTION ..
+ * RETURNS ...
+ * LANGUAGE ...
+ * AS $finction$
+ */
+ lineno = atoi(c) + 4;
+
+ /* remove lineno from function descriptor */
+ *c = '\0';
+ }
+ }
+ }
+
+ return lineno;
+ }
*** ./src/bin/psql/help.c.orig 2010-07-23 19:51:20.039690385 +0200
--- ./src/bin/psql/help.c 2010-07-23 19:51:39.905689608 +0200
***************
*** 174,186 ****
fprintf(output, "\n");
fprintf(output, _("Query Buffer\n"));
! fprintf(output, _(" \\e [FILE] edit the query buffer (or file) with external editor\n"));
! fprintf(output, _(" \\ef [FUNCNAME] edit function definition with external editor\n"));
fprintf(output, _(" \\p show the contents of the query buffer\n"));
fprintf(output, _(" \\r reset (clear) the query buffer\n"));
#ifdef USE_READLINE
fprintf(output, _(" \\s [FILE] display history or save it to file\n"));
#endif
fprintf(output, _(" \\w FILE write query buffer to file\n"));
fprintf(output, "\n");
--- 174,187 ----
fprintf(output, "\n");
fprintf(output, _("Query Buffer\n"));
! fprintf(output, _(" \\e [FILE] [lno] edit the query buffer (or file) with external editor\n"));
! fprintf(output, _(" \\ef [FUNCNAME] [lno] edit function definition with external editor\n"));
fprintf(output, _(" \\p show the contents of the query buffer\n"));
fprintf(output, _(" \\r reset (clear) the query buffer\n"));
#ifdef USE_READLINE
fprintf(output, _(" \\s [FILE] display history or save it to file\n"));
#endif
+ fprintf(output, _(" \\sf[+] FUNCNAME [lno] show finction definition\n"));
fprintf(output, _(" \\w FILE write query buffer to file\n"));
fprintf(output, "\n");
*** ./src/bin/psql/settings.h.orig 2010-07-23 19:51:20.043690094 +0200
--- ./src/bin/psql/settings.h 2010-07-23 20:51:12.235691465 +0200
***************
*** 18,25 ****
--- 18,27 ----
#if defined(WIN32) || defined(__CYGWIN__)
#define DEFAULT_EDITOR "notepad.exe"
+ #define DEFAULT_EDITOR_NAVIGATION_OPTION "/"
#else
#define DEFAULT_EDITOR "vi"
+ #define DEFAULT_EDITOR_NAVIGATION_OPTION "+"
#endif
#define DEFAULT_PROMPT1 "%/%R%# "
*** ./src/bin/psql/tab-complete.c.orig 2010-07-20 05:54:19.000000000 +0200
--- ./src/bin/psql/tab-complete.c 2010-07-23 19:51:39.909687081 +0200
***************
*** 644,650 ****
"\\f", "\\g", "\\h", "\\help", "\\H", "\\i", "\\l",
"\\lo_import", "\\lo_export", "\\lo_list", "\\lo_unlink",
"\\o", "\\p", "\\password", "\\prompt", "\\pset", "\\q", "\\qecho", "\\r",
! "\\set", "\\t", "\\T",
"\\timing", "\\unset", "\\x", "\\w", "\\z", "\\!", NULL
};
--- 644,650 ----
"\\f", "\\g", "\\h", "\\help", "\\H", "\\i", "\\l",
"\\lo_import", "\\lo_export", "\\lo_list", "\\lo_unlink",
"\\o", "\\p", "\\password", "\\prompt", "\\pset", "\\q", "\\qecho", "\\r",
! "\\set", "\\sf", "\\t", "\\T",
"\\timing", "\\unset", "\\x", "\\w", "\\z", "\\!", NULL
};
***************
*** 2501,2506 ****
--- 2501,2509 ----
else if (strcmp(prev_wd, "\\ef") == 0)
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_functions, NULL);
+
+ else if (strncmp(prev_wd, "\\sf", 2) == 0)
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_functions, NULL);
else if (strcmp(prev_wd, "\\encoding") == 0)
COMPLETE_WITH_QUERY(Query_for_list_of_encodings);
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
