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
* Re: Replace known_assigned_xids_lck by memory barrier @ 2023-08-15 10:29 Michail Nikolaev <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Michail Nikolaev @ 2023-08-15 10:29 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]> Hello, Nathan. > What sort of benefits do you see from this patch? It might be worthwhile > in itself to remove spinlocks when possible, but IME it's much easier to > justify such changes when there is a tangible benefit we can point to. Oh, it is not an easy question :) The answer, probably, looks like this: 1) performance benefits of spin lock acquire removing in KnownAssignedXidsGetOldestXmin and KnownAssignedXidsSearch 2) it is closing 13-year-old tech depth But in reality, it is not easy to measure performance improvement consistently for this change. > Are the assignments in question guaranteed to be atomic? IIUC we assume > that aligned 4-byte loads/stores are atomic, so we should be okay as long > as we aren't handling anything larger. Yes, 4-bytes assignment are atomic, locking is used to ensure memory write ordering in this place. > This use of pg_write_barrier() looks correct to me, but don't we need > corresponding read barriers wherever we obtain the pointers? FWIW I tend > to review src/backend/storage/lmgr/README.barrier in its entirety whenever > I deal with this stuff. Oh, yeah, you're right! (1) I'll prepare an updated version of the patch soon. I don't why I was assuming pg_write_barrier is enough (⊙＿⊙') [1]: https://github.com/postgres/postgres/blob/master/src/backend/storage/lmgr/README.barrier#L125 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Replace known_assigned_xids_lck by memory barrier @ 2023-08-15 15:22 Nathan Bossart <[email protected]> parent: Michail Nikolaev <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Nathan Bossart @ 2023-08-15 15:22 UTC (permalink / raw) To: Michail Nikolaev <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]> On Tue, Aug 15, 2023 at 12:29:24PM +0200, Michail Nikolaev wrote: >> What sort of benefits do you see from this patch? It might be worthwhile >> in itself to remove spinlocks when possible, but IME it's much easier to >> justify such changes when there is a tangible benefit we can point to. > > Oh, it is not an easy question :) > > The answer, probably, looks like this: > 1) performance benefits of spin lock acquire removing in > KnownAssignedXidsGetOldestXmin and KnownAssignedXidsSearch > 2) it is closing 13-year-old tech depth > > But in reality, it is not easy to measure performance improvement > consistently for this change. Okay. Elsewhere, it seems like folks are fine with patches that reduce shared memory space via atomics or barriers even if there's no immediate benefit [0], so I think it's fine to proceed. >> Are the assignments in question guaranteed to be atomic? IIUC we assume >> that aligned 4-byte loads/stores are atomic, so we should be okay as long >> as we aren't handling anything larger. > > Yes, 4-bytes assignment are atomic, locking is used to ensure memory > write ordering in this place. Yeah, it looks like both the values that are protected by known_assigned_xids_lck are integers, so this should be okay. One remaining question I have is whether it is okay if we see an updated value for one of the head/tail variables but not the other. It looks like the tail variable is only updated with ProcArrayLock held exclusively, which IIUC wouldn't prevent such mismatches even today, since we use a separate spinlock for reading them in some cases. [0] https://postgr.es/m/20230524214958.mt6f5xokpumvnrio%40awork3.anarazel.de -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Replace known_assigned_xids_lck by memory barrier @ 2023-08-16 15:30 Michail Nikolaev <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Michail Nikolaev @ 2023-08-16 15:30 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]> Hello! Updated version (with read barriers is attached). > One remaining question I have is whether it is okay if we see an updated value > for one of the head/tail variables but not the other. It looks like the > tail variable is only updated with ProcArrayLock held exclusively, which > IIUC wouldn't prevent such mismatches even today, since we use a separate > spinlock for reading them in some cases. 1) "The convention is that backends must hold shared ProcArrayLock to examine the array", it is applied to pointers as well 2) Also, "only the startup process modifies the head/tail pointers." So, the "tail" variable is updated by the startup process with ProcArrayLock held in exclusive-only mode - so, no issues here. Regarding "head" variable - updates by the startup processes are possible in next cases: * ProcArrayLock in exclusive mode (like KnownAssignedXidsCompress or KnownAssignedXidsSearch(remove=true)), no issues here * ProcArrayLock not taken at all (like KnownAssignedXidsAdd(exclusive_lock=false)) in such case we rely on memory barrier machinery Both head and tail variables are changed only with exclusive lock held. I'll think more, but can't find something wrong here so far. Attachments: [text/x-patch] v3-0001-Removes-known_assigned_xids_lck-using-the-write-m.patch (5.1K, ../../CANtu0ogdo86z-+oQVT7LENPPe_W4N5Q4HsisM_Xpf-H=v5vaow@mail.gmail.com/2-v3-0001-Removes-known_assigned_xids_lck-using-the-write-m.patch) download | inline diff: diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 2a3da49b8f..e4093ed06d 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -61,7 +61,6 @@ #include "port/pg_lfind.h" #include "storage/proc.h" #include "storage/procarray.h" -#include "storage/spin.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/rel.h" @@ -82,7 +81,6 @@ typedef struct ProcArrayStruct int numKnownAssignedXids; /* current # of valid entries */ int tailKnownAssignedXids; /* index of oldest valid element */ int headKnownAssignedXids; /* index of newest element, + 1 */ - slock_t known_assigned_xids_lck; /* protects head/tail pointers */ /* * Highest subxid that has been removed from KnownAssignedXids array to @@ -441,7 +439,6 @@ CreateSharedProcArray(void) procArray->numKnownAssignedXids = 0; procArray->tailKnownAssignedXids = 0; procArray->headKnownAssignedXids = 0; - SpinLockInit(&procArray->known_assigned_xids_lck); procArray->lastOverflowedXid = InvalidTransactionId; procArray->replication_slot_xmin = InvalidTransactionId; procArray->replication_slot_catalog_xmin = InvalidTransactionId; @@ -4564,11 +4561,11 @@ KnownAssignedTransactionIdsIdleMaintenance(void) * pointer. This wouldn't require any lock at all, except that on machines * with weak memory ordering we need to be careful that other processors * see the array element changes before they see the head pointer change. - * We handle this by using a spinlock to protect reads and writes of the - * head/tail pointers. (We could dispense with the spinlock if we were to - * create suitable memory access barrier primitives and use those instead.) - * The spinlock must be taken to read or write the head/tail pointers unless - * the caller holds ProcArrayLock exclusively. + * We handle this by using a memory barrier to protect writes of the + * head pointer. + * The memory barrier is taken before write the head pointer unless + * the caller holds ProcArrayLock exclusively. Appropriate read memory barrier + * is taken accordingly. * * Algorithmic analysis: * @@ -4755,7 +4752,7 @@ KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid, /* * Since only the startup process modifies the head/tail pointers, we - * don't need a lock to read them here. + * are safe read them here. */ head = pArray->headKnownAssignedXids; tail = pArray->tailKnownAssignedXids; @@ -4806,22 +4803,17 @@ KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid, pArray->numKnownAssignedXids += nxids; /* - * Now update the head pointer. We use a spinlock to protect this + * Now update the head pointer. We use a memory barrier to protect this * pointer, not because the update is likely to be non-atomic, but to * ensure that other processors see the above array updates before they * see the head pointer change. * * If we're holding ProcArrayLock exclusively, there's no need to take the - * spinlock. + * barrier. */ - if (exclusive_lock) - pArray->headKnownAssignedXids = head; - else - { - SpinLockAcquire(&pArray->known_assigned_xids_lck); - pArray->headKnownAssignedXids = head; - SpinLockRelease(&pArray->known_assigned_xids_lck); - } + if (!exclusive_lock) + pg_write_barrier(); + pArray->headKnownAssignedXids = head; } /* @@ -4843,20 +4835,12 @@ KnownAssignedXidsSearch(TransactionId xid, bool remove) int tail; int result_index = -1; - if (remove) - { - /* we hold ProcArrayLock exclusively, so no need for spinlock */ - tail = pArray->tailKnownAssignedXids; - head = pArray->headKnownAssignedXids; - } - else - { - /* take spinlock to ensure we see up-to-date array contents */ - SpinLockAcquire(&pArray->known_assigned_xids_lck); - tail = pArray->tailKnownAssignedXids; - head = pArray->headKnownAssignedXids; - SpinLockRelease(&pArray->known_assigned_xids_lck); - } + tail = pArray->tailKnownAssignedXids; + head = pArray->headKnownAssignedXids; + + /* in case of remove we hold ProcArrayLock exclusively, so no need for barrier */ + if (!remove) + pg_read_barrier(); /* * Standard binary search. Note we can ignore the KnownAssignedXidsValid @@ -5095,12 +5079,11 @@ KnownAssignedXidsGetAndSetXmin(TransactionId *xarray, TransactionId *xmin, * might miss newly-added xids, but they should be >= xmax so irrelevant * anyway. * - * Must take spinlock to ensure we see up-to-date array contents. + * Must take read memory barrier to ensure we see up-to-date array contents. */ - SpinLockAcquire(&procArray->known_assigned_xids_lck); tail = procArray->tailKnownAssignedXids; head = procArray->headKnownAssignedXids; - SpinLockRelease(&procArray->known_assigned_xids_lck); + pg_read_barrier(); for (i = tail; i < head; i++) { @@ -5147,10 +5130,9 @@ KnownAssignedXidsGetOldestXmin(void) /* * Fetch head just once, since it may change while we loop. */ - SpinLockAcquire(&procArray->known_assigned_xids_lck); tail = procArray->tailKnownAssignedXids; head = procArray->headKnownAssignedXids; - SpinLockRelease(&procArray->known_assigned_xids_lck); + pg_read_barrier(); for (i = tail; i < head; i++) { ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Replace known_assigned_xids_lck by memory barrier @ 2023-08-16 18:32 Nathan Bossart <[email protected]> parent: Michail Nikolaev <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Nathan Bossart @ 2023-08-16 18:32 UTC (permalink / raw) To: Michail Nikolaev <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]> On Wed, Aug 16, 2023 at 05:30:59PM +0200, Michail Nikolaev wrote: > Updated version (with read barriers is attached). Thanks for the updated patch. I've attached v4 in which I've made a number of cosmetic edits. > I'll think more, but can't find something wrong here so far. IIUC this memory barrier stuff is only applicable to KnownAssignedXidsAdd() (without an exclusive lock) when we add entries to the end of the array and then update the head pointer. Otherwise, appropriate locks are taken when reading/writing the array. For example, say we have the following array: head | v [ 0, 1, 2, 3 ] When adding elements, we keep the head pointer where it is: head | v [ 0, 1, 2, 3, 4, 5 ] If another processor sees this intermediate state, it's okay because it will only inspect elements 0 through 3. Only at the end do we update the head pointer: head | v [ 0, 1, 2, 3, 4, 5 ] With weak memory ordering and no barriers, another process may see this (which is obviously no good): head | v [ 0, 1, 2, 3 ] One thing that I'm still trying to understand is this code in KnownAssignedXidsSearch(): /* we hold ProcArrayLock exclusively, so no need for spinlock */ tail = pArray->tailKnownAssignedXids; head = pArray->headKnownAssignedXids; It's not clear to me why holding ProcArrayLock exclusively means we don't need to worry about the spinlock/barriers. If KnownAssignedXidsAdd() adds entries without a lock, holding ProcArrayLock won't protect you, and I don't see anything else that acts as a read barrier before the array entries are inspected. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v4-0001-Replace-known_assigned_xids_lck-with-memory-barri.patch (5.8K, ../../20230816183236.GA2812457@nathanxps13/2-v4-0001-Replace-known_assigned_xids_lck-with-memory-barri.patch) download | inline diff: From 11de5076adc060c0dde32e8538714301f9b98d02 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 16 Aug 2023 10:52:56 -0700 Subject: [PATCH v4 1/1] Replace known_assigned_xids_lck with memory barriers. Suggested-by: Tom Lane Author: Michail Nikolaev Discussion: https://postgr.es/m/CANtu0oh0si%3DjG5z_fLeFtmYcETssQ08kLEa8b6TQqDm_cinroA%40mail.gmail.com --- src/backend/storage/ipc/procarray.c | 71 ++++++++++------------------- 1 file changed, 25 insertions(+), 46 deletions(-) diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 2a3da49b8f..a3def020b6 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -61,7 +61,6 @@ #include "port/pg_lfind.h" #include "storage/proc.h" #include "storage/procarray.h" -#include "storage/spin.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/rel.h" @@ -82,7 +81,6 @@ typedef struct ProcArrayStruct int numKnownAssignedXids; /* current # of valid entries */ int tailKnownAssignedXids; /* index of oldest valid element */ int headKnownAssignedXids; /* index of newest element, + 1 */ - slock_t known_assigned_xids_lck; /* protects head/tail pointers */ /* * Highest subxid that has been removed from KnownAssignedXids array to @@ -441,7 +439,6 @@ CreateSharedProcArray(void) procArray->numKnownAssignedXids = 0; procArray->tailKnownAssignedXids = 0; procArray->headKnownAssignedXids = 0; - SpinLockInit(&procArray->known_assigned_xids_lck); procArray->lastOverflowedXid = InvalidTransactionId; procArray->replication_slot_xmin = InvalidTransactionId; procArray->replication_slot_catalog_xmin = InvalidTransactionId; @@ -4561,14 +4558,10 @@ KnownAssignedTransactionIdsIdleMaintenance(void) * during normal running). Compressing unused entries out of the array * likewise requires exclusive lock. To add XIDs to the array, we just insert * them into slots to the right of the head pointer and then advance the head - * pointer. This wouldn't require any lock at all, except that on machines - * with weak memory ordering we need to be careful that other processors - * see the array element changes before they see the head pointer change. - * We handle this by using a spinlock to protect reads and writes of the - * head/tail pointers. (We could dispense with the spinlock if we were to - * create suitable memory access barrier primitives and use those instead.) - * The spinlock must be taken to read or write the head/tail pointers unless - * the caller holds ProcArrayLock exclusively. + * pointer. This doesn't require any lock at all, but on machines with weak + * memory ordering, we need to be careful that other processors see the array + * element changes before they see the head pointer change. We handle this by + * using memory barriers. * * Algorithmic analysis: * @@ -4806,22 +4799,15 @@ KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid, pArray->numKnownAssignedXids += nxids; /* - * Now update the head pointer. We use a spinlock to protect this - * pointer, not because the update is likely to be non-atomic, but to - * ensure that other processors see the above array updates before they - * see the head pointer change. - * - * If we're holding ProcArrayLock exclusively, there's no need to take the - * spinlock. + * Now update the head pointer. We use a write barrier to ensure that + * other processors see the above array updates before they see the head + * pointer change. The barrier isn't required if we're holding + * ProcArrayLock exclusively. */ - if (exclusive_lock) - pArray->headKnownAssignedXids = head; - else - { - SpinLockAcquire(&pArray->known_assigned_xids_lck); - pArray->headKnownAssignedXids = head; - SpinLockRelease(&pArray->known_assigned_xids_lck); - } + if (!exclusive_lock) + pg_write_barrier(); + + pArray->headKnownAssignedXids = head; } /* @@ -4843,20 +4829,15 @@ KnownAssignedXidsSearch(TransactionId xid, bool remove) int tail; int result_index = -1; - if (remove) - { - /* we hold ProcArrayLock exclusively, so no need for spinlock */ - tail = pArray->tailKnownAssignedXids; - head = pArray->headKnownAssignedXids; - } - else - { - /* take spinlock to ensure we see up-to-date array contents */ - SpinLockAcquire(&pArray->known_assigned_xids_lck); - tail = pArray->tailKnownAssignedXids; - head = pArray->headKnownAssignedXids; - SpinLockRelease(&pArray->known_assigned_xids_lck); - } + tail = pArray->tailKnownAssignedXids; + head = pArray->headKnownAssignedXids; + + /* + * If we know that we're holding ProcArrayLock exclusively, we don't need + * the read barrier. + */ + if (!remove) + pg_read_barrier(); /* pairs with KnownAssignedXidsAdd */ /* * Standard binary search. Note we can ignore the KnownAssignedXidsValid @@ -5094,13 +5075,11 @@ KnownAssignedXidsGetAndSetXmin(TransactionId *xarray, TransactionId *xmin, * cannot enter and then leave the array while we hold ProcArrayLock. We * might miss newly-added xids, but they should be >= xmax so irrelevant * anyway. - * - * Must take spinlock to ensure we see up-to-date array contents. */ - SpinLockAcquire(&procArray->known_assigned_xids_lck); tail = procArray->tailKnownAssignedXids; head = procArray->headKnownAssignedXids; - SpinLockRelease(&procArray->known_assigned_xids_lck); + + pg_read_barrier(); /* pairs with KnownAssignedXidsAdd */ for (i = tail; i < head; i++) { @@ -5147,10 +5126,10 @@ KnownAssignedXidsGetOldestXmin(void) /* * Fetch head just once, since it may change while we loop. */ - SpinLockAcquire(&procArray->known_assigned_xids_lck); tail = procArray->tailKnownAssignedXids; head = procArray->headKnownAssignedXids; - SpinLockRelease(&procArray->known_assigned_xids_lck); + + pg_read_barrier(); /* pairs with KnownAssignedXidsAdd */ for (i = tail; i < head; i++) { -- 2.25.1 ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-08-16 18:32 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-08-15 10:29 Re: Replace known_assigned_xids_lck by memory barrier Michail Nikolaev <[email protected]> 2023-08-15 15:22 ` Re: Replace known_assigned_xids_lck by memory barrier Nathan Bossart <[email protected]> 2023-08-16 15:30 ` Re: Replace known_assigned_xids_lck by memory barrier Michail Nikolaev <[email protected]> 2023-08-16 18:32 ` Re: Replace known_assigned_xids_lck by memory barrier Nathan Bossart <[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