vpath builds and verbose error messages
When using verbose error messages (psql \set VERBOSITY verbose) with a
vpath build, you get this sort of thing:
ERROR: 42703: column "foo" does not exist
LINE 1: select foo;
^
LOCATION: transformColumnRef, /build/buildd-postgresql-9.1_9.1.1-3-i386-AP0ovQ/postgresql-9.1-9.1.1/build/../src/backend/parser/parse_expr.c:766
Can we shorten that path somehow? Either in the C code when printing it
out, or during the build. Any ideas?
Excerpts from Peter Eisentraut's message of vie nov 18 01:34:18 -0300 2011:
When using verbose error messages (psql \set VERBOSITY verbose) with a
vpath build, you get this sort of thing:ERROR: 42703: column "foo" does not exist
LINE 1: select foo;
^
LOCATION: transformColumnRef, /build/buildd-postgresql-9.1_9.1.1-3-i386-AP0ovQ/postgresql-9.1-9.1.1/build/../src/backend/parser/parse_expr.c:766Can we shorten that path somehow? Either in the C code when printing it
out, or during the build. Any ideas?
Huh, I hadn't noticed, even though I see those all the time. I agree
with shortening the path so that it's relative to the root source dir,
if that's what you propose:
LOCATION: transformColumnRef, src/backend/parser/parse_expr.c:766
If it can be done at build time that would be best, because we wouldn't
be carrying thousands of useless copies of the source path ... I have
no suggestions on how to do this, however.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Peter Eisentraut's message of vie nov 18 01:34:18 -0300 2011:
When using verbose error messages (psql \set VERBOSITY verbose) with a
vpath build, you get this sort of thing:
LOCATION: transformColumnRef, /build/buildd-postgresql-9.1_9.1.1-3-i386-AP0ovQ/postgresql-9.1-9.1.1/build/../src/backend/parser/parse_expr.c:766Can we shorten that path somehow? Either in the C code when printing it
out, or during the build. Any ideas?
Huh, I hadn't noticed, even though I see those all the time. I agree
with shortening the path so that it's relative to the root source dir,
if that's what you propose:
In a non-vpath build you just get the file name (or at least that's what
I'm used to seeing). I agree with Peter that a full path seems a bit
over the top.
It wouldn't be that hard for elog.c to do strrchr(fname, '/') or
something like that, but the idea that there are hundreds of full-path
strings embedded in the executable is a bit annoying. I guess we could
hope that the compiler is bright enough to store it only once per source
file, so the actual space requirement may not be all *that* bad.
regards, tom lane
On fre, 2011-11-18 at 09:44 -0500, Tom Lane wrote:
It wouldn't be that hard for elog.c to do strrchr(fname, '/') or
something like that, but the idea that there are hundreds of full-path
strings embedded in the executable is a bit annoying. I guess we
could
hope that the compiler is bright enough to store it only once per
source
file, so the actual space requirement may not be all *that* bad.
A look a the output of "strings" shows that the compiler does appear to
optimize this. So your suggestion is probably the way to go.
One thing that isn't so nice about all this is that it embeds the
personal directory structure of the builder of the binary into the
shipped product. But gcc's cpp doesn't like redefining __FILE__, so the
only way to get around that altogether would be to use something other
than __FILE__ and define that for all builds. Might not be worth it.
Excerpts from Peter Eisentraut's message of mar nov 22 16:22:15 -0300 2011:
One thing that isn't so nice about all this is that it embeds the
personal directory structure of the builder of the binary into the
shipped product. But gcc's cpp doesn't like redefining __FILE__, so the
only way to get around that altogether would be to use something other
than __FILE__ and define that for all builds. Might not be worth it.
This is similar to what's suggested here:
http://stackoverflow.com/questions/1591873/how-do-i-write-a-cpp-dir-macro-similar-to-file
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Peter Eisentraut <peter_e@gmx.net> writes:
One thing that isn't so nice about all this is that it embeds the
personal directory structure of the builder of the binary into the
shipped product. But gcc's cpp doesn't like redefining __FILE__, so the
only way to get around that altogether would be to use something other
than __FILE__ and define that for all builds. Might not be worth it.
Well, if you have a problem with that, don't use a vpath build. I don't
think it's our job to work around gcc behaviors that someone else might
feel to be security issues --- they should take that up with the gcc
maintainers. To me, the only argument for doing anything about this at
all is that we'd like Postgres' behavior (in terms of what it prints in
error messages) to be consistent across different build scenarios.
regards, tom lane
On fre, 2011-11-18 at 09:44 -0500, Tom Lane wrote:
It wouldn't be that hard for elog.c to do strrchr(fname, '/') or
something like that,
Here is a patch for that. I would also like to backpatch this.
Attachments:
ereport-vpath-filename.patchtext/x-patch; charset=UTF-8; name=ereport-vpath-filename.patchDownload
diff --git i/src/backend/utils/error/elog.c w/src/backend/utils/error/elog.c
index 9a99fc7..0131c06 100644
--- i/src/backend/utils/error/elog.c
+++ w/src/backend/utils/error/elog.c
@@ -343,7 +343,14 @@ errstart(int elevel, const char *filename, int lineno,
edata->elevel = elevel;
edata->output_to_server = output_to_server;
edata->output_to_client = output_to_client;
- edata->filename = filename;
+ if (filename)
+ {
+ const char *slash;
+
+ /* keep only base name, useful especially for vpath builds */
+ slash = strrchr(filename, '/');
+ edata->filename = slash ? slash + 1 : filename;
+ }
edata->lineno = lineno;
edata->funcname = funcname;
/* the default text domain is the backend's */
@@ -1142,7 +1149,14 @@ elog_start(const char *filename, int lineno, const char *funcname)
}
edata = &errordata[errordata_stack_depth];
- edata->filename = filename;
+ if (filename)
+ {
+ const char *slash;
+
+ /* keep only base name, useful especially for vpath builds */
+ slash = strrchr(filename, '/');
+ edata->filename = slash ? slash + 1 : filename;
+ }
edata->lineno = lineno;
edata->funcname = funcname;
/* errno is saved now so that error parameter eval can't change it */
Peter Eisentraut <peter_e@gmx.net> writes:
On fre, 2011-11-18 at 09:44 -0500, Tom Lane wrote:
It wouldn't be that hard for elog.c to do strrchr(fname, '/') or
something like that,
Here is a patch for that. I would also like to backpatch this.
Hmmm ... is it possible that strrchr could change errno? If so we'd
need to re-order the operations a bit. Otherwise this seems fine.
regards, tom lane
On lör, 2011-11-26 at 10:45 -0500, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
On fre, 2011-11-18 at 09:44 -0500, Tom Lane wrote:
It wouldn't be that hard for elog.c to do strrchr(fname, '/') or
something like that,Here is a patch for that. I would also like to backpatch this.
Hmmm ... is it possible that strrchr could change errno?
It's not documented to do that, and an implementation would have to go
far out of it's way to do it anyway.