public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v52 5/7] Doc part of shared-memory based stats collector.
5+ messages / 3 participants
[nested] [flat]
* [PATCH v52 5/7] Doc part of shared-memory based stats collector.
@ 2020-03-19 06:11 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Kyotaro Horiguchi @ 2020-03-19 06:11 UTC (permalink / raw)
---
doc/src/sgml/catalogs.sgml | 6 +-
doc/src/sgml/config.sgml | 27 +++---
doc/src/sgml/high-availability.sgml | 13 +--
doc/src/sgml/monitoring.sgml | 127 +++++++++++++---------------
doc/src/sgml/ref/pg_dump.sgml | 9 +-
src/backend/postmaster/postmaster.c | 2 -
6 files changed, 86 insertions(+), 98 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b1de6d0674..0ef684d4d0 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9261,9 +9261,9 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
<para>
<xref linkend="view-table"/> lists the system views described here.
More detailed documentation of each view follows below.
- There are some additional views that provide access to the results of
- the statistics collector; they are described in <xref
- linkend="monitoring-stats-views-table"/>.
+ There are some additional views that provide access to the activity
+ statistics; they are described in
+ <xref linkend="monitoring-stats-views-table"/>.
</para>
<para>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 529876895b..99a8664026 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7375,11 +7375,11 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
<title>Run-time Statistics</title>
<sect2 id="runtime-config-statistics-collector">
- <title>Query and Index Statistics Collector</title>
+ <title>Query and Index Activity Statistics</title>
<para>
- These parameters control server-wide statistics collection features.
- When statistics collection is enabled, the data that is produced can be
+ These parameters control server-wide activity statistics features.
+ When activity statistics is enabled, the data that is produced can be
accessed via the <structname>pg_stat</structname> and
<structname>pg_statio</structname> family of system views.
Refer to <xref linkend="monitoring"/> for more information.
@@ -7395,14 +7395,13 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</term>
<listitem>
<para>
- Enables the collection of information on the currently
- executing command of each session, along with the time when
- that command began execution. This parameter is on by
- default. Note that even when enabled, this information is not
- visible to all users, only to superusers and the user owning
- the session being reported on, so it should not represent a
- security risk.
- Only superusers can change this setting.
+ Enables activity tracking on the currently executing command of
+ each session, along with the time when that command began
+ execution. This parameter is on by default. Note that even when
+ enabled, this information is not visible to all users, only to
+ superusers and the user owning the session being reported on, so it
+ should not represent a security risk. Only superusers can change this
+ setting.
</para>
</listitem>
</varlistentry>
@@ -7433,9 +7432,9 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</term>
<listitem>
<para>
- Enables collection of statistics on database activity.
+ Enables tracking of database activity.
This parameter is on by default, because the autovacuum
- daemon needs the collected information.
+ daemon needs the activity information.
Only superusers can change this setting.
</para>
</listitem>
@@ -8554,7 +8553,7 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
<listitem>
<para>
Specifies the fraction of the total number of heap tuples counted in
- the previous statistics collection that can be inserted without
+ the previously collected statistics that can be inserted without
incurring an index scan at the <command>VACUUM</command> cleanup stage.
This setting currently applies to B-tree indexes only.
</para>
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index f49f5c0108..45095857eb 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -2217,12 +2217,13 @@ HINT: You can then restart the server after making the necessary configuration
</para>
<para>
- The statistics collector is active during recovery. All scans, reads, blocks,
- index usage, etc., will be recorded normally on the standby. Replayed
- actions will not duplicate their effects on primary, so replaying an
- insert will not increment the Inserts column of pg_stat_user_tables.
- The stats file is deleted at the start of recovery, so stats from primary
- and standby will differ; this is considered a feature, not a bug.
+ The activity statistics is collected during recovery. All scans, reads,
+ blocks, index usage, etc., will be recorded normally on the
+ standby. Replayed actions will not duplicate their effects on primary, so
+ replaying an insert will not increment the Inserts column of
+ pg_stat_user_tables. The activity statistics is reset at the start of
+ recovery, so stats from primary and standby will differ; this is
+ considered a feature, not a bug.
</para>
<para>
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 3335d71eba..6217ed9792 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -22,7 +22,7 @@
<para>
Several tools are available for monitoring database activity and
analyzing performance. Most of this chapter is devoted to describing
- <productname>PostgreSQL</productname>'s statistics collector,
+ <productname>PostgreSQL</productname>'s activity statistics,
but one should not neglect regular Unix monitoring programs such as
<command>ps</command>, <command>top</command>, <command>iostat</command>, and <command>vmstat</command>.
Also, once one has identified a
@@ -53,7 +53,6 @@ postgres 15554 0.0 0.0 57536 1184 ? Ss 18:02 0:00 postgres: back
postgres 15555 0.0 0.0 57536 916 ? Ss 18:02 0:00 postgres: checkpointer
postgres 15556 0.0 0.0 57536 916 ? Ss 18:02 0:00 postgres: walwriter
postgres 15557 0.0 0.0 58504 2244 ? Ss 18:02 0:00 postgres: autovacuum launcher
-postgres 15558 0.0 0.0 17512 1068 ? Ss 18:02 0:00 postgres: stats collector
postgres 15582 0.0 0.0 58772 3080 ? Ss 18:04 0:00 postgres: joe runbug 127.0.0.1 idle
postgres 15606 0.0 0.0 58772 3052 ? Ss 18:07 0:00 postgres: tgl regression [local] SELECT waiting
postgres 15610 0.0 0.0 58772 3056 ? Ss 18:07 0:00 postgres: tgl regression [local] idle in transaction
@@ -65,9 +64,8 @@ postgres 15610 0.0 0.0 58772 3056 ? Ss 18:07 0:00 postgres: tgl
primary server process. The command arguments
shown for it are the same ones used when it was launched. The next five
processes are background worker processes automatically launched by the
- primary process. (The <quote>stats collector</quote> process will not be present
- if you have set the system not to start the statistics collector; likewise
- the <quote>autovacuum launcher</quote> process can be disabled.)
+ primary process. (The <quote>autovacuum launcher</quote> process will not
+ be present if you have set the system not to start it.)
Each of the remaining
processes is a server process handling one client connection. Each such
process sets its command line display in the form
@@ -130,20 +128,21 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
</sect1>
<sect1 id="monitoring-stats">
- <title>The Statistics Collector</title>
+ <title>The Activity Statistics</title>
<indexterm zone="monitoring-stats">
<primary>statistics</primary>
</indexterm>
<para>
- <productname>PostgreSQL</productname>'s <firstterm>statistics collector</firstterm>
- is a subsystem that supports collection and reporting of information about
- server activity. Presently, the collector can count accesses to tables
- and indexes in both disk-block and individual-row terms. It also tracks
- the total number of rows in each table, and information about vacuum and
- analyze actions for each table. It can also count calls to user-defined
- functions and the total time spent in each one.
+ <productname>PostgreSQL</productname>'s <firstterm>activity
+ statistics</firstterm> is a subsystem that supports tracking and reporting
+ of information about server activity. Presently, the activity statistics
+ tracks the count of accesses to tables and indexes in both disk-block and
+ individual-row terms. It also tracks the total number of rows in each
+ table, and information about vacuum and analyze actions for each table. It
+ can also track calls to user-defined functions and the total time spent in
+ each one.
</para>
<para>
@@ -151,15 +150,15 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
information about exactly what is going on in the system right now, such as
the exact command currently being executed by other server processes, and
which other connections exist in the system. This facility is independent
- of the collector process.
+ of the activity statistics.
</para>
<sect2 id="monitoring-stats-setup">
- <title>Statistics Collection Configuration</title>
+ <title>Activity Statistics Configuration</title>
<para>
- Since collection of statistics adds some overhead to query execution,
- the system can be configured to collect or not collect information.
+ Since tracking for the activity statistics adds some overhead to query
+ execution, the system can be configured to track or not track activity.
This is controlled by configuration parameters that are normally set in
<filename>postgresql.conf</filename>. (See <xref linkend="runtime-config"/> for
details about setting configuration parameters.)
@@ -172,7 +171,7 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
<para>
The parameter <xref linkend="guc-track-counts"/> controls whether
- statistics are collected about table and index accesses.
+ to track activity about table and index accesses.
</para>
<para>
@@ -201,18 +200,11 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
</para>
<para>
- The statistics collector transmits the collected information to other
- <productname>PostgreSQL</productname> processes through temporary files.
- These files are stored in the directory named by the
- <xref linkend="guc-stats-temp-directory"/> parameter,
- <filename>pg_stat_tmp</filename> by default.
- For better performance, <varname>stats_temp_directory</varname> can be
- pointed at a RAM-based file system, decreasing physical I/O requirements.
- When the server shuts down cleanly, a permanent copy of the statistics
- data is stored in the <filename>pg_stat</filename> subdirectory, so that
- statistics can be retained across server restarts. When recovery is
- performed at server start (e.g., after immediate shutdown, server crash,
- and point-in-time recovery), all statistics counters are reset.
+ down cleanly, a permanent copy of the statistics data is stored in
+ the <filename>pg_stat</filename> subdirectory, so that statistics can be
+ retained across server restarts. When recovery is performed at server
+ start (e.g. after immediate shutdown, server crash, and point-in-time
+ recovery), all statistics counters are reset.
</para>
</sect2>
@@ -225,48 +217,46 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
linkend="monitoring-stats-dynamic-views-table"/>, are available to show
the current state of the system. There are also several other
views, listed in <xref
- linkend="monitoring-stats-views-table"/>, available to show the results
- of statistics collection. Alternatively, one can
- build custom views using the underlying statistics functions, as discussed
- in <xref linkend="monitoring-stats-functions"/>.
+ linkend="monitoring-stats-views-table"/>, available to show the activity
+ statistics. Alternatively, one can build custom views using the underlying
+ statistics functions, as discussed in
+ <xref linkend="monitoring-stats-functions"/>.
</para>
<para>
- When using the statistics to monitor collected data, it is important
- to realize that the information does not update instantaneously.
- Each individual server process transmits new statistical counts to
- the collector just before going idle; so a query or transaction still in
- progress does not affect the displayed totals. Also, the collector itself
- emits a new report at most once per <varname>PGSTAT_STAT_INTERVAL</varname>
- milliseconds (500 ms unless altered while building the server). So the
- displayed information lags behind actual activity. However, current-query
- information collected by <varname>track_activities</varname> is
- always up-to-date.
+ When using the activity statistics, it is important to realize that the
+ information does not update instantaneously. Each individual server writes
+ out new statistical counts just before going idle, not frequent than once
+ per <varname>PGSTAT_STAT_INTERVAL</varname> milliseconds (1 second unless
+ altered while building the server); so a query or transaction still in
+ progress does not affect the displayed totals. However, current-query
+ information tracked by <varname>track_activities</varname> is always
+ up-to-date.
</para>
<para>
Another important point is that when a server process is asked to display
- any of these statistics, it first fetches the most recent report emitted by
- the collector process and then continues to use this snapshot for all
- statistical views and functions until the end of its current transaction.
- So the statistics will show static information as long as you continue the
- current transaction. Similarly, information about the current queries of
- all sessions is collected when any such information is first requested
- within a transaction, and the same information will be displayed throughout
- the transaction.
- This is a feature, not a bug, because it allows you to perform several
- queries on the statistics and correlate the results without worrying that
- the numbers are changing underneath you. But if you want to see new
- results with each query, be sure to do the queries outside any transaction
- block. Alternatively, you can invoke
+ any of these statistics, it first reads the current statistics and then
+ continues to use this snapshot for all statistical views and functions
+ until the end of its current transaction. So the statistics will show
+ static information as long as you continue the current transaction.
+ Similarly, information about the current queries of all sessions is tracked
+ when any such information is first requested within a transaction, and the
+ same information will be displayed throughout the transaction. This is a
+ feature, not a bug, because it allows you to perform several queries on the
+ statistics and correlate the results without worrying that the numbers are
+ changing underneath you. But if you want to see new results with each
+ query, be sure to do the queries outside any transaction block.
+ Alternatively, you can invoke
<function>pg_stat_clear_snapshot</function>(), which will discard the
current transaction's statistics snapshot (if any). The next use of
statistical information will cause a new snapshot to be fetched.
</para>
-
+
<para>
- A transaction can also see its own statistics (as yet untransmitted to the
- collector) in the views <structname>pg_stat_xact_all_tables</structname>,
+ A transaction can also see its own statistics (as yet unwritten to the
+ server-wide activity statistics) in the
+ views <structname>pg_stat_xact_all_tables</structname>,
<structname>pg_stat_xact_sys_tables</structname>,
<structname>pg_stat_xact_user_tables</structname>, and
<structname>pg_stat_xact_user_functions</structname>. These numbers do not act as
@@ -648,7 +638,7 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
kernel's I/O cache, and might therefore still be fetched without
requiring a physical read. Users interested in obtaining more
detailed information on <productname>PostgreSQL</productname> I/O behavior are
- advised to use the <productname>PostgreSQL</productname> statistics collector
+ advised to use the <productname>PostgreSQL</productname> activity statistics
in combination with operating system utilities that allow insight
into the kernel's handling of I/O.
</para>
@@ -1085,10 +1075,6 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
<entry><literal>LogicalLauncherMain</literal></entry>
<entry>Waiting in main loop of logical replication launcher process.</entry>
</row>
- <row>
- <entry><literal>PgStatMain</literal></entry>
- <entry>Waiting in main loop of statistics collector process.</entry>
- </row>
<row>
<entry><literal>RecoveryWalStream</literal></entry>
<entry>Waiting in main loop of startup process for WAL to arrive, during
@@ -1843,6 +1829,10 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
</thead>
<tbody>
+ <row>
+ <entry><literal>ActivityStatistics</literal></entry>
+ <entry>Waiting to write out activity statistics to shared memory.</entry>
+ </row>
<row>
<entry><literal>AddinShmemInit</literal></entry>
<entry>Waiting to manage an extension's space allocation in shared
@@ -6123,9 +6113,10 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
<entry><literal>performing final cleanup</literal></entry>
<entry>
<command>VACUUM</command> is performing final cleanup. During this phase,
- <command>VACUUM</command> will vacuum the free space map, update statistics
- in <literal>pg_class</literal>, and report statistics to the statistics
- collector. When this phase is completed, <command>VACUUM</command> will end.
+ <command>VACUUM</command> will vacuum the free space map, update
+ statistics in <literal>pg_class</literal>, and system-wide activity
+ statistics. When this phase is completed, <command>VACUUM</command>
+ will end.
</entry>
</row>
</tbody>
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index bcbb7a25fb..1fa59a2fdf 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -1280,11 +1280,10 @@ PostgreSQL documentation
</para>
<para>
- The database activity of <application>pg_dump</application> is
- normally collected by the statistics collector. If this is
- undesirable, you can set parameter <varname>track_counts</varname>
- to false via <envar>PGOPTIONS</envar> or the <literal>ALTER
- USER</literal> command.
+ The database activity of <application>pg_dump</application> is normally
+ collected. If this is undesirable, you can set
+ parameter <varname>track_counts</varname> to false
+ via <envar>PGOPTIONS</envar> or the <literal>ALTER USER</literal> command.
</para>
</refsect1>
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 0abc41c33c..d74b5eed82 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -6328,8 +6328,6 @@ restore_backend_variables(BackendParameters *param, Port *port)
if (postmaster_alive_fds[1] >= 0)
ReserveExternalFD();
#endif
- if (pgStatSock != PGINVALID_SOCKET)
- ReserveExternalFD();
}
--
2.27.0
----Next_Part(Wed_Mar_10_17_51_37_2021_192)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v52-0006-Remove-the-GUC-stats_temp_directory.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Making aggregate deserialization (and WAL receive) functions slightly faster
@ 2023-02-12 05:38 David Rowley <[email protected]>
2023-02-12 06:39 ` Re: Making aggregate deserialization (and WAL receive) functions slightly faster Tom Lane <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: David Rowley @ 2023-02-12 05:38 UTC (permalink / raw)
To: PostgreSQL Developers <[email protected]>
While working on 16fd03e95, I noticed that in each aggregate
deserialization function, in order to "receive" the bytea value that
is the serialized aggregate state, appendBinaryStringInfo is used to
append the bytes of the bytea value onto a temporary StringInfoData.
Using appendBinaryStringInfo seems a bit wasteful here. We could
really just fake up a StringInfoData and point directly to the bytes
of the bytea value.
The best way I could think of to do this was to invent
initStringInfoFromString() which initialises a StringInfoData and has
the ->data field point directly at the specified buffer. This will
mean that it would be unsafe to do any appendStringInfo* operations on
the resulting StringInfoData as enlargeStringInfo would try to
repalloc the data buffer, which might not even point to a palloc'd
string. I thought it might be fine just to mention that in the
comments for the function, but we could probably do a bit better and
set maxlen to something like -1 and Assert() we never see -1 in the
various append functions. I wasn't sure it was worth it, so didn't do
that.
I had a look around for other places that might be following the same
pattern. I only found range_recv() and XLogWalRcvProcessMsg(). I
didn't adjust the range_recv() one as I couldn't see how to do that
without casting away a const. I did adjust the XLogWalRcvProcessMsg()
one and got rid of a global variable in the process.
I've attached the benchmark results I got after testing how the
modification changed the performance of string_agg_deserialize().
I was hoping this would have a slightly more impressive performance
impact, especially for string_agg() and array_agg() as the aggregate
states of those can be large. However, in the test I ran, there's
only a very slight performance gain. I may just not have found the
best case, however.
David
#!/bin/bash
psql -c "drop table if exists t;" postgres
psql -c "create table t (a text not null);" postgres
psql -c "insert into t select x::text from generate_series(1,1000000)x;" postgres
psql -c "vacuum freeze t;" postgres
psql -c "select pg_prewarm('t');" postgres
for q in "select length(string_agg(a,',')) from t;"
do
for i in {0..32}
do
echo Testing with $i parallel workers: $q
psql -c "alter table t set (parallel_workers = $i);" postgres
echo $q > bench.sql
pgbench -f bench.sql -n -T 10 postgres | grep latency
done
done
From 75d97a066ac81f5a50b1b7618ad9e240f5497860 Mon Sep 17 00:00:00 2001
From: David Rowley <[email protected]>
Date: Sat, 11 Feb 2023 10:05:32 +1300
Subject: [PATCH v1 1/2] Optimize various aggregate deserialization functions
The serialized representation of an internal aggregate state is a bytea
value. In each deserial function, in order to "receive" the bytea value
we appended it onto a short-lived StringInfoData using
appendBinaryStringInfo. This was a little wasteful as it meant having to
palloc memory, copy a (possibly long) series of bytes then later pfree
that memory. Instead of doing to this extra trouble, we can just fake up
a StringInfoData and point the data directly at the bytea's payload. This
should help increase the performance of internal aggregate
deserialization.
To make this work, we introduce a function named initStringInfoFromString.
This just makes the given StringInfoData point directly to the input
buffer and sets the length to the given length.
---
src/backend/utils/adt/array_userfuncs.c | 20 +++++--------
src/backend/utils/adt/numeric.c | 40 ++++++++++---------------
src/backend/utils/adt/varlena.c | 10 +++----
src/common/stringinfo.c | 20 +++++++++++++
src/include/lib/stringinfo.h | 13 ++++++++
5 files changed, 61 insertions(+), 42 deletions(-)
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index c8a8d33ca3..78fd0d2340 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -723,12 +723,11 @@ array_agg_deserialize(PG_FUNCTION_ARGS)
sstate = PG_GETARG_BYTEA_PP(0);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
/* element_type */
element_type = pq_getmsgint(&buf, 4);
@@ -825,7 +824,6 @@ array_agg_deserialize(PG_FUNCTION_ARGS)
}
pq_getmsgend(&buf);
- pfree(buf.data);
PG_RETURN_POINTER(result);
}
@@ -1134,12 +1132,11 @@ array_agg_array_deserialize(PG_FUNCTION_ARGS)
sstate = PG_GETARG_BYTEA_PP(0);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
/* element_type */
element_type = pq_getmsgint(&buf, 4);
@@ -1197,7 +1194,6 @@ array_agg_array_deserialize(PG_FUNCTION_ARGS)
memcpy(result->lbs, temp, sizeof(result->lbs));
pq_getmsgend(&buf);
- pfree(buf.data);
PG_RETURN_POINTER(result);
}
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index a83feea396..dec29d2b9b 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -5179,12 +5179,11 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
result = makeNumericAggStateCurrentContext(false);
@@ -5211,7 +5210,6 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS)
result->nInfcount = pq_getmsgint64(&buf);
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
@@ -5295,12 +5293,11 @@ numeric_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
result = makeNumericAggStateCurrentContext(false);
@@ -5331,7 +5328,6 @@ numeric_deserialize(PG_FUNCTION_ARGS)
result->nInfcount = pq_getmsgint64(&buf);
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
@@ -5666,12 +5662,11 @@ numeric_poly_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
result = makePolyNumAggStateCurrentContext(false);
@@ -5695,7 +5690,6 @@ numeric_poly_deserialize(PG_FUNCTION_ARGS)
#endif
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
@@ -5857,12 +5851,11 @@ int8_avg_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
result = makePolyNumAggStateCurrentContext(false);
@@ -5878,7 +5871,6 @@ int8_avg_deserialize(PG_FUNCTION_ARGS)
#endif
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 170b3a3820..be43bf314b 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5532,12 +5532,11 @@ string_agg_deserialize(PG_FUNCTION_ARGS)
sstate = PG_GETARG_BYTEA_PP(0);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
result = makeStringAggState(fcinfo);
@@ -5550,7 +5549,6 @@ string_agg_deserialize(PG_FUNCTION_ARGS)
appendBinaryStringInfo(result, data, datalen);
pq_getmsgend(&buf);
- pfree(buf.data);
PG_RETURN_POINTER(result);
}
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 05b22b5c53..2e0bc28527 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -65,6 +65,26 @@ initStringInfo(StringInfo str)
resetStringInfo(str);
}
+/*
+ * initStringInfoFromString
+ *
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * and sets the string to directly point to the contents of 'str' with the
+ * given 'len'.
+ *
+ * Caution: this is primarily used for very short-lived StringInfos. The
+ * resulting StringInfo cannot be appended to unless 'buf' points directly to
+ * a memory address as returned by palloc. Generally, you should be using
+ * appendBinaryStringInfo() instead of this function.
+ */
+void
+initStringInfoFromString(StringInfo str, char *buf, int len)
+{
+ str->data = buf;
+ str->len = str->maxlen = len;
+ str->cursor = 0;
+}
+
/*
* resetStringInfo
*
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 36a416f8e0..4e4d73658d 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -79,6 +79,19 @@ extern StringInfo makeStringInfo(void);
*/
extern void initStringInfo(StringInfo str);
+/*------------------------
+ * initStringInfoFromString
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * and sets the string to directly point to the contents of 'str' with the
+ * given 'len'.
+ *
+ * Caution: this is primarily used for very short-lived StringInfos. The
+ * resulting StringInfo cannot be appended to unless 'buf' points directly to
+ * a memory address as returned by palloc. Generally, you should be using
+ * appendBinaryStringInfo() instead of this function.
+ */
+extern void initStringInfoFromString(StringInfo str, char *buf, int len);
+
/*------------------------
* resetStringInfo
* Clears the current content of the StringInfo, if any. The
--
2.37.2
From 0b31533674adb90644298c638cc7dfe066ab5bd6 Mon Sep 17 00:00:00 2001
From: David Rowley <[email protected]>
Date: Sun, 12 Feb 2023 16:45:15 +1300
Subject: [PATCH v1 2/2] Add more usages of initStringInfoFromString
This slightly reduces some memcpy work in walreceiver.c
---
src/backend/replication/walreceiver.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index f6446da2d6..366b91f923 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -132,7 +132,6 @@ typedef enum WalRcvWakeupReason
static TimestampTz wakeup[NUM_WALRCV_WAKEUPS];
static StringInfoData reply_message;
-static StringInfoData incoming_message;
/* Prototypes for private functions */
static void WalRcvFetchTimeLineHistoryFiles(TimeLineID first, TimeLineID last);
@@ -422,10 +421,12 @@ WalReceiverMain(void)
LSN_FORMAT_ARGS(startpoint), startpointTLI)));
first_stream = false;
- /* Initialize LogstreamResult and buffers for processing messages */
+ /*
+ * Initialize LogstreamResult and reply buffer for processing
+ * messages
+ */
LogstreamResult.Write = LogstreamResult.Flush = GetXLogReplayRecPtr(NULL);
initStringInfo(&reply_message);
- initStringInfo(&incoming_message);
/* Initialize nap wakeup times. */
now = GetCurrentTimestamp();
@@ -843,19 +844,19 @@ XLogWalRcvProcessMsg(unsigned char type, char *buf, Size len, TimeLineID tli)
TimestampTz sendTime;
bool replyRequested;
- resetStringInfo(&incoming_message);
-
switch (type)
{
case 'w': /* WAL records */
{
- /* copy message to StringInfo */
+ StringInfoData incoming_message;
+
+ /* set the message in the incoming_message StringInfo */
hdrlen = sizeof(int64) + sizeof(int64) + sizeof(int64);
if (len < hdrlen)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg_internal("invalid WAL message received from primary")));
- appendBinaryStringInfo(&incoming_message, buf, hdrlen);
+ initStringInfoFromString(&incoming_message, buf, hdrlen);
/* read the fields */
dataStart = pq_getmsgint64(&incoming_message);
@@ -870,13 +871,15 @@ XLogWalRcvProcessMsg(unsigned char type, char *buf, Size len, TimeLineID tli)
}
case 'k': /* Keepalive */
{
- /* copy message to StringInfo */
+ StringInfoData incoming_message;
+
+ /* set the message in the incoming_message StringInfo */
hdrlen = sizeof(int64) + sizeof(int64) + sizeof(char);
if (len != hdrlen)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg_internal("invalid keepalive message received from primary")));
- appendBinaryStringInfo(&incoming_message, buf, hdrlen);
+ initStringInfoFromString(&incoming_message, buf, hdrlen);
/* read the fields */
walEnd = pq_getmsgint64(&incoming_message);
--
2.37.2
Attachments:
[text/plain] string_agg_test.sh.txt (550B, ../../CAApHDvr=e-YOigriSHHm324a40HPqcUhSp6pWWgjz5WwegR=cQ@mail.gmail.com/2-string_agg_test.sh.txt)
download | inline:
#!/bin/bash
psql -c "drop table if exists t;" postgres
psql -c "create table t (a text not null);" postgres
psql -c "insert into t select x::text from generate_series(1,1000000)x;" postgres
psql -c "vacuum freeze t;" postgres
psql -c "select pg_prewarm('t');" postgres
for q in "select length(string_agg(a,',')) from t;"
do
for i in {0..32}
do
echo Testing with $i parallel workers: $q
psql -c "alter table t set (parallel_workers = $i);" postgres
echo $q > bench.sql
pgbench -f bench.sql -n -T 10 postgres | grep latency
done
done
[image/png] string_agg_single_group_bench.png (76.4K, ../../CAApHDvr=e-YOigriSHHm324a40HPqcUhSp6pWWgjz5WwegR=cQ@mail.gmail.com/3-string_agg_single_group_bench.png)
download | view image
[text/plain] v1-0001-Optimize-various-aggregate-deserialization-functi.patch (9.1K, ../../CAApHDvr=e-YOigriSHHm324a40HPqcUhSp6pWWgjz5WwegR=cQ@mail.gmail.com/4-v1-0001-Optimize-various-aggregate-deserialization-functi.patch)
download | inline diff:
From 75d97a066ac81f5a50b1b7618ad9e240f5497860 Mon Sep 17 00:00:00 2001
From: David Rowley <[email protected]>
Date: Sat, 11 Feb 2023 10:05:32 +1300
Subject: [PATCH v1 1/2] Optimize various aggregate deserialization functions
The serialized representation of an internal aggregate state is a bytea
value. In each deserial function, in order to "receive" the bytea value
we appended it onto a short-lived StringInfoData using
appendBinaryStringInfo. This was a little wasteful as it meant having to
palloc memory, copy a (possibly long) series of bytes then later pfree
that memory. Instead of doing to this extra trouble, we can just fake up
a StringInfoData and point the data directly at the bytea's payload. This
should help increase the performance of internal aggregate
deserialization.
To make this work, we introduce a function named initStringInfoFromString.
This just makes the given StringInfoData point directly to the input
buffer and sets the length to the given length.
---
src/backend/utils/adt/array_userfuncs.c | 20 +++++--------
src/backend/utils/adt/numeric.c | 40 ++++++++++---------------
src/backend/utils/adt/varlena.c | 10 +++----
src/common/stringinfo.c | 20 +++++++++++++
src/include/lib/stringinfo.h | 13 ++++++++
5 files changed, 61 insertions(+), 42 deletions(-)
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index c8a8d33ca3..78fd0d2340 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -723,12 +723,11 @@ array_agg_deserialize(PG_FUNCTION_ARGS)
sstate = PG_GETARG_BYTEA_PP(0);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
/* element_type */
element_type = pq_getmsgint(&buf, 4);
@@ -825,7 +824,6 @@ array_agg_deserialize(PG_FUNCTION_ARGS)
}
pq_getmsgend(&buf);
- pfree(buf.data);
PG_RETURN_POINTER(result);
}
@@ -1134,12 +1132,11 @@ array_agg_array_deserialize(PG_FUNCTION_ARGS)
sstate = PG_GETARG_BYTEA_PP(0);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
/* element_type */
element_type = pq_getmsgint(&buf, 4);
@@ -1197,7 +1194,6 @@ array_agg_array_deserialize(PG_FUNCTION_ARGS)
memcpy(result->lbs, temp, sizeof(result->lbs));
pq_getmsgend(&buf);
- pfree(buf.data);
PG_RETURN_POINTER(result);
}
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index a83feea396..dec29d2b9b 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -5179,12 +5179,11 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
result = makeNumericAggStateCurrentContext(false);
@@ -5211,7 +5210,6 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS)
result->nInfcount = pq_getmsgint64(&buf);
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
@@ -5295,12 +5293,11 @@ numeric_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
result = makeNumericAggStateCurrentContext(false);
@@ -5331,7 +5328,6 @@ numeric_deserialize(PG_FUNCTION_ARGS)
result->nInfcount = pq_getmsgint64(&buf);
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
@@ -5666,12 +5662,11 @@ numeric_poly_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
result = makePolyNumAggStateCurrentContext(false);
@@ -5695,7 +5690,6 @@ numeric_poly_deserialize(PG_FUNCTION_ARGS)
#endif
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
@@ -5857,12 +5851,11 @@ int8_avg_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
result = makePolyNumAggStateCurrentContext(false);
@@ -5878,7 +5871,6 @@ int8_avg_deserialize(PG_FUNCTION_ARGS)
#endif
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 170b3a3820..be43bf314b 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5532,12 +5532,11 @@ string_agg_deserialize(PG_FUNCTION_ARGS)
sstate = PG_GETARG_BYTEA_PP(0);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ initStringInfoFromString(&buf, VARDATA_ANY(sstate),
+ VARSIZE_ANY_EXHDR(sstate));
result = makeStringAggState(fcinfo);
@@ -5550,7 +5549,6 @@ string_agg_deserialize(PG_FUNCTION_ARGS)
appendBinaryStringInfo(result, data, datalen);
pq_getmsgend(&buf);
- pfree(buf.data);
PG_RETURN_POINTER(result);
}
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 05b22b5c53..2e0bc28527 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -65,6 +65,26 @@ initStringInfo(StringInfo str)
resetStringInfo(str);
}
+/*
+ * initStringInfoFromString
+ *
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * and sets the string to directly point to the contents of 'str' with the
+ * given 'len'.
+ *
+ * Caution: this is primarily used for very short-lived StringInfos. The
+ * resulting StringInfo cannot be appended to unless 'buf' points directly to
+ * a memory address as returned by palloc. Generally, you should be using
+ * appendBinaryStringInfo() instead of this function.
+ */
+void
+initStringInfoFromString(StringInfo str, char *buf, int len)
+{
+ str->data = buf;
+ str->len = str->maxlen = len;
+ str->cursor = 0;
+}
+
/*
* resetStringInfo
*
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 36a416f8e0..4e4d73658d 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -79,6 +79,19 @@ extern StringInfo makeStringInfo(void);
*/
extern void initStringInfo(StringInfo str);
+/*------------------------
+ * initStringInfoFromString
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * and sets the string to directly point to the contents of 'str' with the
+ * given 'len'.
+ *
+ * Caution: this is primarily used for very short-lived StringInfos. The
+ * resulting StringInfo cannot be appended to unless 'buf' points directly to
+ * a memory address as returned by palloc. Generally, you should be using
+ * appendBinaryStringInfo() instead of this function.
+ */
+extern void initStringInfoFromString(StringInfo str, char *buf, int len);
+
/*------------------------
* resetStringInfo
* Clears the current content of the StringInfo, if any. The
--
2.37.2
[text/plain] v1-0002-Add-more-usages-of-initStringInfoFromString.patch (2.9K, ../../CAApHDvr=e-YOigriSHHm324a40HPqcUhSp6pWWgjz5WwegR=cQ@mail.gmail.com/5-v1-0002-Add-more-usages-of-initStringInfoFromString.patch)
download | inline diff:
From 0b31533674adb90644298c638cc7dfe066ab5bd6 Mon Sep 17 00:00:00 2001
From: David Rowley <[email protected]>
Date: Sun, 12 Feb 2023 16:45:15 +1300
Subject: [PATCH v1 2/2] Add more usages of initStringInfoFromString
This slightly reduces some memcpy work in walreceiver.c
---
src/backend/replication/walreceiver.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index f6446da2d6..366b91f923 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -132,7 +132,6 @@ typedef enum WalRcvWakeupReason
static TimestampTz wakeup[NUM_WALRCV_WAKEUPS];
static StringInfoData reply_message;
-static StringInfoData incoming_message;
/* Prototypes for private functions */
static void WalRcvFetchTimeLineHistoryFiles(TimeLineID first, TimeLineID last);
@@ -422,10 +421,12 @@ WalReceiverMain(void)
LSN_FORMAT_ARGS(startpoint), startpointTLI)));
first_stream = false;
- /* Initialize LogstreamResult and buffers for processing messages */
+ /*
+ * Initialize LogstreamResult and reply buffer for processing
+ * messages
+ */
LogstreamResult.Write = LogstreamResult.Flush = GetXLogReplayRecPtr(NULL);
initStringInfo(&reply_message);
- initStringInfo(&incoming_message);
/* Initialize nap wakeup times. */
now = GetCurrentTimestamp();
@@ -843,19 +844,19 @@ XLogWalRcvProcessMsg(unsigned char type, char *buf, Size len, TimeLineID tli)
TimestampTz sendTime;
bool replyRequested;
- resetStringInfo(&incoming_message);
-
switch (type)
{
case 'w': /* WAL records */
{
- /* copy message to StringInfo */
+ StringInfoData incoming_message;
+
+ /* set the message in the incoming_message StringInfo */
hdrlen = sizeof(int64) + sizeof(int64) + sizeof(int64);
if (len < hdrlen)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg_internal("invalid WAL message received from primary")));
- appendBinaryStringInfo(&incoming_message, buf, hdrlen);
+ initStringInfoFromString(&incoming_message, buf, hdrlen);
/* read the fields */
dataStart = pq_getmsgint64(&incoming_message);
@@ -870,13 +871,15 @@ XLogWalRcvProcessMsg(unsigned char type, char *buf, Size len, TimeLineID tli)
}
case 'k': /* Keepalive */
{
- /* copy message to StringInfo */
+ StringInfoData incoming_message;
+
+ /* set the message in the incoming_message StringInfo */
hdrlen = sizeof(int64) + sizeof(int64) + sizeof(char);
if (len != hdrlen)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg_internal("invalid keepalive message received from primary")));
- appendBinaryStringInfo(&incoming_message, buf, hdrlen);
+ initStringInfoFromString(&incoming_message, buf, hdrlen);
/* read the fields */
walEnd = pq_getmsgint64(&incoming_message);
--
2.37.2
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Making aggregate deserialization (and WAL receive) functions slightly faster
2023-02-12 05:38 Making aggregate deserialization (and WAL receive) functions slightly faster David Rowley <[email protected]>
@ 2023-02-12 06:39 ` Tom Lane <[email protected]>
2023-02-12 10:43 ` Re: Making aggregate deserialization (and WAL receive) functions slightly faster David Rowley <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2023-02-12 06:39 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: PostgreSQL Developers <[email protected]>
David Rowley <[email protected]> writes:
> While working on 16fd03e95, I noticed that in each aggregate
> deserialization function, in order to "receive" the bytea value that
> is the serialized aggregate state, appendBinaryStringInfo is used to
> append the bytes of the bytea value onto a temporary StringInfoData.
> Using appendBinaryStringInfo seems a bit wasteful here. We could
> really just fake up a StringInfoData and point directly to the bytes
> of the bytea value.
Perhaps, but ...
> The best way I could think of to do this was to invent
> initStringInfoFromString() which initialises a StringInfoData and has
> the ->data field point directly at the specified buffer. This will
> mean that it would be unsafe to do any appendStringInfo* operations on
> the resulting StringInfoData as enlargeStringInfo would try to
> repalloc the data buffer, which might not even point to a palloc'd
> string.
I find this patch horribly dangerous.
It could maybe be okay if we added the capability for StringInfoData
to understand (and enforce) that its "data" buffer is read-only.
However, that'd add overhead to every existing use-case.
> I've attached the benchmark results I got after testing how the
> modification changed the performance of string_agg_deserialize().
> I was hoping this would have a slightly more impressive performance
> impact, especially for string_agg() and array_agg() as the aggregate
> states of those can be large. However, in the test I ran, there's
> only a very slight performance gain. I may just not have found the
> best case, however.
I do not think we should even consider this without solid evidence
for *major* performance improvements. As it stands, it's a
quintessential example of a loaded foot-gun, and it seems clear
that making it safe enough to use would add more overhead than
it saves.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Making aggregate deserialization (and WAL receive) functions slightly faster
2023-02-12 05:38 Making aggregate deserialization (and WAL receive) functions slightly faster David Rowley <[email protected]>
2023-02-12 06:39 ` Re: Making aggregate deserialization (and WAL receive) functions slightly faster Tom Lane <[email protected]>
@ 2023-02-12 10:43 ` David Rowley <[email protected]>
2023-10-03 05:02 ` Re: Making aggregate deserialization (and WAL receive) functions slightly faster David Rowley <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: David Rowley @ 2023-02-12 10:43 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: PostgreSQL Developers <[email protected]>
On Sun, 12 Feb 2023 at 19:39, Tom Lane <[email protected]> wrote:
> I find this patch horribly dangerous.
I see LogicalRepApplyLoop() does something similar with a
StringInfoData. Maybe it's just scarier having an external function in
stringinfo.c which does this as having it increases the chances of
someone using it for the wrong thing.
> It could maybe be okay if we added the capability for StringInfoData
> to understand (and enforce) that its "data" buffer is read-only.
> However, that'd add overhead to every existing use-case.
I'm not very excited by that. I considered just setting maxlen = -1
in the new function and adding Asserts to check for that in each of
the appendStringInfo* functions. However, since the performance gains
are not so great, I'll probably just drop the whole thing given
there's resistance.
David
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Making aggregate deserialization (and WAL receive) functions slightly faster
2023-02-12 05:38 Making aggregate deserialization (and WAL receive) functions slightly faster David Rowley <[email protected]>
2023-02-12 06:39 ` Re: Making aggregate deserialization (and WAL receive) functions slightly faster Tom Lane <[email protected]>
2023-02-12 10:43 ` Re: Making aggregate deserialization (and WAL receive) functions slightly faster David Rowley <[email protected]>
@ 2023-10-03 05:02 ` David Rowley <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: David Rowley @ 2023-10-03 05:02 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: PostgreSQL Developers <[email protected]>
On Sun, 12 Feb 2023 at 23:43, David Rowley <[email protected]> wrote:
>
> On Sun, 12 Feb 2023 at 19:39, Tom Lane <[email protected]> wrote:
> > It could maybe be okay if we added the capability for StringInfoData
> > to understand (and enforce) that its "data" buffer is read-only.
> > However, that'd add overhead to every existing use-case.
>
> I'm not very excited by that. I considered just setting maxlen = -1
> in the new function and adding Asserts to check for that in each of
> the appendStringInfo* functions. However, since the performance gains
> are not so great, I'll probably just drop the whole thing given
> there's resistance.
I know I said I'd drop this, but I was reminded of it again today. I
ended up adjusting the patch so that it no longer adds a helper
function to stringinfo.c and instead just manually assigns the
StringInfo.data field to point to the bytea's buffer. This follows
what's done in some existing places such as
LogicalParallelApplyLoop(), ReadArrayBinary() and record_recv() to
name a few.
I ran a fresh set of benchmarks on today's master with and without the
patch applied. I used the same benchmark as I did in [1]. The average
performance increase from between 0 and 12 workers is about 6.6%.
This seems worthwhile to me. Any objections?
David
[1] https://postgr.es/m/CAApHDvr%3De-YOigriSHHm324a40HPqcUhSp6pWWgjz5WwegR%3DcQ%40mail.gmail.com
Attachments:
[image/png] optimize_deserial_bench.png (70.1K, ../../CAApHDvrcYXh72GKjhQh2V=BswETUPsRbR+8=1On23S5jtS4fdA@mail.gmail.com/2-optimize_deserial_bench.png)
download | view image
[application/octet-stream] v1-0001-Optimize-various-aggregate-deserialization-functi.patch (7.1K, ../../CAApHDvrcYXh72GKjhQh2V=BswETUPsRbR+8=1On23S5jtS4fdA@mail.gmail.com/3-v1-0001-Optimize-various-aggregate-deserialization-functi.patch)
download | inline diff:
From 7fc60dc245804a353df4ba1e2ccd96f7d0237eae Mon Sep 17 00:00:00 2001
From: David Rowley <[email protected]>
Date: Sat, 11 Feb 2023 10:05:32 +1300
Subject: [PATCH v1] Optimize various aggregate deserialization functions
The serialized representation of an internal aggregate state is a bytea
value. In each deserial function, in order to "receive" the bytea value
we appended it onto a short-lived StringInfoData using
appendBinaryStringInfo. This was a little wasteful as it meant having to
palloc memory, copy a (possibly long) series of bytes then later pfree
that memory. Instead of doing to this extra trouble, we can just fake up
a StringInfoData and point the data directly at the bytea's payload. This
should help increase the performance of internal aggregate
deserialization.
Discussion: https://postgr.es/m/CAApHDvr=e-YOigriSHHm324a40HPqcUhSp6pWWgjz5WwegR=cQ@mail.gmail.com
---
src/backend/utils/adt/array_userfuncs.c | 24 ++++++-------
src/backend/utils/adt/numeric.c | 48 ++++++++++++-------------
src/backend/utils/adt/varlena.c | 12 +++----
3 files changed, 42 insertions(+), 42 deletions(-)
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index 5c4fdcfba4..7f87df45df 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -723,12 +723,13 @@ array_agg_deserialize(PG_FUNCTION_ARGS)
sstate = PG_GETARG_BYTEA_PP(0);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ buf.data = VARDATA_ANY(sstate);
+ buf.len = VARSIZE_ANY_EXHDR(sstate);
+ buf.maxlen = 0;
+ buf.cursor = 0;
/* element_type */
element_type = pq_getmsgint(&buf, 4);
@@ -825,7 +826,6 @@ array_agg_deserialize(PG_FUNCTION_ARGS)
}
pq_getmsgend(&buf);
- pfree(buf.data);
PG_RETURN_POINTER(result);
}
@@ -1134,12 +1134,13 @@ array_agg_array_deserialize(PG_FUNCTION_ARGS)
sstate = PG_GETARG_BYTEA_PP(0);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ buf.data = VARDATA_ANY(sstate);
+ buf.len = VARSIZE_ANY_EXHDR(sstate);
+ buf.maxlen = 0;
+ buf.cursor = 0;
/* element_type */
element_type = pq_getmsgint(&buf, 4);
@@ -1197,7 +1198,6 @@ array_agg_array_deserialize(PG_FUNCTION_ARGS)
memcpy(result->lbs, temp, sizeof(result->lbs));
pq_getmsgend(&buf);
- pfree(buf.data);
PG_RETURN_POINTER(result);
}
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 3c3184f15b..f4b885005f 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -5190,12 +5190,13 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ buf.data = VARDATA_ANY(sstate);
+ buf.len = VARSIZE_ANY_EXHDR(sstate);
+ buf.maxlen = 0;
+ buf.cursor = 0;
result = makeNumericAggStateCurrentContext(false);
@@ -5222,7 +5223,6 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS)
result->nInfcount = pq_getmsgint64(&buf);
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
@@ -5306,12 +5306,13 @@ numeric_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ buf.data = VARDATA_ANY(sstate);
+ buf.len = VARSIZE_ANY_EXHDR(sstate);
+ buf.maxlen = 0;
+ buf.cursor = 0;
result = makeNumericAggStateCurrentContext(false);
@@ -5342,7 +5343,6 @@ numeric_deserialize(PG_FUNCTION_ARGS)
result->nInfcount = pq_getmsgint64(&buf);
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
@@ -5677,12 +5677,13 @@ numeric_poly_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ buf.data = VARDATA_ANY(sstate);
+ buf.len = VARSIZE_ANY_EXHDR(sstate);
+ buf.maxlen = 0;
+ buf.cursor = 0;
result = makePolyNumAggStateCurrentContext(false);
@@ -5706,7 +5707,6 @@ numeric_poly_deserialize(PG_FUNCTION_ARGS)
#endif
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
@@ -5868,12 +5868,13 @@ int8_avg_deserialize(PG_FUNCTION_ARGS)
init_var(&tmp_var);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ buf.data = VARDATA_ANY(sstate);
+ buf.len = VARSIZE_ANY_EXHDR(sstate);
+ buf.maxlen = 0;
+ buf.cursor = 0;
result = makePolyNumAggStateCurrentContext(false);
@@ -5889,7 +5890,6 @@ int8_avg_deserialize(PG_FUNCTION_ARGS)
#endif
pq_getmsgend(&buf);
- pfree(buf.data);
free_var(&tmp_var);
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 72e1e24fe0..1aff04fa77 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5289,12 +5289,13 @@ string_agg_deserialize(PG_FUNCTION_ARGS)
sstate = PG_GETARG_BYTEA_PP(0);
/*
- * Copy the bytea into a StringInfo so that we can "receive" it using the
- * standard recv-function infrastructure.
+ * Fake up a StringInfo pointing to the bytea's value so we can "receive"
+ * the serialized aggregate state value.
*/
- initStringInfo(&buf);
- appendBinaryStringInfo(&buf,
- VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+ buf.data = VARDATA_ANY(sstate);
+ buf.len = VARSIZE_ANY_EXHDR(sstate);
+ buf.maxlen = 0;
+ buf.cursor = 0;
result = makeStringAggState(fcinfo);
@@ -5307,7 +5308,6 @@ string_agg_deserialize(PG_FUNCTION_ARGS)
appendBinaryStringInfo(result, data, datalen);
pq_getmsgend(&buf);
- pfree(buf.data);
PG_RETURN_POINTER(result);
}
--
2.40.1.windows.1
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-10-03 05:02 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-19 06:11 [PATCH v52 5/7] Doc part of shared-memory based stats collector. Kyotaro Horiguchi <[email protected]>
2023-02-12 05:38 Making aggregate deserialization (and WAL receive) functions slightly faster David Rowley <[email protected]>
2023-02-12 06:39 ` Re: Making aggregate deserialization (and WAL receive) functions slightly faster Tom Lane <[email protected]>
2023-02-12 10:43 ` Re: Making aggregate deserialization (and WAL receive) functions slightly faster David Rowley <[email protected]>
2023-10-03 05:02 ` Re: Making aggregate deserialization (and WAL receive) functions slightly faster David Rowley <[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