On 2013-01-11 14:01:40 -0500, Tom Lane wrote:
> Andres Freund <and...@2ndquadrant.com> writes:
> > The attached patch:
>
> > * adds configure checks for __VA_ARGS__ and __builtin_unreachable
> > support
> > * provides a pg_unreachable() macro that expands to
> > __builtin_unreachable() or abort()
> > * changes #define elog ... into #define elog(elevel, ...) if varargs are
> > available
>
> Seems like a good thing to do --- will review and commit.
Thanks.
I guess you will catch that anyway, but afaik msvc supports __VA_ARGS__
these days (since v2005 seemingly) and I didn't add HAVE__VA_ARGS to the
respective pg_config.h.win32
> > It does *not* combine elog_start and elog_finish into one function if
> > varargs are available although that brings a rather measurable
> > size/performance benefit.
>
> Since you've apparently already done the measurement: how much?
> It would be a bit tedious to support two different infrastructures for
> elog(), but if it's a big enough win maybe we should.
Imo its pretty definitely a big enough win. So big I have a hard time
believing it can be true without negative effects somewhere else.
Ontop of the patch youve replied to it saves somewhere around 80kb and
between 0.8 (-S -M prepared), 2% (-S -M simple) and 4% (own stuff) I had
lying around and it consistently gives 6%-7% in Pavel's testcase. With
todays HEAD, not with the one before your fix.
I attached the absolutely ugly and unready patch I used for testing.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index e710f22..c1f3200 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1151,6 +1151,43 @@ getinternalerrposition(void)
return edata->internalpos;
}
+#if defined(HAVE__VA_ARGS) && defined(COMBINE_ELOG)
+void
+elog_all(const char *filename, int lineno, const char *funcname, int elevel, const char *fmt,...)
+{
+ elog_start(filename, lineno, funcname);
+ {
+ ErrorData *edata = &errordata[errordata_stack_depth];
+ MemoryContext oldcontext;
+
+ CHECK_STACK_DEPTH();
+
+ /*
+ * Do errstart() to see if we actually want to report the message.
+ */
+ errordata_stack_depth--;
+ errno = edata->saved_errno;
+ if (!errstart(elevel, edata->filename, edata->lineno, edata->funcname, NULL))
+ return; /* nothing to do */
+
+ /*
+ * Format error message just like errmsg_internal().
+ */
+ recursion_depth++;
+ oldcontext = MemoryContextSwitchTo(ErrorContext);
+
+ EVALUATE_MESSAGE(edata->domain, message, false, false);
+
+ MemoryContextSwitchTo(oldcontext);
+ recursion_depth--;
+
+ /*
+ * And let errfinish() finish up.
+ */
+ errfinish(0);
+ }
+}
+#endif
/*
* elog_start --- startup for old-style API
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index d6e1054..9af530f 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -216,7 +216,15 @@ extern int getinternalerrposition(void);
* generated.
*----------
*/
-#ifdef HAVE__VA_ARGS
+#define COMBINE_ELOG
+#if defined(HAVE__VA_ARGS) && defined(COMBINE_ELOG)
+extern void elog_all(const char *filename, int lineno, const char *funcname, int elevel, const char *fmt,...)
+__attribute__((format(PG_PRINTF_ATTRIBUTE, 5, 6)));
+
+#define elog(elevel, ...) \
+ elog_all(__FILE__, __LINE__, PG_FUNCNAME_MACRO, elevel, __VA_ARGS__), \
+ ((elevel) >= ERROR ? pg_unreachable() : (void) 0)
+#elif defined(HAVE__VA_ARGS)
#define elog(elevel, ...) \
elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO), \
elog_finish(elevel, __VA_ARGS__), \
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
