public inbox for [email protected]  

help / color / mirror / Atom feed

[PATCH v1] elog.c: Remove special case which avoided %*s format strings..
21+ messages / 2 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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* [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; 21+ 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] 21+ messages in thread

* Strange presentaion related to inheritance in \d+
@ 2023-08-28 07:16 Kyotaro Horiguchi <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Kyotaro Horiguchi @ 2023-08-28 07:16 UTC (permalink / raw)
  To: [email protected]

While translating a message, I found a questionable behavior in \d+,
introduced by a recent commit b0e96f3119. In short, the current code
hides the constraint's origin when "NO INHERIT" is used.

For these tables:

create table p (a int, b int not null default 0);
create table c1 (a int, b int not null default 1) inherits (p);

The output from "\d+ c1" contains the lines:

> Not-null constraints:
>    "c1_b_not_null" NOT NULL "b" *(local, inherited)*

But with these tables:

create table p (a int, b int not null default 0);
create table c1 (a int, b int not null NO INHERIT default 1) inherits (p);

I get:

> Not-null constraints:
>    "c1_b_not_null" NOT NULL "b" *NO INHERIT*

Here, "NO INHERIT" is mapped from connoinherit, and conislocal and
"coninhcount <> 0" align with "local" and "inherited". For a clearer
picuture, those values for c1 are as follows.

=# SELECT co.conname, at.attname, co.connoinherit, co.conislocal, co.coninhcount FROM pg_catalog.pg_constraint co JOIN pg_catalog.pg_attribute at ON (at.attnum = co.conkey[1]) WHERE co.contype = 'n' AND co.conrelid = 'c1'::pg_catalog.regclass AND at.attrelid = 'c1'::pg_catalog.regclass ORDER BY at.attnum;
    conname    | attname | connoinherit | conislocal | coninhcount 
---------------+---------+--------------+------------+-------------
 c1_b_not_null | b       | t            | t          |           1

It feels off to me, but couldn't find any discussion about it. Is it
the intended behavior? I believe it's more appropriate to show the
origins even when specifed as NO INHERIT.

======
If not so, the following change might be possible, which is quite simple.

> Not-null constraints:
>    "c1_b_not_null" NOT NULL "b" NO INHERIT(local, inherited)

However, it looks somewhat strange as the information in parentheses
is not secondary to "NO INHERIT".  Thus, perhaps a clearer or more
proper representation would be:

>    "c1_b_not_null" NOT NULL "b" (local, inherited, not inheritable)

That being said, I don't come up with a simple way to do this for now..
(Note that we need to translate the puctuations and the words.)

There's no need to account for all combinations. "Local" and
"inherited" don't be false at the same time and the combination (local
& !inherited) is not displayed. Given these factors, we're left with 6
possible combinations, which I don't think aren't worth the hassle:

(local, inherited, not inheritable)
(inherited, not inheritable) # I didn't figure out how to cause this.
(not inheritable)
(local, inherited)
(inherited)
"" (empty string, means local)

A potential solution that comes to mind is presenting the attributes
in a space sparated list after a colon as attached. (Honestly, I'm not
fond of the format and the final term, though.)

>    "c1_b_not_null" NOT NULL "b": local inherited uninheritable

In 0001, I did wonder about hiding "local" when it's not inherited,
but this behavior rfollows existing code.

In 0002, I'm not completely satisfied with the location, but standard
regression test suite seems more suitable for this check than the TAP
test suite used for testing psql.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


Attachments:

  [text/x-patch] 0001-Fix-not-null-constraint-representation-in-d.patch (21.0K, ../../[email protected]/2-0001-Fix-not-null-constraint-representation-in-d.patch)
  download | inline diff:
From 6c8511499d5dbfc769c38b32292d415fa8982707 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email protected]>
Date: Mon, 28 Aug 2023 14:38:58 +0900
Subject: [PATCH 1/2] Fix not-null constraint representation in \d+

The recent commit b0e96f3119 added the description about not-null
constraints in the output of \d+ command. It hided constraints' origin
when it is marked as NO INHERIT.  Show their origin irrespective of
the NO INHERIT state.
---
 src/bin/psql/describe.c                       | 22 +++++++---
 src/test/regress/expected/constraints.out     | 12 ++---
 src/test/regress/expected/create_table.out    |  6 +--
 .../regress/expected/create_table_like.out    |  6 +--
 src/test/regress/expected/foreign_data.out    | 44 +++++++++----------
 src/test/regress/expected/generated.out       |  2 +-
 src/test/regress/expected/inherit.out         | 30 ++++++-------
 7 files changed, 66 insertions(+), 56 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..3bf1c0cb97 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3079,16 +3079,26 @@ describeOneTableDetails(const char *schemaname,
 			/* Might be an empty set - that's ok */
 			for (i = 0; i < tuples; i++)
 			{
+				bool		noinherit = PQgetvalue(result, i, 2)[0] == 't';
 				bool		islocal = PQgetvalue(result, i, 3)[0] == 't';
 				bool		inherited = PQgetvalue(result, i, 4)[0] == 't';
 
-				printfPQExpBuffer(&buf, "    \"%s\" NOT NULL \"%s\"%s",
+				printfPQExpBuffer(&buf, "    \"%s\" NOT NULL \"%s\"",
 								  PQgetvalue(result, i, 0),
-								  PQgetvalue(result, i, 1),
-								  PQgetvalue(result, i, 2)[0] == 't' ?
-								  " NO INHERIT" :
-								  islocal && inherited ? _(" (local, inherited)") :
-								  inherited ? _(" (inherited)") : "");
+								  PQgetvalue(result, i, 1));
+				if (inherited || noinherit)
+				{
+					appendPQExpBufferChar(&buf, ':');
+
+					if (inherited)
+					{
+						if (islocal)
+							appendPQExpBufferStr(&buf, _(" local"));
+						appendPQExpBufferStr(&buf, _(" inherited"));
+					}
+					if (noinherit)
+						appendPQExpBufferStr(&buf, _(" uninheritable"));
+				}
 
 				printTableAddFooter(&cont, buf.data);
 			}
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index b7de50ad6a..d4adbaa518 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -914,7 +914,7 @@ ALTER TABLE cnn_parent ADD PRIMARY KEY (b);
  a      | integer |           |          |         | plain   |              | 
  b      | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "cnn_grandchild_b_not_null" NOT NULL "b" (local, inherited)
+    "cnn_grandchild_b_not_null" NOT NULL "b": local inherited
 Inherits: cnn_child
 Child tables: cnn_grandchild2
 
@@ -925,7 +925,7 @@ Child tables: cnn_grandchild2
  a      | integer |           |          |         | plain   |              | 
  b      | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "cnn_grandchild_b_not_null" NOT NULL "b" (inherited)
+    "cnn_grandchild_b_not_null" NOT NULL "b": inherited
 Inherits: cnn_grandchild,
           cnn_child2
 
@@ -951,7 +951,7 @@ ERROR:  multiple primary keys for table "cnn_parent" are not allowed
  a      | integer |           |          |         | plain   |              | 
  b      | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "cnn_grandchild_b_not_null" NOT NULL "b" (local, inherited)
+    "cnn_grandchild_b_not_null" NOT NULL "b": local inherited
 Inherits: cnn_child
 Child tables: cnn_grandchild2
 
@@ -962,7 +962,7 @@ Child tables: cnn_grandchild2
  a      | integer |           |          |         | plain   |              | 
  b      | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "cnn_grandchild_b_not_null" NOT NULL "b" (inherited)
+    "cnn_grandchild_b_not_null" NOT NULL "b": inherited
 Inherits: cnn_grandchild,
           cnn_child2
 
@@ -988,7 +988,7 @@ ALTER TABLE cnn_parent ADD PRIMARY KEY USING INDEX b_uq;
  a      | integer |           |          |         | plain   |              | 
  b      | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "cnn_grandchild_b_not_null" NOT NULL "b" (local, inherited)
+    "cnn_grandchild_b_not_null" NOT NULL "b": local inherited
 Inherits: cnn_child
 Child tables: cnn_grandchild2
 
@@ -999,7 +999,7 @@ Child tables: cnn_grandchild2
  a      | integer |           |          |         | plain   |              | 
  b      | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "cnn_grandchild_b_not_null" NOT NULL "b" (inherited)
+    "cnn_grandchild_b_not_null" NOT NULL "b": inherited
 Inherits: cnn_grandchild,
           cnn_child2
 
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 344d05233a..293e4292e9 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -855,7 +855,7 @@ drop table test_part_coll_posix;
 Partition of: parted FOR VALUES IN ('b')
 Partition constraint: ((a IS NOT NULL) AND (a = 'b'::text))
 Not-null constraints:
-    "part_b_b_not_null" NOT NULL "b" (local, inherited)
+    "part_b_b_not_null" NOT NULL "b": local inherited
 
 -- Both partition bound and partition key in describe output
 \d+ part_c
@@ -868,7 +868,7 @@ Partition of: parted FOR VALUES IN ('c')
 Partition constraint: ((a IS NOT NULL) AND (a = 'c'::text))
 Partition key: RANGE (b)
 Not-null constraints:
-    "part_c_b_not_null" NOT NULL "b" (local, inherited)
+    "part_c_b_not_null" NOT NULL "b": local inherited
 Partitions: part_c_1_10 FOR VALUES FROM (1) TO (10)
 
 -- a level-2 partition's constraint will include the parent's expressions
@@ -881,7 +881,7 @@ Partitions: part_c_1_10 FOR VALUES FROM (1) TO (10)
 Partition of: part_c FOR VALUES FROM (1) TO (10)
 Partition constraint: ((a IS NOT NULL) AND (a = 'c'::text) AND (b IS NOT NULL) AND (b >= 1) AND (b < 10))
 Not-null constraints:
-    "part_c_b_not_null" NOT NULL "b" (inherited)
+    "part_c_b_not_null" NOT NULL "b": inherited
 
 -- Show partition count in the parent's describe output
 -- Tempted to include \d+ output listing partitions with bound info but
diff --git a/src/test/regress/expected/create_table_like.out b/src/test/regress/expected/create_table_like.out
index 61956773ff..6604d0c7fa 100644
--- a/src/test/regress/expected/create_table_like.out
+++ b/src/test/regress/expected/create_table_like.out
@@ -360,7 +360,7 @@ NOTICE:  merging constraint "ctlt1_a_check" with inherited definition
 Check constraints:
     "ctlt1_a_check" CHECK (length(a) > 2)
 Not-null constraints:
-    "ctlt1_inh_a_not_null" NOT NULL "a" (local, inherited)
+    "ctlt1_inh_a_not_null" NOT NULL "a": local inherited
 Inherits: ctlt1
 
 SELECT description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt1_inh'::regclass;
@@ -383,7 +383,7 @@ Check constraints:
     "ctlt3_a_check" CHECK (length(a) < 5)
     "ctlt3_c_check" CHECK (length(c) < 7)
 Not-null constraints:
-    "ctlt13_inh_a_not_null" NOT NULL "a" (inherited)
+    "ctlt13_inh_a_not_null" NOT NULL "a": inherited
 Inherits: ctlt1,
           ctlt3
 
@@ -403,7 +403,7 @@ Check constraints:
     "ctlt3_a_check" CHECK (length(a) < 5)
     "ctlt3_c_check" CHECK (length(c) < 7)
 Not-null constraints:
-    "ctlt13_like_a_not_null" NOT NULL "a" (inherited)
+    "ctlt13_like_a_not_null" NOT NULL "a": inherited
 Inherits: ctlt1
 
 SELECT description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt13_like'::regclass;
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 1dfe23cc1e..17cdd7e5b3 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -1421,7 +1421,7 @@ Child tables: ft2, FOREIGN
  c2     | text    |           |          |         |             | extended |              | 
  c3     | date    |           |          |         |             | plain    |              | 
 Not-null constraints:
-    "fd_pt1_c1_not_null" NOT NULL "c1" (inherited)
+    "fd_pt1_c1_not_null" NOT NULL "c1": inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1474,7 +1474,7 @@ Child tables: ft2, FOREIGN
  c2     | text    |           |          |         |             | extended |              | 
  c3     | date    |           |          |         |             | plain    |              | 
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+    "ft2_c1_not_null" NOT NULL "c1": local inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1497,7 +1497,7 @@ NOTICE:  merging column "c3" with inherited definition
  c2     | text    |           |          |         |             | extended |              | 
  c3     | date    |           |          |         |             | plain    |              | 
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+    "ft2_c1_not_null" NOT NULL "c1": local inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1512,7 +1512,7 @@ Child tables: ct3,
  c2     | text    |           |          |         | extended |              | 
  c3     | date    |           |          |         | plain    |              | 
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (inherited)
+    "ft2_c1_not_null" NOT NULL "c1": inherited
 Inherits: ft2
 
 \d+ ft3
@@ -1523,7 +1523,7 @@ Inherits: ft2
  c2     | text    |           |          |         |             | extended |              | 
  c3     | date    |           |          |         |             | plain    |              | 
 Not-null constraints:
-    "ft3_c1_not_null" NOT NULL "c1" (local, inherited)
+    "ft3_c1_not_null" NOT NULL "c1": local inherited
 Server: s0
 Inherits: ft2
 
@@ -1563,8 +1563,8 @@ Child tables: ft2, FOREIGN
  c7     | integer |           | not null |         |             | plain    |              | 
  c8     | integer |           |          |         |             | plain    |              | 
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
-    "fd_pt1_c7_not_null" NOT NULL "c7" (inherited)
+    "ft2_c1_not_null" NOT NULL "c1": local inherited
+    "fd_pt1_c7_not_null" NOT NULL "c7": inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1584,8 +1584,8 @@ Child tables: ct3,
  c7     | integer |           | not null |         | plain    |              | 
  c8     | integer |           |          |         | plain    |              | 
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (inherited)
-    "fd_pt1_c7_not_null" NOT NULL "c7" (inherited)
+    "ft2_c1_not_null" NOT NULL "c1": inherited
+    "fd_pt1_c7_not_null" NOT NULL "c7": inherited
 Inherits: ft2
 
 \d+ ft3
@@ -1601,8 +1601,8 @@ Inherits: ft2
  c7     | integer |           | not null |         |             | plain    |              | 
  c8     | integer |           |          |         |             | plain    |              | 
 Not-null constraints:
-    "ft3_c1_not_null" NOT NULL "c1" (local, inherited)
-    "fd_pt1_c7_not_null" NOT NULL "c7" (inherited)
+    "ft3_c1_not_null" NOT NULL "c1": local inherited
+    "fd_pt1_c7_not_null" NOT NULL "c7": inherited
 Server: s0
 Inherits: ft2
 
@@ -1649,8 +1649,8 @@ Child tables: ft2, FOREIGN
  c7     | integer |           |          |         |             | plain    |              | 
  c8     | text    |           |          |         |             | external |              | 
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
-    "fd_pt1_c6_not_null" NOT NULL "c6" (inherited)
+    "ft2_c1_not_null" NOT NULL "c1": local inherited
+    "fd_pt1_c6_not_null" NOT NULL "c6": inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1682,7 +1682,7 @@ Child tables: ft2, FOREIGN
  c2     | text    |           |          |         |             | extended |              | 
  c3     | date    |           |          |         |             | plain    |              | 
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+    "ft2_c1_not_null" NOT NULL "c1": local inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1729,7 +1729,7 @@ Child tables: ft2, FOREIGN
 Check constraints:
     "fd_pt1chk2" CHECK (c2 <> ''::text)
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+    "ft2_c1_not_null" NOT NULL "c1": local inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1780,7 +1780,7 @@ Child tables: ft2, FOREIGN
 Check constraints:
     "fd_pt1chk2" CHECK (c2 <> ''::text)
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+    "ft2_c1_not_null" NOT NULL "c1": local inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1815,7 +1815,7 @@ Check constraints:
     "fd_pt1chk2" CHECK (c2 <> ''::text)
     "fd_pt1chk3" CHECK (c2 <> ''::text) NOT VALID
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+    "ft2_c1_not_null" NOT NULL "c1": local inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1846,7 +1846,7 @@ Check constraints:
     "fd_pt1chk2" CHECK (c2 <> ''::text)
     "fd_pt1chk3" CHECK (c2 <> ''::text)
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+    "ft2_c1_not_null" NOT NULL "c1": local inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1881,7 +1881,7 @@ Check constraints:
     "f2_check" CHECK (f2 <> ''::text)
     "fd_pt1chk2" CHECK (f2 <> ''::text)
 Not-null constraints:
-    "ft2_c1_not_null" NOT NULL "f1" (local, inherited)
+    "ft2_c1_not_null" NOT NULL "f1": local inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 Inherits: fd_pt1
@@ -1942,7 +1942,7 @@ Partitions: fd_pt2_1 FOR VALUES IN (1), FOREIGN
 Partition of: fd_pt2 FOR VALUES IN (1)
 Partition constraint: ((c1 IS NOT NULL) AND (c1 = 1))
 Not-null constraints:
-    "fd_pt2_c1_not_null" NOT NULL "c1" (inherited)
+    "fd_pt2_c1_not_null" NOT NULL "c1": inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 
@@ -2024,7 +2024,7 @@ Partitions: fd_pt2_1 FOR VALUES IN (1), FOREIGN
 Partition of: fd_pt2 FOR VALUES IN (1)
 Partition constraint: ((c1 IS NOT NULL) AND (c1 = 1))
 Not-null constraints:
-    "fd_pt2_1_c1_not_null" NOT NULL "c1" (inherited)
+    "fd_pt2_1_c1_not_null" NOT NULL "c1": inherited
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
 
@@ -2058,7 +2058,7 @@ Partition constraint: ((c1 IS NOT NULL) AND (c1 = 1))
 Check constraints:
     "p21chk" CHECK (c2 <> ''::text)
 Not-null constraints:
-    "fd_pt2_1_c1_not_null" NOT NULL "c1" (inherited)
+    "fd_pt2_1_c1_not_null" NOT NULL "c1": inherited
     "fd_pt2_1_c3_not_null" NOT NULL "c3"
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index dc97ed3fe0..4a1744f129 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -316,7 +316,7 @@ NOTICE:  merging column "b" with inherited definition
  b      | integer |           |          | generated always as (a * 22) stored | plain   |              | 
  x      | integer |           |          |                                     | plain   |              | 
 Not-null constraints:
-    "gtestx_a_not_null" NOT NULL "a" (inherited)
+    "gtestx_a_not_null" NOT NULL "a": inherited
 Inherits: gtest1
 
 CREATE TABLE gtestxx_1 (a int NOT NULL, b int);
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 6daca12340..c80e398f71 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2007,7 +2007,7 @@ Child tables: cc2
  f4     | double precision |           |          |         | plain    |              | 
  a2     | integer          |           | not null |         | plain    |              | 
 Not-null constraints:
-    "nn" NOT NULL "a2" (inherited)
+    "nn" NOT NULL "a2": inherited
 Inherits: pp1,
           cc1
 
@@ -2031,7 +2031,7 @@ Child tables: cc1,
  f3     | integer |           |          |         | plain    |              | 
  a2     | integer |           | not null |         | plain    |              | 
 Not-null constraints:
-    "pp1_f1_not_null" NOT NULL "f1" (inherited)
+    "pp1_f1_not_null" NOT NULL "f1": inherited
     "nn" NOT NULL "a2"
 Inherits: pp1
 Child tables: cc2
@@ -2046,8 +2046,8 @@ Child tables: cc2
  f4     | double precision |           |          |         | plain    |              | 
  a2     | integer          |           | not null |         | plain    |              | 
 Not-null constraints:
-    "pp1_f1_not_null" NOT NULL "f1" (inherited)
-    "nn" NOT NULL "a2" (inherited)
+    "pp1_f1_not_null" NOT NULL "f1": inherited
+    "nn" NOT NULL "a2": inherited
 Inherits: pp1,
           cc1
 
@@ -2065,7 +2065,7 @@ alter table cc1 alter column a2 drop not null;
  f3     | integer |           |          |         | plain    |              | 
  a2     | integer |           |          |         | plain    |              | 
 Not-null constraints:
-    "pp1_f1_not_null" NOT NULL "f1" (inherited)
+    "pp1_f1_not_null" NOT NULL "f1": inherited
 Inherits: pp1
 Child tables: cc2
 
@@ -2082,7 +2082,7 @@ ERROR:  cannot drop inherited constraint "pp1_f1_not_null" of relation "cc2"
  f4     | double precision |           |          |         | plain    |              | 
  a2     | integer          |           |          |         | plain    |              | 
 Not-null constraints:
-    "pp1_f1_not_null" NOT NULL "f1" (inherited)
+    "pp1_f1_not_null" NOT NULL "f1": inherited
 Inherits: pp1,
           cc1
 
@@ -2112,8 +2112,8 @@ create table inh_child () inherits (inh_parent1, inh_parent2);
  a      | integer |           | not null |         | plain   |              | 
  b      | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "nn" NOT NULL "a" (inherited)
-    "inh_child_b_not_null" NOT NULL "b" (inherited)
+    "nn" NOT NULL "a": inherited
+    "inh_child_b_not_null" NOT NULL "b": inherited
 Inherits: inh_parent1,
           inh_parent2
 
@@ -2147,9 +2147,9 @@ select conrelid::regclass, conname, contype, conkey,
  d      | integer |           | not null |         | plain   |              | 
  e      | integer |           |          |         | plain   |              | 
 Not-null constraints:
-    "inh_child_a_not_null" NOT NULL "a" (inherited)
-    "inh_child_b_not_null" NOT NULL "b" (inherited)
-    "inh_child_d_not_null" NOT NULL "d" (inherited)
+    "inh_child_a_not_null" NOT NULL "a": inherited
+    "inh_child_b_not_null" NOT NULL "b": inherited
+    "inh_child_d_not_null" NOT NULL "d": inherited
 Inherits: inh_parent1,
           inh_parent2
 
@@ -2188,7 +2188,7 @@ Inherits: inh_nn_parent
 --------+---------+-----------+----------+---------+---------+--------------+-------------
  a      | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "inh_nn_parent_a_not_null" NOT NULL "a" NO INHERIT
+    "inh_nn_parent_a_not_null" NOT NULL "a": uninheritable
 Child tables: inh_nn_child,
               inh_nn_child2
 
@@ -2223,7 +2223,7 @@ Child tables: inh_child1
 --------+---------+-----------+----------+---------+---------+--------------+-------------
  f1     | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "inh_child1_f1_not_null" NOT NULL "f1" (local, inherited)
+    "inh_child1_f1_not_null" NOT NULL "f1": local inherited
 Inherits: inh_parent
 Child tables: inh_child2
 
@@ -2233,7 +2233,7 @@ Child tables: inh_child2
 --------+---------+-----------+----------+---------+---------+--------------+-------------
  f1     | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "inh_child2_f1_not_null" NOT NULL "f1" (local, inherited)
+    "inh_child2_f1_not_null" NOT NULL "f1": local inherited
 Inherits: inh_child1
 
 select conrelid::regclass, conname, contype, coninhcount, conislocal
@@ -2277,7 +2277,7 @@ Child tables: inh_child2,
 --------+---------+-----------+----------+---------+---------+--------------+-------------
  f1     | integer |           | not null |         | plain   |              | 
 Not-null constraints:
-    "inh_child2_f1_not_null" NOT NULL "f1" (local, inherited)
+    "inh_child2_f1_not_null" NOT NULL "f1": local inherited
 Inherits: inh_child1
 
 select conrelid::regclass, conname, contype, coninhcount, conislocal
-- 
2.39.3



  [text/x-patch] 0002-Add-tests-for-d-not-null-constraints.patch (6.0K, ../../[email protected]/3-0002-Add-tests-for-d-not-null-constraints.patch)
  download | inline diff:
From 606eb7074f18d330833d044d262eeadfe7f6ac12 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email protected]>
Date: Mon, 28 Aug 2023 15:27:53 +0900
Subject: [PATCH 2/2] Add tests for \d+ not-null constraints

Separated for improved readability.
---
 src/test/regress/expected/inherit.out | 35 ++++++++++++++++++++++++---
 src/test/regress/sql/inherit.sql      |  7 +++---
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index c80e398f71..99f260b1a5 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2155,10 +2155,14 @@ Inherits: inh_parent1,
 
 drop table inh_parent1, inh_parent2, inh_child;
 -- NOT NULL NO INHERIT
-create table inh_nn_parent(a int);
+create table inh_nn_parent(a int, b int);
 create table inh_nn_child() inherits (inh_nn_parent);
 alter table inh_nn_parent add not null a no inherit;
-create table inh_nn_child2() inherits (inh_nn_parent);
+create table inh_nn_child2(c int not null) inherits (inh_nn_parent);
+create table inh_nn_child3(a int not null, b int not null no inherit, c int not null no inherit) inherits (inh_nn_child2);
+NOTICE:  merging column "a" with inherited definition
+NOTICE:  merging column "b" with inherited definition
+NOTICE:  merging column "c" with inherited definition
 select conrelid::regclass, conname, contype, conkey,
  (select attname from pg_attribute where attrelid = conrelid and attnum = conkey[1]),
  coninhcount, conislocal, connoinherit
@@ -2167,32 +2171,55 @@ select conrelid::regclass, conname, contype, conkey,
  order by 2, 1;
    conrelid    |         conname          | contype | conkey | attname | coninhcount | conislocal | connoinherit 
 ---------------+--------------------------+---------+--------+---------+-------------+------------+--------------
+ inh_nn_child2 | inh_nn_child2_c_not_null | n       | {3}    | c       |           0 | t          | f
+ inh_nn_child3 | inh_nn_child3_a_not_null | n       | {1}    | a       |           0 | t          | f
+ inh_nn_child3 | inh_nn_child3_b_not_null | n       | {2}    | b       |           0 | t          | t
+ inh_nn_child3 | inh_nn_child3_c_not_null | n       | {3}    | c       |           1 | t          | t
  inh_nn_parent | inh_nn_parent_a_not_null | n       | {1}    | a       |           0 | t          | t
-(1 row)
+(5 rows)
 
 \d+ inh_nn*
                                Table "public.inh_nn_child"
  Column |  Type   | Collation | Nullable | Default | Storage | Stats target | Description 
 --------+---------+-----------+----------+---------+---------+--------------+-------------
  a      | integer |           |          |         | plain   |              | 
+ b      | integer |           |          |         | plain   |              | 
 Inherits: inh_nn_parent
 
                                Table "public.inh_nn_child2"
  Column |  Type   | Collation | Nullable | Default | Storage | Stats target | Description 
 --------+---------+-----------+----------+---------+---------+--------------+-------------
  a      | integer |           |          |         | plain   |              | 
+ b      | integer |           |          |         | plain   |              | 
+ c      | integer |           | not null |         | plain   |              | 
+Not-null constraints:
+    "inh_nn_child2_c_not_null" NOT NULL "c"
 Inherits: inh_nn_parent
+Child tables: inh_nn_child3
+
+                               Table "public.inh_nn_child3"
+ Column |  Type   | Collation | Nullable | Default | Storage | Stats target | Description 
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ a      | integer |           | not null |         | plain   |              | 
+ b      | integer |           | not null |         | plain   |              | 
+ c      | integer |           | not null |         | plain   |              | 
+Not-null constraints:
+    "inh_nn_child3_a_not_null" NOT NULL "a"
+    "inh_nn_child3_b_not_null" NOT NULL "b": uninheritable
+    "inh_nn_child3_c_not_null" NOT NULL "c": local inherited uninheritable
+Inherits: inh_nn_child2
 
                                Table "public.inh_nn_parent"
  Column |  Type   | Collation | Nullable | Default | Storage | Stats target | Description 
 --------+---------+-----------+----------+---------+---------+--------------+-------------
  a      | integer |           | not null |         | plain   |              | 
+ b      | integer |           |          |         | plain   |              | 
 Not-null constraints:
     "inh_nn_parent_a_not_null" NOT NULL "a": uninheritable
 Child tables: inh_nn_child,
               inh_nn_child2
 
-drop table inh_nn_parent, inh_nn_child, inh_nn_child2;
+drop table inh_nn_parent, inh_nn_child, inh_nn_child2, inh_nn_child3;
 --
 -- test inherit/deinherit
 --
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index d8fae92a53..82e9118745 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -777,10 +777,11 @@ select conrelid::regclass, conname, contype, conkey,
 drop table inh_parent1, inh_parent2, inh_child;
 
 -- NOT NULL NO INHERIT
-create table inh_nn_parent(a int);
+create table inh_nn_parent(a int, b int);
 create table inh_nn_child() inherits (inh_nn_parent);
 alter table inh_nn_parent add not null a no inherit;
-create table inh_nn_child2() inherits (inh_nn_parent);
+create table inh_nn_child2(c int not null) inherits (inh_nn_parent);
+create table inh_nn_child3(a int not null, b int not null no inherit, c int not null no inherit) inherits (inh_nn_child2);
 select conrelid::regclass, conname, contype, conkey,
  (select attname from pg_attribute where attrelid = conrelid and attnum = conkey[1]),
  coninhcount, conislocal, connoinherit
@@ -788,7 +789,7 @@ select conrelid::regclass, conname, contype, conkey,
  conrelid::regclass::text like 'inh\_nn\_%'
  order by 2, 1;
 \d+ inh_nn*
-drop table inh_nn_parent, inh_nn_child, inh_nn_child2;
+drop table inh_nn_parent, inh_nn_child, inh_nn_child2, inh_nn_child3;
 
 --
 -- test inherit/deinherit
-- 
2.39.3



^ permalink  raw  reply  [nested|flat] 21+ messages in thread

end of thread, other threads:[~2023-08-28 07:16 UTC | newest]

Thread overview: 21+ 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]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2023-08-28 07:16 Strange presentaion related to inheritance in \d+ Kyotaro Horiguchi <[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