Hi, On 2020-03-19 19:32:55 -0400, Tom Lane wrote: > Andres Freund <and...@anarazel.de> writes: > > On 2020-03-19 14:07:04 -0400, Tom Lane wrote: > >> Now that we can rely on having varargs macros, I think we could > >> stop requiring the extra level of parentheses, > > > I think that'd be an improvement, because: > > ane of the ones I saw confuse people is just: > > /home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: error: > > ‘ereport’ undeclared (first use in this function); did you mean ‘rresvport’? > > 3727 | ereport(FATAL, > > | ^~~~~~~ > > | rresvport > > /home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: note: each > > undeclared identifier is reported only once for each function it appears in > > because the extra parens haven't been added. > > Ah, so the macro isn't expanded at all if it appears to have more than > two parameters? That seems odd, but I suppose some compilers might > work that way. Switching to varargs would improve that for sure.
Yea. Newer gcc versions do warn about a parameter mismatch count, which helps. I'm not sure what the preprocessor / compiler should do intead? FWIW, clang also doesn't expand the macro. > > Another one I've both seen and committed byself is converting an elog to > > an ereport, and not adding an errcode() around the error code - which > > silently looks like it works. > > You mean not adding errmsg() around the error string? Yeah, I can > believe that one easily. It's sort of the same problem as forgetting > to wrap errcode() around the ERRCODE_ constant. Both of these, I think. > Could we restructure things so that the errcode/errmsg/etc calls form > a standalone comma expression rather than appearing to be arguments of > a varargs function? The compiler's got no hope of realizing there's > anything wrong when it thinks it's passing an integer or string > constant to a function it knows nothing about. But if it could see > that nothing at all is being done with the constant, maybe we'd get > somewhere. Worth a try. Not doing a pointless varargs call could also end up reducing elog overhead a bit (right now we push a lot of 0 as vararg arguments for no reason). A quick hack suggests that it works: /home/andres/src/postgresql/src/backend/tcop/postgres.c: In function ‘process_postgres_switches’: /home/andres/src/postgresql/src/backend/tcop/postgres.c:3728:27: warning: left-hand operand of comma expression has no effect [-Wunused-value] 3728 | (ERRCODE_SYNTAX_ERROR, | ^ /home/andres/src/postgresql/src/include/utils/elog.h:126:4: note: in definition of macro ‘ereport_domain’ 126 | rest; \ | ^~~~ /home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: note: in expansion of macro ‘ereport’ 3727 | ereport(FATAL, | ^~~~~~~ and in an optimized build the resulting code indeed looks a bit better: before: text data bss dec hex filename 8462795 176128 204464 8843387 86f07b src/backend/postgres Looking at FloatExceptionHandler as an example: 2860 /* We're not returning, so no need to save errno */ 2861 ereport(ERROR, 0x0000000000458bc0 <+0>: push %rbp 0x0000000000458bc1 <+1>: mov $0xb2d,%edx 0x0000000000458bc6 <+6>: lea 0x214c9b(%rip),%rsi # 0x66d868 0x0000000000458bcd <+13>: mov %rsp,%rbp 0x0000000000458bd0 <+16>: push %r13 0x0000000000458bd2 <+18>: xor %r8d,%r8d 0x0000000000458bd5 <+21>: lea 0x2d9dc4(%rip),%rcx # 0x7329a0 <__func__.41598> 0x0000000000458bdc <+28>: push %r12 0x0000000000458bde <+30>: mov $0x14,%edi 0x0000000000458be3 <+35>: callq 0x5a8c00 <errstart> 0x0000000000458be8 <+40>: lea 0x214e59(%rip),%rdi # 0x66da48 0x0000000000458bef <+47>: xor %eax,%eax 0x0000000000458bf1 <+49>: callq 0x5acb00 <errdetail> 0x0000000000458bf6 <+54>: mov %eax,%r13d 0x0000000000458bf9 <+57>: lea 0x1bf7fb(%rip),%rdi # 0x6183fb 0x0000000000458c00 <+64>: xor %eax,%eax 0x0000000000458c02 <+66>: callq 0x5ac710 <errmsg> 0x0000000000458c07 <+71>: mov $0x1020082,%edi 0x0000000000458c0c <+76>: mov %eax,%r12d 0x0000000000458c0f <+79>: callq 0x5ac5b0 <errcode> 0x0000000000458c14 <+84>: mov %eax,%edi 0x0000000000458c16 <+86>: mov %r13d,%edx 0x0000000000458c19 <+89>: mov %r12d,%esi 0x0000000000458c1c <+92>: xor %eax,%eax 0x0000000000458c1e <+94>: callq 0x5ac020 <errfinish> vs after: text data bss dec hex filename 8395731 176128 204464 8776323 85ea83 src/backend/postgres 2861 ereport(ERROR, 0x0000000000449dd0 <+0>: push %rbp 0x0000000000449dd1 <+1>: xor %r8d,%r8d 0x0000000000449dd4 <+4>: lea 0x2d8a65(%rip),%rcx # 0x722840 <__func__.41598> 0x0000000000449ddb <+11>: mov %rsp,%rbp 0x0000000000449dde <+14>: mov $0xb2d,%edx 0x0000000000449de3 <+19>: lea 0x2137ee(%rip),%rsi # 0x65d5d8 0x0000000000449dea <+26>: mov $0x14,%edi 0x0000000000449def <+31>: callq 0x5992e0 <errstart> 0x0000000000449df4 <+36>: mov $0x1020082,%edi 0x0000000000449df9 <+41>: callq 0x59cc80 <errcode> 0x0000000000449dfe <+46>: lea 0x1be496(%rip),%rdi # 0x60829b 0x0000000000449e05 <+53>: xor %eax,%eax 0x0000000000449e07 <+55>: callq 0x59cde0 <errmsg> 0x0000000000449e0c <+60>: lea 0x2139a5(%rip),%rdi # 0x65d7b8 0x0000000000449e13 <+67>: xor %eax,%eax 0x0000000000449e15 <+69>: callq 0x59d1d0 <errdetail> 0x0000000000449e1a <+74>: callq 0x59c700 <errfinish> I wonder if it'd become a relevant backpatch pain if we started to have some ereports() without the additional parens in 13+. Would it perhaps make sense to backpatch just the part that removes the need for the parents, but not the return type changes? This is patch 0001. We can also remove elog() support code now, because with __VA_ARGS__ support it's really just a wrapper for ereport(elevel, errmsg(__VA_ARGS_)). This is patch 0002. I think it might also be a good idea to move __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain from being parameters to errstart to errfinish. For elevel < ERROR its sad that we currently pass them even if we don't emit the message. This is patch 0003. I wonder if its worth additionally ensuring that errcode, errmsg, ... are only called within errstart/errfinish? We could have errstart() return the error being "prepared" and store it in a local variable, and make errmsg() etc macros that internally pass that variable to the "real" errmsg() etc. Like #define errmsg(...) errmsg_impl(current_error, __VA_ARGS__) and set up current_error in ereport_domain() roughly like do { ErrorData *current_error_; if ((current_error_ = errstart(elevel, the, rest)) != NULL) { ... errfinish() } ... probably not worth it? Greetings, Andres Freund
>From 11d5ead367deacd78b3d2e931fef0c7c27ed2838 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Thu, 19 Mar 2020 18:05:56 -0700 Subject: [PATCH v2 1/3] WIP: elog: Make ereport a bit easier to use and a bit faster. Author: Andres Freund Discussion: https://postgr.es/m/5995.1584660...@sss.pgh.pa.us --- src/include/executor/executor.h | 2 +- src/include/parser/parse_coerce.h | 2 +- src/include/parser/parse_node.h | 2 +- src/include/parser/scanner.h | 2 +- src/include/utils/elog.h | 64 ++++++++++++---------- src/backend/executor/execUtils.c | 8 +-- src/backend/parser/parse_coerce.c | 6 +-- src/backend/parser/parse_node.c | 8 +-- src/backend/parser/scan.l | 6 +-- src/backend/storage/ipc/dsm_impl.c | 8 +-- src/backend/tcop/postgres.c | 14 ++--- src/backend/utils/adt/jsonfuncs.c | 8 +-- src/backend/utils/error/elog.c | 86 +++++++++--------------------- src/pl/plpgsql/src/pl_scanner.c | 6 +-- src/pl/plpgsql/src/plpgsql.h | 2 +- 15 files changed, 98 insertions(+), 126 deletions(-) diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 94890512dc8..cd0e6439cde 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -546,7 +546,7 @@ exec_rt_fetch(Index rti, EState *estate) extern Relation ExecGetRangeTableRelation(EState *estate, Index rti); -extern int executor_errposition(EState *estate, int location); +extern void executor_errposition(EState *estate, int location); extern void RegisterExprContextCallback(ExprContext *econtext, ExprContextCallbackFunction function, diff --git a/src/include/parser/parse_coerce.h b/src/include/parser/parse_coerce.h index 8686eaacbc9..a3295b3fa45 100644 --- a/src/include/parser/parse_coerce.h +++ b/src/include/parser/parse_coerce.h @@ -61,7 +61,7 @@ extern Node *coerce_to_specific_type_typmod(ParseState *pstate, Node *node, Oid targetTypeId, int32 targetTypmod, const char *constructName); -extern int parser_coercion_errposition(ParseState *pstate, +extern void parser_coercion_errposition(ParseState *pstate, int coerce_location, Node *input_expr); diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h index d25819aa28b..b223b595423 100644 --- a/src/include/parser/parse_node.h +++ b/src/include/parser/parse_node.h @@ -307,7 +307,7 @@ typedef struct ParseCallbackState extern ParseState *make_parsestate(ParseState *parentParseState); extern void free_parsestate(ParseState *pstate); -extern int parser_errposition(ParseState *pstate, int location); +extern void parser_errposition(ParseState *pstate, int location); extern void setup_parser_errposition_callback(ParseCallbackState *pcbstate, ParseState *pstate, int location); diff --git a/src/include/parser/scanner.h b/src/include/parser/scanner.h index a27352afc14..02ae10a2250 100644 --- a/src/include/parser/scanner.h +++ b/src/include/parser/scanner.h @@ -140,7 +140,7 @@ extern core_yyscan_t scanner_init(const char *str, extern void scanner_finish(core_yyscan_t yyscanner); extern int core_yylex(core_YYSTYPE *lvalp, YYLTYPE *llocp, core_yyscan_t yyscanner); -extern int scanner_errposition(int location, core_yyscan_t yyscanner); +extern void scanner_errposition(int location, core_yyscan_t yyscanner); extern void setup_scanner_errposition_callback(ScannerCallbackState *scbstate, core_yyscan_t yyscanner, int location); diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 0a4ef029ceb..241457f35bd 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -118,59 +118,65 @@ *---------- */ #ifdef HAVE__BUILTIN_CONSTANT_P -#define ereport_domain(elevel, domain, rest) \ +#define ereport_domain(elevel, domain, ...) \ do { \ pg_prevent_errno_in_scope(); \ if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \ - errfinish rest; \ + { \ + __VA_ARGS__; \ + errfinish(); \ + } \ if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \ pg_unreachable(); \ } while(0) #else /* !HAVE__BUILTIN_CONSTANT_P */ -#define ereport_domain(elevel, domain, rest) \ +#define ereport_domain(elevel, domain, ...) \ do { \ const int elevel_ = (elevel); \ pg_prevent_errno_in_scope(); \ if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \ - errfinish rest; \ + { \ + __VA_ARGS__; \ + errfinish(); \ + } \ if (elevel_ >= ERROR) \ pg_unreachable(); \ } while(0) #endif /* HAVE__BUILTIN_CONSTANT_P */ -#define ereport(elevel, rest) \ - ereport_domain(elevel, TEXTDOMAIN, rest) +#define ereport(elevel, ...) \ + ereport_domain(elevel, TEXTDOMAIN, __VA_ARGS__) #define TEXTDOMAIN NULL extern bool errstart(int elevel, const char *filename, int lineno, const char *funcname, const char *domain); -extern void errfinish(int dummy,...); +extern void errfinish(void); -extern int errcode(int sqlerrcode); +extern void errcode(int sqlerrcode); -extern int errcode_for_file_access(void); -extern int errcode_for_socket_access(void); +extern void errcode_for_file_access(void); +extern void errcode_for_socket_access(void); -extern int errmsg(const char *fmt,...) pg_attribute_printf(1, 2); -extern int errmsg_internal(const char *fmt,...) pg_attribute_printf(1, 2); +extern void errmsg(const char *fmt,...) pg_attribute_printf(1, 2); +extern void errmsg_internal(const char *fmt,...) pg_attribute_printf(1, 2); -extern int errmsg_plural(const char *fmt_singular, const char *fmt_plural, +extern void errmsg_plural(const char *fmt_singular, const char *fmt_plural, unsigned long n,...) pg_attribute_printf(1, 4) pg_attribute_printf(2, 4); -extern int errdetail(const char *fmt,...) pg_attribute_printf(1, 2); -extern int errdetail_internal(const char *fmt,...) pg_attribute_printf(1, 2); +extern void errdetail(const char *fmt,...) pg_attribute_printf(1, 2); +extern void errdetail_internal(const char *fmt,...) pg_attribute_printf(1, 2); -extern int errdetail_log(const char *fmt,...) pg_attribute_printf(1, 2); +extern void errdetail_log(const char *fmt,...) pg_attribute_printf(1, 2); -extern int errdetail_log_plural(const char *fmt_singular, +extern void errdetail_log_plural(const char *fmt_singular, const char *fmt_plural, unsigned long n,...) pg_attribute_printf(1, 4) pg_attribute_printf(2, 4); -extern int errdetail_plural(const char *fmt_singular, const char *fmt_plural, +extern void errdetail_plural(const char *fmt_singular, const char *fmt_plural, unsigned long n,...) pg_attribute_printf(1, 4) pg_attribute_printf(2, 4); -extern int errhint(const char *fmt,...) pg_attribute_printf(1, 2); +extern void errhint(const char *fmt,...) pg_attribute_printf(1, 2); /* * errcontext() is typically called in error context callback functions, not @@ -182,22 +188,22 @@ extern int errhint(const char *fmt,...) pg_attribute_printf(1, 2); */ #define errcontext set_errcontext_domain(TEXTDOMAIN), errcontext_msg -extern int set_errcontext_domain(const char *domain); +extern void set_errcontext_domain(const char *domain); -extern int errcontext_msg(const char *fmt,...) pg_attribute_printf(1, 2); +extern void errcontext_msg(const char *fmt,...) pg_attribute_printf(1, 2); -extern int errhidestmt(bool hide_stmt); -extern int errhidecontext(bool hide_ctx); +extern void errhidestmt(bool hide_stmt); +extern void errhidecontext(bool hide_ctx); -extern int errbacktrace(void); +extern void errbacktrace(void); -extern int errfunction(const char *funcname); -extern int errposition(int cursorpos); +extern void errfunction(const char *funcname); +extern void errposition(int cursorpos); -extern int internalerrposition(int cursorpos); -extern int internalerrquery(const char *query); +extern void internalerrposition(int cursorpos); +extern void internalerrquery(const char *query); -extern int err_generic_string(int field, const char *str); +extern void err_generic_string(int field, const char *str); extern int geterrcode(void); extern int geterrposition(void); diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c index cc5177cc2b9..6deef1c679a 100644 --- a/src/backend/executor/execUtils.c +++ b/src/backend/executor/execUtils.c @@ -832,21 +832,21 @@ UpdateChangedParamSet(PlanState *node, Bitmapset *newchg) * normal non-error case: computing character indexes would be much more * expensive than storing token offsets.) */ -int +void executor_errposition(EState *estate, int location) { int pos; /* No-op if location was not provided */ if (location < 0) - return 0; + return; /* Can't do anything if source text is not available */ if (estate == NULL || estate->es_sourceText == NULL) - return 0; + return; /* Convert offset to character number */ pos = pg_mbstrlen_with_len(estate->es_sourceText, location) + 1; /* And pass it to the ereport mechanism */ - return errposition(pos); + errposition(pos); } /* diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c index 645e4aa4ceb..91d4e99d345 100644 --- a/src/backend/parser/parse_coerce.c +++ b/src/backend/parser/parse_coerce.c @@ -1246,15 +1246,15 @@ coerce_to_specific_type(ParseState *pstate, Node *node, * XXX possibly this is more generally useful than coercion errors; * if so, should rename and place with parser_errposition. */ -int +void parser_coercion_errposition(ParseState *pstate, int coerce_location, Node *input_expr) { if (coerce_location >= 0) - return parser_errposition(pstate, coerce_location); + parser_errposition(pstate, coerce_location); else - return parser_errposition(pstate, exprLocation(input_expr)); + parser_errposition(pstate, exprLocation(input_expr)); } diff --git a/src/backend/parser/parse_node.c b/src/backend/parser/parse_node.c index 6e98fe55fc4..9a2fd924b67 100644 --- a/src/backend/parser/parse_node.c +++ b/src/backend/parser/parse_node.c @@ -106,21 +106,21 @@ free_parsestate(ParseState *pstate) * normal non-error case: computing character indexes would be much more * expensive than storing token offsets.) */ -int +void parser_errposition(ParseState *pstate, int location) { int pos; /* No-op if location was not provided */ if (location < 0) - return 0; + return; /* Can't do anything if source text is not available */ if (pstate == NULL || pstate->p_sourcetext == NULL) - return 0; + return; /* Convert offset to character number */ pos = pg_mbstrlen_with_len(pstate->p_sourcetext, location) + 1; /* And pass it to the ereport mechanism */ - return errposition(pos); + errposition(pos); } diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l index b1ea0cb5384..50ba68abd4f 100644 --- a/src/backend/parser/scan.l +++ b/src/backend/parser/scan.l @@ -1076,18 +1076,18 @@ other . * (essentially, scan.l, parser.c, and gram.y), since it requires the * yyscanner struct to still be available. */ -int +void scanner_errposition(int location, core_yyscan_t yyscanner) { int pos; if (location < 0) - return 0; /* no-op if location is unknown */ + return; /* no-op if location is unknown */ /* Convert byte offset to character number */ pos = pg_mbstrlen_with_len(yyextra->scanbuf, location) + 1; /* And pass it to the ereport mechanism */ - return errposition(pos); + errposition(pos); } /* diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c index 1972aecbedc..515686ca152 100644 --- a/src/backend/storage/ipc/dsm_impl.c +++ b/src/backend/storage/ipc/dsm_impl.c @@ -92,7 +92,7 @@ static bool dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size, void **impl_private, void **mapped_address, Size *mapped_size, int elevel); #endif -static int errcode_for_dynamic_shared_memory(void); +static void errcode_for_dynamic_shared_memory(void); const struct config_enum_entry dynamic_shared_memory_options[] = { #ifdef USE_DSM_POSIX @@ -1030,11 +1030,11 @@ dsm_impl_unpin_segment(dsm_handle handle, void **impl_private) } } -static int +static void errcode_for_dynamic_shared_memory(void) { if (errno == EFBIG || errno == ENOMEM) - return errcode(ERRCODE_OUT_OF_MEMORY); + errcode(ERRCODE_OUT_OF_MEMORY); else - return errcode_for_file_access(); + errcode_for_file_access(); } diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 00c77b66c74..cb8c23e4b76 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3720,15 +3720,15 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx, /* spell the error message a bit differently depending on context */ if (IsUnderPostmaster) ereport(FATAL, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("invalid command-line argument for server process: %s", argv[optind]), - errhint("Try \"%s --help\" for more information.", progname))); + errcode(ERRCODE_SYNTAX_ERROR), + errmsg("invalid command-line argument for server process: %s", argv[optind]), + errhint("Try \"%s --help\" for more information.", progname)); else ereport(FATAL, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("%s: invalid command-line argument: %s", - progname, argv[optind]), - errhint("Try \"%s --help\" for more information.", progname))); + errcode(ERRCODE_SYNTAX_ERROR), + errmsg("%s: invalid command-line argument: %s", + progname, argv[optind]), + errhint("Try \"%s --help\" for more information.", progname)); } /* diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 4b5007e0d6f..321ab9a0db9 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -329,7 +329,7 @@ typedef struct JsObject hash_destroy((jso)->val.json_hash); \ } while (0) -static int report_json_context(JsonLexContext *lex); +static void report_json_context(JsonLexContext *lex); /* semantic action functions for json_object_keys */ static void okeys_object_field_start(void *state, char *fname, bool isnull); @@ -631,7 +631,7 @@ json_ereport_error(JsonParseErrorType error, JsonLexContext *lex) * The return value isn't meaningful, but we make it non-void so that this * can be invoked inside ereport(). */ -static int +static void report_json_context(JsonLexContext *lex) { const char *context_start; @@ -689,8 +689,8 @@ report_json_context(JsonLexContext *lex) prefix = (context_start > line_start) ? "..." : ""; suffix = (lex->token_type != JSON_TOKEN_END && context_end - lex->input < lex->input_length && *context_end != '\n' && *context_end != '\r') ? "..." : ""; - return errcontext("JSON data, line %d: %s%s%s", - line_number, prefix, ctxt, suffix); + errcontext("JSON data, line %d: %s%s%s", + line_number, prefix, ctxt, suffix); } diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index 62eef7b71f4..dc1fda03a8f 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -438,7 +438,7 @@ matches_backtrace_functions(const char *funcname) * See elog.h for the error level definitions. */ void -errfinish(int dummy,...) +errfinish(void) { ErrorData *edata = &errordata[errordata_stack_depth]; int elevel; @@ -605,7 +605,7 @@ errfinish(int dummy,...) * * The code is expected to be represented as per MAKE_SQLSTATE(). */ -int +void errcode(int sqlerrcode) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -614,8 +614,6 @@ errcode(int sqlerrcode) CHECK_STACK_DEPTH(); edata->sqlerrcode = sqlerrcode; - - return 0; /* return value does not matter */ } @@ -628,7 +626,7 @@ errcode(int sqlerrcode) * NOTE: the primary error message string should generally include %m * when this is used. */ -int +void errcode_for_file_access(void) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -686,8 +684,6 @@ errcode_for_file_access(void) edata->sqlerrcode = ERRCODE_INTERNAL_ERROR; break; } - - return 0; /* return value does not matter */ } /* @@ -699,7 +695,7 @@ errcode_for_file_access(void) * NOTE: the primary error message string should generally include %m * when this is used. */ -int +void errcode_for_socket_access(void) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -722,8 +718,6 @@ errcode_for_socket_access(void) edata->sqlerrcode = ERRCODE_INTERNAL_ERROR; break; } - - return 0; /* return value does not matter */ } @@ -819,7 +813,7 @@ errcode_for_socket_access(void) * Note: no newline is needed at the end of the fmt string, since * ereport will provide one for the output methods that need it. */ -int +void errmsg(const char *fmt,...) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -834,14 +828,13 @@ errmsg(const char *fmt,...) MemoryContextSwitchTo(oldcontext); recursion_depth--; - return 0; /* return value does not matter */ } /* * Add a backtrace to the containing ereport() call. This is intended to be * added temporarily during debugging. */ -int +void errbacktrace(void) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -855,8 +848,6 @@ errbacktrace(void) MemoryContextSwitchTo(oldcontext); recursion_depth--; - - return 0; } /* @@ -906,7 +897,7 @@ set_backtrace(ErrorData *edata, int num_skip) * the message because the translation would fail and result in infinite * error recursion. */ -int +void errmsg_internal(const char *fmt,...) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -921,7 +912,6 @@ errmsg_internal(const char *fmt,...) MemoryContextSwitchTo(oldcontext); recursion_depth--; - return 0; /* return value does not matter */ } @@ -929,7 +919,7 @@ errmsg_internal(const char *fmt,...) * errmsg_plural --- add a primary error message text to the current error, * with support for pluralization of the message text */ -int +void errmsg_plural(const char *fmt_singular, const char *fmt_plural, unsigned long n,...) { @@ -945,14 +935,13 @@ errmsg_plural(const char *fmt_singular, const char *fmt_plural, MemoryContextSwitchTo(oldcontext); recursion_depth--; - return 0; /* return value does not matter */ } /* * errdetail --- add a detail error message text to the current error */ -int +void errdetail(const char *fmt,...) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -966,7 +955,6 @@ errdetail(const char *fmt,...) MemoryContextSwitchTo(oldcontext); recursion_depth--; - return 0; /* return value does not matter */ } @@ -979,7 +967,7 @@ errdetail(const char *fmt,...) * messages that seem not worth translating for one reason or another * (typically, that they don't seem to be useful to average users). */ -int +void errdetail_internal(const char *fmt,...) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -993,14 +981,13 @@ errdetail_internal(const char *fmt,...) MemoryContextSwitchTo(oldcontext); recursion_depth--; - return 0; /* return value does not matter */ } /* * errdetail_log --- add a detail_log error message text to the current error */ -int +void errdetail_log(const char *fmt,...) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1014,14 +1001,13 @@ errdetail_log(const char *fmt,...) MemoryContextSwitchTo(oldcontext); recursion_depth--; - return 0; /* return value does not matter */ } /* * errdetail_log_plural --- add a detail_log error message text to the current error * with support for pluralization of the message text */ -int +void errdetail_log_plural(const char *fmt_singular, const char *fmt_plural, unsigned long n,...) { @@ -1036,7 +1022,6 @@ errdetail_log_plural(const char *fmt_singular, const char *fmt_plural, MemoryContextSwitchTo(oldcontext); recursion_depth--; - return 0; /* return value does not matter */ } @@ -1044,7 +1029,7 @@ errdetail_log_plural(const char *fmt_singular, const char *fmt_plural, * errdetail_plural --- add a detail error message text to the current error, * with support for pluralization of the message text */ -int +void errdetail_plural(const char *fmt_singular, const char *fmt_plural, unsigned long n,...) { @@ -1059,14 +1044,13 @@ errdetail_plural(const char *fmt_singular, const char *fmt_plural, MemoryContextSwitchTo(oldcontext); recursion_depth--; - return 0; /* return value does not matter */ } /* * errhint --- add a hint error message text to the current error */ -int +void errhint(const char *fmt,...) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1080,7 +1064,6 @@ errhint(const char *fmt,...) MemoryContextSwitchTo(oldcontext); recursion_depth--; - return 0; /* return value does not matter */ } @@ -1091,7 +1074,7 @@ errhint(const char *fmt,...) * context information. We assume earlier calls represent more-closely-nested * states. */ -int +void errcontext_msg(const char *fmt,...) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1105,7 +1088,6 @@ errcontext_msg(const char *fmt,...) MemoryContextSwitchTo(oldcontext); recursion_depth--; - return 0; /* return value does not matter */ } /* @@ -1127,7 +1109,7 @@ errcontext_msg(const char *fmt,...) * TEXTDOMAIN value that the errstart call did, so order does not matter * so long as errstart initializes context_domain along with domain. */ -int +void set_errcontext_domain(const char *domain) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1137,8 +1119,6 @@ set_errcontext_domain(const char *domain) /* the default text domain is the backend's */ edata->context_domain = domain ? domain : PG_TEXTDOMAIN("postgres"); - - return 0; /* return value does not matter */ } @@ -1147,7 +1127,7 @@ set_errcontext_domain(const char *domain) * * This should be called if the message text already includes the statement. */ -int +void errhidestmt(bool hide_stmt) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1156,8 +1136,6 @@ errhidestmt(bool hide_stmt) CHECK_STACK_DEPTH(); edata->hide_stmt = hide_stmt; - - return 0; /* return value does not matter */ } /* @@ -1166,7 +1144,7 @@ errhidestmt(bool hide_stmt) * This should only be used for verbose debugging messages where the repeated * inclusion of context would bloat the log volume too much. */ -int +void errhidecontext(bool hide_ctx) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1175,8 +1153,6 @@ errhidecontext(bool hide_ctx) CHECK_STACK_DEPTH(); edata->hide_ctx = hide_ctx; - - return 0; /* return value does not matter */ } @@ -1187,7 +1163,7 @@ errhidecontext(bool hide_ctx) * name appear in messages sent to old-protocol clients. Note that the * passed string is expected to be a non-freeable constant string. */ -int +void errfunction(const char *funcname) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1197,14 +1173,12 @@ errfunction(const char *funcname) edata->funcname = funcname; edata->show_funcname = true; - - return 0; /* return value does not matter */ } /* * errposition --- add cursor position to the current error */ -int +void errposition(int cursorpos) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1213,14 +1187,12 @@ errposition(int cursorpos) CHECK_STACK_DEPTH(); edata->cursorpos = cursorpos; - - return 0; /* return value does not matter */ } /* * internalerrposition --- add internal cursor position to the current error */ -int +void internalerrposition(int cursorpos) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1229,8 +1201,6 @@ internalerrposition(int cursorpos) CHECK_STACK_DEPTH(); edata->internalpos = cursorpos; - - return 0; /* return value does not matter */ } /* @@ -1240,7 +1210,7 @@ internalerrposition(int cursorpos) * is intended for use in error callback subroutines that are editorializing * on the layout of the error report. */ -int +void internalerrquery(const char *query) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1256,8 +1226,6 @@ internalerrquery(const char *query) if (query) edata->internalquery = MemoryContextStrdup(edata->assoc_context, query); - - return 0; /* return value does not matter */ } /* @@ -1270,7 +1238,7 @@ internalerrquery(const char *query) * Most potential callers should not use this directly, but instead prefer * higher-level abstractions, such as errtablecol() (see relcache.c). */ -int +void err_generic_string(int field, const char *str) { ErrorData *edata = &errordata[errordata_stack_depth]; @@ -1299,8 +1267,6 @@ err_generic_string(int field, const char *str) elog(ERROR, "unsupported ErrorData field id: %d", field); break; } - - return 0; /* return value does not matter */ } /* @@ -1463,7 +1429,7 @@ elog_finish(int elevel, const char *fmt,...) /* * And let errfinish() finish up. */ - errfinish(0); + errfinish(); } @@ -1749,7 +1715,7 @@ ThrowErrorData(ErrorData *edata) recursion_depth--; /* Process the error. */ - errfinish(0); + errfinish(); } /* @@ -1863,7 +1829,7 @@ pg_re_throw(void) */ error_context_stack = NULL; - errfinish(0); + errfinish(); } /* Doesn't return ... */ diff --git a/src/pl/plpgsql/src/pl_scanner.c b/src/pl/plpgsql/src/pl_scanner.c index 9cea2e42ac4..55b82b8a74c 100644 --- a/src/pl/plpgsql/src/pl_scanner.c +++ b/src/pl/plpgsql/src/pl_scanner.c @@ -468,20 +468,20 @@ plpgsql_peek2(int *tok1_p, int *tok2_p, int *tok1_loc, int *tok2_loc) * parsing of a plpgsql function, since it requires the scanorig string * to still be available. */ -int +void plpgsql_scanner_errposition(int location) { int pos; if (location < 0 || scanorig == NULL) - return 0; /* no-op if location is unknown */ + return; /* no-op if location is unknown */ /* Convert byte offset to character number */ pos = pg_mbstrlen_with_len(scanorig, location) + 1; /* And pass it to the ereport mechanism */ (void) internalerrposition(pos); /* Also pass the function body string */ - return internalerrquery(scanorig); + internalerrquery(scanorig); } /* diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 69df3306fda..325f4f7fd45 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -1319,7 +1319,7 @@ extern void plpgsql_append_source_text(StringInfo buf, extern int plpgsql_peek(void); extern void plpgsql_peek2(int *tok1_p, int *tok2_p, int *tok1_loc, int *tok2_loc); -extern int plpgsql_scanner_errposition(int location); +extern void plpgsql_scanner_errposition(int location); extern void plpgsql_yyerror(const char *message) pg_attribute_noreturn(); extern int plpgsql_location_to_lineno(int location); extern int plpgsql_latest_lineno(void); -- 2.25.0.114.g5b0ca878e0
>From 7bda2a03d0d05034c122df22d476d7bb5c6acb09 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Thu, 19 Mar 2020 18:54:08 -0700 Subject: [PATCH v2 2/3] WIP: reimplement elog() using ereport(). Author: Andres Freund Discussion: https://postgr.es/m/5995.1584660...@sss.pgh.pa.us --- src/include/utils/elog.h | 31 +--------- src/backend/utils/error/elog.c | 103 --------------------------------- 2 files changed, 1 insertion(+), 133 deletions(-) diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 241457f35bd..8404760ae39 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -215,37 +215,8 @@ extern int getinternalerrposition(void); * elog(ERROR, "portal \"%s\" not found", stmt->portalname); *---------- */ -/* - * Using variadic macros, we can give the compiler a hint about the - * call not returning when elevel >= ERROR. See comments for ereport(). - * Note that historically elog() has called elog_start (which saves errno) - * before evaluating "elevel", so we preserve that behavior here. - */ -#ifdef HAVE__BUILTIN_CONSTANT_P #define elog(elevel, ...) \ - do { \ - pg_prevent_errno_in_scope(); \ - elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \ - elog_finish(elevel, __VA_ARGS__); \ - if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \ - pg_unreachable(); \ - } while(0) -#else /* !HAVE__BUILTIN_CONSTANT_P */ -#define elog(elevel, ...) \ - do { \ - pg_prevent_errno_in_scope(); \ - elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \ - { \ - const int elevel_ = (elevel); \ - elog_finish(elevel_, __VA_ARGS__); \ - if (elevel_ >= ERROR) \ - pg_unreachable(); \ - } \ - } while(0) -#endif /* HAVE__BUILTIN_CONSTANT_P */ - -extern void elog_start(const char *filename, int lineno, const char *funcname); -extern void elog_finish(int elevel, const char *fmt,...) pg_attribute_printf(2, 3); + ereport(elevel, errmsg(__VA_ARGS__)) /* Support for constructing error strings separately from ereport() calls */ diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index dc1fda03a8f..141eddeeeba 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -1330,109 +1330,6 @@ getinternalerrposition(void) return edata->internalpos; } - -/* - * elog_start --- startup for old-style API - * - * All that we do here is stash the hidden filename/lineno/funcname - * arguments into a stack entry, along with the current value of errno. - * - * We need this to be separate from elog_finish because there's no other - * C89-compliant way to deal with inserting extra arguments into the elog - * call. (When using C99's __VA_ARGS__, we could possibly merge this with - * elog_finish, but there doesn't seem to be a good way to save errno before - * evaluating the format arguments if we do that.) - */ -void -elog_start(const char *filename, int lineno, const char *funcname) -{ - ErrorData *edata; - - /* Make sure that memory context initialization has finished */ - if (ErrorContext == NULL) - { - /* Oops, hard crash time; very little we can do safely here */ - write_stderr("error occurred at %s:%d before error message processing is available\n", - filename ? filename : "(unknown file)", lineno); - exit(2); - } - - if (++errordata_stack_depth >= ERRORDATA_STACK_SIZE) - { - /* - * Wups, stack not big enough. We treat this as a PANIC condition - * because it suggests an infinite loop of errors during error - * recovery. Note that the message is intentionally not localized, - * else failure to convert it to client encoding could cause further - * recursion. - */ - errordata_stack_depth = -1; /* make room on stack */ - ereport(PANIC, (errmsg_internal("ERRORDATA_STACK_SIZE exceeded"))); - } - - edata = &errordata[errordata_stack_depth]; - if (filename) - { - const char *slash; - - /* keep only base name, useful especially for vpath builds */ - slash = strrchr(filename, '/'); - if (slash) - filename = slash + 1; - } - edata->filename = filename; - edata->lineno = lineno; - edata->funcname = funcname; - /* errno is saved now so that error parameter eval can't change it */ - edata->saved_errno = errno; - - /* Use ErrorContext for any allocations done at this level. */ - edata->assoc_context = ErrorContext; -} - -/* - * elog_finish --- finish up for old-style API - */ -void -elog_finish(int elevel, const char *fmt,...) -{ - 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(edata->assoc_context); - - if (!edata->backtrace && - edata->funcname && - matches_backtrace_functions(edata->funcname)) - set_backtrace(edata, 2); - - edata->message_id = fmt; - EVALUATE_MESSAGE(edata->domain, message, false, false); - - MemoryContextSwitchTo(oldcontext); - recursion_depth--; - - /* - * And let errfinish() finish up. - */ - errfinish(); -} - - /* * Functions to allow construction of error message strings separately from * the ereport() call itself. -- 2.25.0.114.g5b0ca878e0
>From 5231e698e4674085667fa1e729d75580c54d1595 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Thu, 19 Mar 2020 18:57:23 -0700 Subject: [PATCH v2 3/3] WIP: elog: Move file / line / function domain setup to errfinish(). TODO: Need to verify domain handling is equivalent to before (especially vs set_errcontext_domain() etc). Author: Andres Freund Discussion: https://postgr.es/m/5995.1584660...@sss.pgh.pa.us --- src/include/utils/elog.h | 14 ++++---- src/backend/utils/error/elog.c | 59 ++++++++++++++++++---------------- 2 files changed, 39 insertions(+), 34 deletions(-) diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 8404760ae39..7d728043747 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -121,10 +121,10 @@ #define ereport_domain(elevel, domain, ...) \ do { \ pg_prevent_errno_in_scope(); \ - if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \ + if (errstart(elevel)) \ { \ __VA_ARGS__; \ - errfinish(); \ + errfinish(__FILE__, __LINE__, PG_FUNCNAME_MACRO, domain); \ } \ if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \ pg_unreachable(); \ @@ -134,10 +134,10 @@ do { \ const int elevel_ = (elevel); \ pg_prevent_errno_in_scope(); \ - if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \ + if (errstart(elevel_) \ { \ __VA_ARGS__; \ - errfinish(); \ + errfinish(__FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)); \ } \ if (elevel_ >= ERROR) \ pg_unreachable(); \ @@ -149,9 +149,9 @@ #define TEXTDOMAIN NULL -extern bool errstart(int elevel, const char *filename, int lineno, - const char *funcname, const char *domain); -extern void errfinish(void); +extern bool errstart(int elevel); +extern void errfinish(const char *filename, int lineno, + const char *funcname, const char *domain); extern void errcode(int sqlerrcode); diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index 141eddeeeba..c5392b41726 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -223,17 +223,15 @@ err_gettext(const char *str) /* * errstart --- begin an error-reporting cycle * - * Create a stack entry and store the given parameters in it. Subsequently, - * errmsg() and perhaps other routines will be called to further populate - * the stack entry. Finally, errfinish() will be called to actually process - * the error report. + * Create and initialize error stack entry. Subsequently, errmsg() and + * perhaps other routines will be called to further populate the stack entry. + * Finally, errfinish() will be called to actually process the error report. * * Returns true in normal case. Returns false to short-circuit the error * report (if it's a warning or lower and not to be reported anywhere). */ bool -errstart(int elevel, const char *filename, int lineno, - const char *funcname, const char *domain) +errstart(int elevel) { ErrorData *edata; bool output_to_server; @@ -321,8 +319,7 @@ errstart(int elevel, const char *filename, int lineno, if (ErrorContext == NULL) { /* Oops, hard crash time; very little we can do safely here */ - write_stderr("error occurred at %s:%d before error message processing is available\n", - filename ? filename : "(unknown file)", lineno); + write_stderr("error occurred before error message processing is available\n"); exit(2); } @@ -368,20 +365,9 @@ 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; - if (filename) - { - const char *slash; - /* keep only base name, useful especially for vpath builds */ - slash = strrchr(filename, '/'); - if (slash) - filename = slash + 1; - } - edata->filename = filename; - edata->lineno = lineno; - edata->funcname = funcname; /* the default text domain is the backend's */ - edata->domain = domain ? domain : PG_TEXTDOMAIN("postgres"); + edata->domain = PG_TEXTDOMAIN("postgres"); /* initialize context_domain the same way (see set_errcontext_domain()) */ edata->context_domain = edata->domain; /* Select default errcode based on elevel */ @@ -434,11 +420,12 @@ matches_backtrace_functions(const char *funcname) * * Produce the appropriate error report(s) and pop the error stack. * - * If elevel is ERROR or worse, control does not return to the caller. - * See elog.h for the error level definitions. + * If elevel, as passed to errstart(), is ERROR or worse, control does not + * return to the caller. See elog.h for the error level definitions. */ void -errfinish(void) +errfinish(const char *filename, int lineno, const char *funcname, + const char *domain) { ErrorData *edata = &errordata[errordata_stack_depth]; int elevel; @@ -449,6 +436,23 @@ errfinish(void) CHECK_STACK_DEPTH(); elevel = edata->elevel; + if (filename) + { + const char *slash; + + /* keep only base name, useful especially for vpath builds */ + slash = strrchr(filename, '/'); + if (slash) + filename = slash + 1; + } + + edata->filename = filename; + edata->lineno = lineno; + edata->funcname = funcname; + + if (domain) + edata->domain = domain; + /* * Do processing in ErrorContext, which we hope has enough reserved space * to report an error. @@ -1569,8 +1573,7 @@ ThrowErrorData(ErrorData *edata) ErrorData *newedata; MemoryContext oldcontext; - if (!errstart(edata->elevel, edata->filename, edata->lineno, - edata->funcname, NULL)) + if (!errstart(edata->elevel)) return; /* error is not to be reported at all */ newedata = &errordata[errordata_stack_depth]; @@ -1612,7 +1615,8 @@ ThrowErrorData(ErrorData *edata) recursion_depth--; /* Process the error. */ - errfinish(); + errfinish(edata->filename, edata->lineno, + edata->funcname, edata->domain); } /* @@ -1726,7 +1730,8 @@ pg_re_throw(void) */ error_context_stack = NULL; - errfinish(); + errfinish(edata->filename, edata->lineno, + edata->funcname, edata->domain); } /* Doesn't return ... */ -- 2.25.0.114.g5b0ca878e0