public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 4/4] Update functions to pass only errinfo struct.. 4+ messages / 3 participants [nested] [flat]
* [PATCH 4/4] Update functions to pass only errinfo struct.. @ 2020-06-22 22:13 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2020-06-22 22:13 UTC (permalink / raw) ..this is a more complete change, and probably a good idea since parallel workers have an partially-populated LVRelStats, and it's better to avoid the idea that it's usable (dead_tuples in particular is a bogus pointer). --- src/backend/access/heap/vacuumlazy.c | 58 ++++++++++++++-------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 6bd2867660..db3bd86338 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -344,10 +344,10 @@ static void lazy_vacuum_all_indexes(Relation onerel, Relation *Irel, LVRelStats *vacrelstats, LVParallelState *lps, int nindexes); static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, - LVDeadTuples *dead_tuples, double reltuples, LVRelStats *vacrelstats); + LVDeadTuples *dead_tuples, double reltuples, LVSavedPosition *errinfo); static void lazy_cleanup_index(Relation indrel, IndexBulkDeleteResult **stats, - double reltuples, bool estimated_count, LVRelStats *vacrelstats); + double reltuples, bool estimated_count, LVSavedPosition *errinfo); static int lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer, int tupindex, LVRelStats *vacrelstats, Buffer *vmbuffer); static bool should_attempt_truncation(VacuumParams *params, @@ -367,13 +367,13 @@ static void lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult * int nindexes); static void parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVDeadTuples *dead_tuples, - int nindexes, LVRelStats *vacrelstats); + int nindexes, LVSavedPosition *errinfo); static void vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats, LVRelStats *vacrelstats, LVParallelState *lps, int nindexes); static void vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVSharedIndStats *shared_indstats, - LVDeadTuples *dead_tuples, LVRelStats *vacrelstats); + LVDeadTuples *dead_tuples, LVSavedPosition *errinfo); static void lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, LVRelStats *vacrelstats, LVParallelState *lps, int nindexes); @@ -1798,7 +1798,7 @@ lazy_vacuum_all_indexes(Relation onerel, Relation *Irel, for (idx = 0; idx < nindexes; idx++) lazy_vacuum_index(Irel[idx], &stats[idx], vacrelstats->dead_tuples, - vacrelstats->old_live_tuples, vacrelstats); + vacrelstats->old_live_tuples, &vacrelstats->errinfo); } /* Increase and report the number of index scans */ @@ -2163,7 +2163,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats, * indexes in the case where no workers are launched. */ parallel_vacuum_index(Irel, stats, lps->lvshared, - vacrelstats->dead_tuples, nindexes, vacrelstats); + vacrelstats->dead_tuples, nindexes, &vacrelstats->errinfo); /* * Next, accumulate buffer and WAL usage. (This must wait for the workers @@ -2198,7 +2198,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats, static void parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVDeadTuples *dead_tuples, - int nindexes, LVRelStats *vacrelstats) + int nindexes, LVSavedPosition *errinfo) { /* * Increment the active worker count if we are able to launch any worker. @@ -2232,7 +2232,7 @@ parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats, /* Do vacuum or cleanup of the index */ vacuum_one_index(Irel[idx], &(stats[idx]), lvshared, shared_indstats, - dead_tuples, vacrelstats); + dead_tuples, errinfo); } /* @@ -2273,7 +2273,7 @@ vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats, skip_parallel_vacuum_index(Irel[i], lps->lvshared)) vacuum_one_index(Irel[i], &(stats[i]), lps->lvshared, shared_indstats, vacrelstats->dead_tuples, - vacrelstats); + &vacrelstats->errinfo); } /* @@ -2293,7 +2293,7 @@ vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats, static void vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVSharedIndStats *shared_indstats, - LVDeadTuples *dead_tuples, LVRelStats *vacrelstats) + LVDeadTuples *dead_tuples, LVSavedPosition *errinfo) { IndexBulkDeleteResult *bulkdelete_res = NULL; @@ -2313,10 +2313,10 @@ vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, /* Do vacuum or cleanup of the index */ if (lvshared->for_cleanup) lazy_cleanup_index(indrel, stats, lvshared->reltuples, - lvshared->estimated_count, vacrelstats); + lvshared->estimated_count, errinfo); else lazy_vacuum_index(indrel, stats, dead_tuples, - lvshared->reltuples, vacrelstats); + lvshared->reltuples, errinfo); /* * Copy the index bulk-deletion result returned from ambulkdelete and @@ -2392,7 +2392,7 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, lazy_cleanup_index(Irel[idx], &stats[idx], vacrelstats->new_rel_tuples, vacrelstats->tupcount_pages < vacrelstats->rel_pages, - vacrelstats); + &vacrelstats->errinfo); } } @@ -2407,7 +2407,7 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, */ static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, - LVDeadTuples *dead_tuples, double reltuples, LVRelStats *vacrelstats) + LVDeadTuples *dead_tuples, double reltuples, LVSavedPosition *errinfo) { IndexVacuumInfo ivinfo; const char *msg; @@ -2425,8 +2425,8 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, ivinfo.strategy = vac_strategy; /* Update error traceback information */ - olderrinfo = vacrelstats->errinfo; - update_vacuum_error_info(&vacrelstats->errinfo, + olderrinfo = *errinfo; + update_vacuum_error_info(errinfo, VACUUM_ERRCB_PHASE_VACUUM_INDEX, InvalidBlockNumber, RelationGetRelationName(indrel)); @@ -2442,12 +2442,12 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, ereport(elevel, (errmsg(msg, - vacrelstats->errinfo.indname, + errinfo->indname, dead_tuples->num_tuples), errdetail_internal("%s", pg_rusage_show(&ru0)))); /* Revert to the previous phase information for error traceback */ - update_vacuum_error_info(&vacrelstats->errinfo, + update_vacuum_error_info(errinfo, olderrinfo.phase, olderrinfo.blkno, olderrinfo.indname); @@ -2462,7 +2462,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, static void lazy_cleanup_index(Relation indrel, IndexBulkDeleteResult **stats, - double reltuples, bool estimated_count, LVRelStats *vacrelstats) + double reltuples, bool estimated_count, LVSavedPosition *errinfo) { IndexVacuumInfo ivinfo; const char *msg; @@ -2481,8 +2481,8 @@ lazy_cleanup_index(Relation indrel, ivinfo.strategy = vac_strategy; /* Update error traceback information */ - olderrinfo = vacrelstats->errinfo; - update_vacuum_error_info(&vacrelstats->errinfo, + olderrinfo = *errinfo; + update_vacuum_error_info(errinfo, VACUUM_ERRCB_PHASE_INDEX_CLEANUP, InvalidBlockNumber, RelationGetRelationName(indrel)); @@ -2490,7 +2490,7 @@ lazy_cleanup_index(Relation indrel, *stats = index_vacuum_cleanup(&ivinfo, *stats); /* Revert back to the old phase information for error traceback */ - update_vacuum_error_info(&vacrelstats->errinfo, + update_vacuum_error_info(errinfo, olderrinfo.phase, olderrinfo.blkno, olderrinfo.indname); @@ -3474,7 +3474,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) int nindexes; char *sharedquery; IndexBulkDeleteResult **stats; - LVRelStats vacrelstats; + LVSavedPosition errinfo; ErrorContextCallback errcallback; lvshared = (LVShared *) shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_SHARED, @@ -3529,14 +3529,14 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) * Initialize vacrelstats for use as error callback arg by parallel * worker. */ - vacrelstats.errinfo.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); - vacrelstats.errinfo.relname = pstrdup(RelationGetRelationName(onerel)); - vacrelstats.errinfo.indname = NULL; - vacrelstats.errinfo.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */ + errinfo.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); + errinfo.relname = pstrdup(RelationGetRelationName(onerel)); + errinfo.indname = NULL; + errinfo.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */ /* Setup error traceback support for ereport() */ errcallback.callback = vacuum_error_callback; - errcallback.arg = &vacrelstats.errinfo; + errcallback.arg = &errinfo; errcallback.previous = error_context_stack; error_context_stack = &errcallback; @@ -3545,7 +3545,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) /* Process indexes to perform vacuum/cleanup */ parallel_vacuum_index(indrels, stats, lvshared, dead_tuples, nindexes, - &vacrelstats); + &errinfo); /* Report buffer/WAL usage during parallel execution */ buffer_usage = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, false); -- 2.17.0 --oLBj+sq0vYjzfsbl-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* [PATCH v2 5/5] Update functions to pass only errinfo struct.. @ 2020-06-22 22:13 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2020-06-22 22:13 UTC (permalink / raw) ..this is a more complete change, and probably a good idea since parallel workers have an partially-populated LVRelStats, and it's better to avoid the idea that it's usable (dead_tuples in particular is a bogus pointer). --- src/backend/access/heap/vacuumlazy.c | 64 ++++++++++++++-------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 9eb4bc66ae..d239ad4d62 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -344,10 +344,10 @@ static void lazy_vacuum_all_indexes(Relation onerel, Relation *Irel, LVRelStats *vacrelstats, LVParallelState *lps, int nindexes); static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, - LVDeadTuples *dead_tuples, double reltuples, LVRelStats *vacrelstats); + LVDeadTuples *dead_tuples, double reltuples, LVSavedPosition *errinfo); static void lazy_cleanup_index(Relation indrel, IndexBulkDeleteResult **stats, - double reltuples, bool estimated_count, LVRelStats *vacrelstats); + double reltuples, bool estimated_count, LVSavedPosition *errinfo); static int lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer, int tupindex, LVRelStats *vacrelstats, Buffer *vmbuffer); static bool should_attempt_truncation(VacuumParams *params, @@ -367,13 +367,13 @@ static void lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult * int nindexes); static void parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVDeadTuples *dead_tuples, - int nindexes, LVRelStats *vacrelstats); + int nindexes, LVSavedPosition *errinfo); static void vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats, LVRelStats *vacrelstats, LVParallelState *lps, int nindexes); static void vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVSharedIndStats *shared_indstats, - LVDeadTuples *dead_tuples, LVRelStats *vacrelstats); + LVDeadTuples *dead_tuples, LVSavedPosition *errinfo); static void lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, LVRelStats *vacrelstats, LVParallelState *lps, int nindexes); @@ -1797,7 +1797,7 @@ lazy_vacuum_all_indexes(Relation onerel, Relation *Irel, for (idx = 0; idx < nindexes; idx++) lazy_vacuum_index(Irel[idx], &stats[idx], vacrelstats->dead_tuples, - vacrelstats->old_live_tuples, vacrelstats); + vacrelstats->old_live_tuples, &vacrelstats->errinfo); } /* Increase and report the number of index scans */ @@ -2160,7 +2160,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats, * indexes in the case where no workers are launched. */ parallel_vacuum_index(Irel, stats, lps->lvshared, - vacrelstats->dead_tuples, nindexes, vacrelstats); + vacrelstats->dead_tuples, nindexes, &vacrelstats->errinfo); /* * Next, accumulate buffer and WAL usage. (This must wait for the workers @@ -2195,7 +2195,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats, static void parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVDeadTuples *dead_tuples, - int nindexes, LVRelStats *vacrelstats) + int nindexes, LVSavedPosition *errinfo) { /* * Increment the active worker count if we are able to launch any worker. @@ -2229,7 +2229,7 @@ parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats, /* Do vacuum or cleanup of the index */ vacuum_one_index(Irel[idx], &(stats[idx]), lvshared, shared_indstats, - dead_tuples, vacrelstats); + dead_tuples, errinfo); } /* @@ -2270,7 +2270,7 @@ vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats, skip_parallel_vacuum_index(Irel[i], lps->lvshared)) vacuum_one_index(Irel[i], &(stats[i]), lps->lvshared, shared_indstats, vacrelstats->dead_tuples, - vacrelstats); + &vacrelstats->errinfo); } /* @@ -2290,7 +2290,7 @@ vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats, static void vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVSharedIndStats *shared_indstats, - LVDeadTuples *dead_tuples, LVRelStats *vacrelstats) + LVDeadTuples *dead_tuples, LVSavedPosition *errinfo) { IndexBulkDeleteResult *bulkdelete_res = NULL; @@ -2310,10 +2310,10 @@ vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, /* Do vacuum or cleanup of the index */ if (lvshared->for_cleanup) lazy_cleanup_index(indrel, stats, lvshared->reltuples, - lvshared->estimated_count, vacrelstats); + lvshared->estimated_count, errinfo); else lazy_vacuum_index(indrel, stats, dead_tuples, - lvshared->reltuples, vacrelstats); + lvshared->reltuples, errinfo); /* * Copy the index bulk-deletion result returned from ambulkdelete and @@ -2389,7 +2389,7 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, lazy_cleanup_index(Irel[idx], &stats[idx], vacrelstats->new_rel_tuples, vacrelstats->tupcount_pages < vacrelstats->rel_pages, - vacrelstats); + &vacrelstats->errinfo); } } @@ -2404,7 +2404,7 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, */ static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, - LVDeadTuples *dead_tuples, double reltuples, LVRelStats *vacrelstats) + LVDeadTuples *dead_tuples, double reltuples, LVSavedPosition *errinfo) { IndexVacuumInfo ivinfo; const char *msg; @@ -2422,10 +2422,10 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, ivinfo.strategy = vac_strategy; /* Update error traceback information */ - olderrinfo = vacrelstats->errinfo; + olderrinfo = *errinfo; /* The index name is also saved during this phase */ - vacrelstats->errinfo.indname = pstrdup(RelationGetRelationName(indrel)); - update_vacuum_error_info(&vacrelstats->errinfo, + errinfo->indname = pstrdup(RelationGetRelationName(indrel)); + update_vacuum_error_info(errinfo, VACUUM_ERRCB_PHASE_VACUUM_INDEX, InvalidBlockNumber); @@ -2440,15 +2440,15 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, ereport(elevel, (errmsg(msg, - vacrelstats->errinfo.indname, + errinfo->indname, dead_tuples->num_tuples), errdetail_internal("%s", pg_rusage_show(&ru0)))); /* Revert to the previous phase information for error traceback */ - update_vacuum_error_info(&vacrelstats->errinfo, + update_vacuum_error_info(errinfo, olderrinfo.phase, olderrinfo.blkno); - pfree(vacrelstats->errinfo.indname); + pfree(errinfo->indname); } /* @@ -2460,7 +2460,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, static void lazy_cleanup_index(Relation indrel, IndexBulkDeleteResult **stats, - double reltuples, bool estimated_count, LVRelStats *vacrelstats) + double reltuples, bool estimated_count, LVSavedPosition *errinfo) { IndexVacuumInfo ivinfo; const char *msg; @@ -2479,20 +2479,20 @@ lazy_cleanup_index(Relation indrel, ivinfo.strategy = vac_strategy; /* Update error traceback information */ - olderrinfo = vacrelstats->errinfo; + olderrinfo = *errinfo; /* The index name is also saved during this phase */ - vacrelstats->errinfo.indname = pstrdup(RelationGetRelationName(indrel)); - update_vacuum_error_info(&vacrelstats->errinfo, + errinfo->indname = pstrdup(RelationGetRelationName(indrel)); + update_vacuum_error_info(errinfo, VACUUM_ERRCB_PHASE_INDEX_CLEANUP, InvalidBlockNumber); *stats = index_vacuum_cleanup(&ivinfo, *stats); /* Revert back to the old phase information for error traceback */ - update_vacuum_error_info(&vacrelstats->errinfo, + update_vacuum_error_info(errinfo, olderrinfo.phase, olderrinfo.blkno); - pfree(vacrelstats->errinfo.indname); + pfree(errinfo->indname); if (!(*stats)) return; @@ -3474,7 +3474,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) int nindexes; char *sharedquery; IndexBulkDeleteResult **stats; - LVRelStats vacrelstats; + LVSavedPosition errinfo; ErrorContextCallback errcallback; lvshared = (LVShared *) shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_SHARED, @@ -3529,13 +3529,13 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) * Initialize vacrelstats for use as error callback arg by parallel * worker. */ - vacrelstats.errinfo.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); - vacrelstats.errinfo.relname = pstrdup(RelationGetRelationName(onerel)); - vacrelstats.errinfo.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */ + errinfo.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); + errinfo.relname = pstrdup(RelationGetRelationName(onerel)); + errinfo.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */ /* Setup error traceback support for ereport() */ errcallback.callback = vacuum_error_callback; - errcallback.arg = &vacrelstats.errinfo; + errcallback.arg = &errinfo; errcallback.previous = error_context_stack; error_context_stack = &errcallback; @@ -3544,7 +3544,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) /* Process indexes to perform vacuum/cleanup */ parallel_vacuum_index(indrels, stats, lvshared, dead_tuples, nindexes, - &vacrelstats); + &errinfo); /* Report buffer/WAL usage during parallel execution */ buffer_usage = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, false); -- 2.17.0 --ALfTUftag+2gvp1h-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Replace known_assigned_xids_lck by memory barrier @ 2023-08-16 19:29 Michail Nikolaev <[email protected]> 2023-08-16 20:07 ` Re: Replace known_assigned_xids_lck by memory barrier Nathan Bossart <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Michail Nikolaev @ 2023-08-16 19:29 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]> Hello, good question! Thanks for your edits. As answer: probably we need to change "If we know that we're holding ProcArrayLock exclusively, we don't need the read barrier." to "If we're removing xid, we don't need the read barrier because only the startup process can remove and add xids to KnownAssignedXids" Best regards, Mikhail. ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Replace known_assigned_xids_lck by memory barrier 2023-08-16 19:29 Re: Replace known_assigned_xids_lck by memory barrier Michail Nikolaev <[email protected]> @ 2023-08-16 20:07 ` Nathan Bossart <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Nathan Bossart @ 2023-08-16 20:07 UTC (permalink / raw) To: Michail Nikolaev <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]> On Wed, Aug 16, 2023 at 09:29:10PM +0200, Michail Nikolaev wrote: > As answer: probably we need to change > "If we know that we're holding ProcArrayLock exclusively, we don't > need the read barrier." > to > "If we're removing xid, we don't need the read barrier because only > the startup process can remove and add xids to KnownAssignedXids" Ah, that explains it. v5 of the patch is attached. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v5-0001-Replace-known_assigned_xids_lck-with-memory-barri.patch (5.9K, ../../20230816200715.GB2908487@nathanxps13/2-v5-0001-Replace-known_assigned_xids_lck-with-memory-barri.patch) download | inline diff: From 6038182ac96226f0eec43b1c6ab2e060e8b1e3a8 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 16 Aug 2023 10:52:56 -0700 Subject: [PATCH v5 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 | 72 +++++++++++------------------ 1 file changed, 26 insertions(+), 46 deletions(-) diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 2a3da49b8f..401e816275 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,11 @@ 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 when reading or writing the head/tail pointers (unless + * the caller holds ProcArrayLock exclusively). * * Algorithmic analysis: * @@ -4806,22 +4800,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 +4830,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; + + /* + * Only the startup process removes entries, so we don't need the read + * barrier in that case. + */ + if (!remove) + pg_read_barrier(); /* pairs with KnownAssignedXidsAdd */ /* * Standard binary search. Note we can ignore the KnownAssignedXidsValid @@ -5094,13 +5076,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 +5127,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] 4+ messages in thread
end of thread, other threads:[~2023-08-16 20:07 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-06-22 22:13 [PATCH 4/4] Update functions to pass only errinfo struct.. Justin Pryzby <[email protected]> 2020-06-22 22:13 [PATCH v2 5/5] Update functions to pass only errinfo struct.. Justin Pryzby <[email protected]> 2023-08-16 19:29 Re: Replace known_assigned_xids_lck by memory barrier Michail Nikolaev <[email protected]> 2023-08-16 20:07 ` 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