public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v1] elog.c: Remove special case which avoided %*s format strings.. 3+ messages / 3 participants [nested] [flat]
* [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. @ 2020-08-01 02:53 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2020-08-01 02:53 UTC (permalink / raw) ..which should no longer be needed since it was a performance hack for specific platform snprintf, which are no longer used. See also: 4334639f4 Allow printf-style padding specifications in log_line_prefix. 96bf88d52 Always use our own versions of *printf(). abd9ca377 Make assorted performance improvements in snprintf.c. --- src/backend/utils/error/elog.c | 134 ++++++++------------------------- 1 file changed, 32 insertions(+), 102 deletions(-) diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index d0b368530e..6b6749965f 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -2350,11 +2350,6 @@ log_line_prefix(StringInfo buf, ErrorData *edata) * Note: Since only '-', '0' to '9' are valid formatting characters we * can do a quick check here to pre-check for formatting. If the char * is not formatting then we can skip a useless function call. - * - * Further note: At least on some platforms, passing %*s rather than - * %s to appendStringInfo() is substantially slower, so many of the - * cases below avoid doing that unless non-zero padding is in fact - * specified. */ if (*p > '9') padding = 0; @@ -2371,10 +2366,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata) if (appname == NULL || *appname == '\0') appname = _("[unknown]"); - if (padding != 0) - appendStringInfo(buf, "%*s", padding, appname); - else - appendStringInfoString(buf, appname); + appendStringInfo(buf, "%*s", padding, appname); } else if (padding != 0) appendStringInfoSpaces(buf, @@ -2392,10 +2384,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata) else backend_type_str = GetBackendTypeDesc(MyBackendType); - if (padding != 0) - appendStringInfo(buf, "%*s", padding, backend_type_str); - else - appendStringInfoString(buf, backend_type_str); + appendStringInfo(buf, "%*s", padding, backend_type_str); break; } case 'u': @@ -2405,10 +2394,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata) if (username == NULL || *username == '\0') username = _("[unknown]"); - if (padding != 0) - appendStringInfo(buf, "%*s", padding, username); - else - appendStringInfoString(buf, username); + appendStringInfo(buf, "%*s", padding, username); } else if (padding != 0) appendStringInfoSpaces(buf, @@ -2421,17 +2407,13 @@ log_line_prefix(StringInfo buf, ErrorData *edata) if (dbname == NULL || *dbname == '\0') dbname = _("[unknown]"); - if (padding != 0) - appendStringInfo(buf, "%*s", padding, dbname); - else - appendStringInfoString(buf, dbname); + appendStringInfo(buf, "%*s", padding, dbname); } else if (padding != 0) appendStringInfoSpaces(buf, padding > 0 ? padding : -padding); break; case 'c': - if (padding != 0) { char strfbuf[128]; @@ -2439,14 +2421,9 @@ log_line_prefix(StringInfo buf, ErrorData *edata) (long) (MyStartTime), MyProcPid); appendStringInfo(buf, "%*s", padding, strfbuf); } - else - appendStringInfo(buf, "%lx.%x", (long) (MyStartTime), MyProcPid); break; case 'p': - if (padding != 0) - appendStringInfo(buf, "%*d", padding, MyProcPid); - else - appendStringInfo(buf, "%d", MyProcPid); + appendStringInfo(buf, "%*d", padding, MyProcPid); break; case 'P': @@ -2472,17 +2449,11 @@ log_line_prefix(StringInfo buf, ErrorData *edata) break; case 'l': - if (padding != 0) - appendStringInfo(buf, "%*ld", padding, log_line_number); - else - appendStringInfo(buf, "%ld", log_line_number); + appendStringInfo(buf, "%*ld", padding, log_line_number); break; case 'm': setup_formatted_log_time(); - if (padding != 0) - appendStringInfo(buf, "%*s", padding, formatted_log_time); - else - appendStringInfoString(buf, formatted_log_time); + appendStringInfo(buf, "%*s", padding, formatted_log_time); break; case 't': { @@ -2492,10 +2463,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata) pg_strftime(strfbuf, sizeof(strfbuf), "%Y-%m-%d %H:%M:%S %Z", pg_localtime(&stamp_time, log_timezone)); - if (padding != 0) - appendStringInfo(buf, "%*s", padding, strfbuf); - else - appendStringInfoString(buf, strfbuf); + appendStringInfo(buf, "%*s", padding, strfbuf); } break; case 'n': @@ -2512,19 +2480,13 @@ log_line_prefix(StringInfo buf, ErrorData *edata) (long) saved_timeval.tv_sec, (int) (saved_timeval.tv_usec / 1000)); - if (padding != 0) - appendStringInfo(buf, "%*s", padding, strfbuf); - else - appendStringInfoString(buf, strfbuf); + appendStringInfo(buf, "%*s", padding, strfbuf); } break; case 's': if (formatted_start_time[0] == '\0') setup_formatted_start_time(); - if (padding != 0) - appendStringInfo(buf, "%*s", padding, formatted_start_time); - else - appendStringInfoString(buf, formatted_start_time); + appendStringInfo(buf, "%*s", padding, formatted_start_time); break; case 'i': if (MyProcPort) @@ -2533,10 +2495,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata) int displen; psdisp = get_ps_display(&displen); - if (padding != 0) - appendStringInfo(buf, "%*s", padding, psdisp); - else - appendBinaryStringInfo(buf, psdisp, displen); + appendStringInfo(buf, "%*s", padding, psdisp); } else if (padding != 0) @@ -2546,37 +2505,24 @@ log_line_prefix(StringInfo buf, ErrorData *edata) case 'r': if (MyProcPort && MyProcPort->remote_host) { - if (padding != 0) + if (MyProcPort->remote_port && MyProcPort->remote_port[0] != '\0') { - if (MyProcPort->remote_port && MyProcPort->remote_port[0] != '\0') - { - /* - * This option is slightly special as the port - * number may be appended onto the end. Here we - * need to build 1 string which contains the - * remote_host and optionally the remote_port (if - * set) so we can properly align the string. - */ - - char *hostport; - - hostport = psprintf("%s(%s)", MyProcPort->remote_host, MyProcPort->remote_port); - appendStringInfo(buf, "%*s", padding, hostport); - pfree(hostport); - } - else - appendStringInfo(buf, "%*s", padding, MyProcPort->remote_host); + /* + * This option is slightly special as the port + * number may be appended onto the end. Here we + * need to build 1 string which contains the + * remote_host and optionally the remote_port (if + * set) so we can properly align the string. + */ + + char *hostport; + + hostport = psprintf("%s(%s)", MyProcPort->remote_host, MyProcPort->remote_port); + appendStringInfo(buf, "%*s", padding, hostport); + pfree(hostport); } else - { - /* padding is 0, so we don't need a temp buffer */ - appendStringInfoString(buf, MyProcPort->remote_host); - if (MyProcPort->remote_port && - MyProcPort->remote_port[0] != '\0') - appendStringInfo(buf, "(%s)", - MyProcPort->remote_port); - } - + appendStringInfo(buf, "%*s", padding, MyProcPort->remote_host); } else if (padding != 0) appendStringInfoSpaces(buf, @@ -2584,12 +2530,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata) break; case 'h': if (MyProcPort && MyProcPort->remote_host) - { - if (padding != 0) - appendStringInfo(buf, "%*s", padding, MyProcPort->remote_host); - else - appendStringInfoString(buf, MyProcPort->remote_host); - } + appendStringInfo(buf, "%*s", padding, MyProcPort->remote_host); else if (padding != 0) appendStringInfoSpaces(buf, padding > 0 ? padding : -padding); @@ -2604,32 +2545,21 @@ log_line_prefix(StringInfo buf, ErrorData *edata) /* keep VXID format in sync with lockfuncs.c */ if (MyProc != NULL && MyProc->backendId != InvalidBackendId) { - if (padding != 0) - { - char strfbuf[128]; + char strfbuf[128]; - snprintf(strfbuf, sizeof(strfbuf) - 1, "%d/%u", - MyProc->backendId, MyProc->lxid); + snprintf(strfbuf, sizeof(strfbuf) - 1, "%d/%u", + MyProc->backendId, MyProc->lxid); appendStringInfo(buf, "%*s", padding, strfbuf); - } - else - appendStringInfo(buf, "%d/%u", MyProc->backendId, MyProc->lxid); } else if (padding != 0) appendStringInfoSpaces(buf, padding > 0 ? padding : -padding); break; case 'x': - if (padding != 0) - appendStringInfo(buf, "%*u", padding, GetTopTransactionIdIfAny()); - else - appendStringInfo(buf, "%u", GetTopTransactionIdIfAny()); + appendStringInfo(buf, "%*u", padding, GetTopTransactionIdIfAny()); break; case 'e': - if (padding != 0) - appendStringInfo(buf, "%*s", padding, unpack_sql_state(edata->sqlerrcode)); - else - appendStringInfoString(buf, unpack_sql_state(edata->sqlerrcode)); + appendStringInfo(buf, "%*s", padding, unpack_sql_state(edata->sqlerrcode)); break; default: /* format error - ignore it */ -- 2.17.0 --pAwQNkOnpTn9IO2O-- ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: abi-compliance-checker @ 2024-02-27 13:25 Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Alvaro Herrera @ 2024-02-27 13:25 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On 2023-Nov-01, Peter Eisentraut wrote: > v3-0001-abidw-option.patch > > This adds the abidw meson option, which produces the xml files with the ABI > description. With that, you can then implement a variety of workflows, such > as the abidiff proposed in the later patches, or something rigged up via CI, > or you can just build various versions locally and compare them. With this > patch, you get the files to compare built automatically and don't have to > remember to cover all the libraries or which options to use. > > I think this patch is mostly pretty straightforward and agreeable, subject > to technical review in detail. I like this idea and I think we should integrate it with the objective of it becoming the workhorse of ABI-stability testing. However, I do not think that the subsequent patches should be part of the tree at all; certainly not the produced .xml files in your 0004, as that would be far too unstable and would cause a lot of pointless churn. > TODO: documentation Yes, please. > TODO: Do we want a configure/make variant of this? Not needed IMO. The way I see this working, is that we set up a buildfarm animal [per architecture] that runs the new rules produced by the abidw option and stores the result locally, so that for stable branches it can turn red when an ABI-breaking change with the previous minor release of the same branch is introduced. There's no point on it ever turning red in the master branch, since we're obviously not concerned with ABI changes there. (Perhaps we do need 0003 as an easy mechanism to run the comparison, but I'm not sure to what extent that would be actually helpful.) -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: abi-compliance-checker @ 2024-02-27 13:45 Peter Geoghegan <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Peter Geoghegan @ 2024-02-27 13:45 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Feb 27, 2024 at 8:25 AM Alvaro Herrera <[email protected]> wrote: > The way I see this working, is that we set up a buildfarm animal [per > architecture] that runs the new rules produced by the abidw option and > stores the result locally, so that for stable branches it can turn red > when an ABI-breaking change with the previous minor release of the same > branch is introduced. There's no point on it ever turning red in the > master branch, since we're obviously not concerned with ABI changes there. ABI stability doesn't seem like something that you can alert on. There are quite a few individual cases where the ABI was technically broken, in a way that these tools will complain about. And yet it was generally understood that these changes did not really break ABI stability, for various high-context reasons that no tool can possibly be expected to understand. This will at least be true under our existing practices, or anything like them. For example, if you look at the "Problems with Symbols, High Severity" from the report I posted comparing REL_11_0 to REL_11_20, you'll see that I removed _bt_pagedel() when backpatching a fix. That was justified by the fact that any extension that was calling that function was already hopelessly broken (I pointed this out at the time). Having some tooling in this area would be extremely useful. The absolute number of false positives seems quite manageable, but the fact is that most individual complaints that the tooling makes are false positives. At least in some deeper sense. -- Peter Geoghegan ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-02-27 13:45 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]> 2024-02-27 13:25 Re: abi-compliance-checker Alvaro Herrera <[email protected]> 2024-02-27 13:45 ` Re: abi-compliance-checker Peter Geoghegan <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox