public inbox for [email protected]help / color / mirror / Atom feed
Fix data checksum processing for temp relations and dropped databases 3+ messages / 3 participants [nested] [flat]
* Fix data checksum processing for temp relations and dropped databases @ 2026-07-09 02:24 Fujii Masao <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Fujii Masao @ 2026-07-09 02:24 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hi, While reading the data checksum code, I found that BuildRelationList() filters out non-temporary relations without storage, but does not apply the same filtering to temporary relations. As a result, temporary relations without storage, such as temporary views, are included in the list of relations to wait for even though they are irrelevant to checksum processing. For example, enabling data checksums can unnecessarily wait for a long-lived session that owns only a temporary view. I think BuildRelationList() should exclude temporary relations without RELKIND_HAS_STORAGE(), matching the existing behavior for non-temporary relations. Attached patch implements this. Also there seems a small race condition. When enabling data checksums online, the launcher assigns the first worker to process the shared catalogs and prevents later workers from doing so. However, if that worker's database is dropped after it has been selected for processing but before checksum processing begins (a very small window), the worker fails without processing the shared catalogs, yet they are still marked as processed. As a result, later workers skip them, and checksum enabling can complete successfully even though the shared catalogs were never processed. To fix this, we should mark the shared catalogs as processed only after a worker completes successfully. Attached patch also implements this. Thoughts? Regards, -- Fujii Masao Attachments: [application/octet-stream] v1-0001-Fix-data-checksum-processing-for-temp-relations-a.patch (3.4K, ../../CAHGQGwGDHAQw=bmpRzk+EmKzVtxZiD5YDurMUffBMwr6WXugQA@mail.gmail.com/2-v1-0001-Fix-data-checksum-processing-for-temp-relations-a.patch) download | inline diff: From 6095cc49dce11e88840ed0c94071b124a1114461 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Wed, 8 Jul 2026 23:52:39 +0900 Subject: [PATCH v1] Fix data checksum processing for temp relations and dropped databases When building the list of temporary relations to wait for, the code previously included temporary relations without storage, such as temporary views, even though they are irrelevant to checksum processing. As a result, enabling data checksums could wait for a long-lived session that owned only a temporary view. This commit fixes the issue by filtering temporary relations with storage only, matching the existing behavior for non-temporary relations. Also, when enabling data checksums online, the launcher assigns the first worker to process shared catalogs and prevents later workers from doing so. Previously, if that worker's database was dropped after it had been selected for processing but before checksum processing began, the worker failed without processing the shared catalogs, yet they were still marked as processed. As a result, later workers skipped them, and checksum enabling could complete successfully even though the shared catalogs had never been processed. This commit fixes the issue by marking shared catalogs as processed only after a worker completes successfully. --- src/backend/postmaster/datachecksum_state.c | 22 +++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/backend/postmaster/datachecksum_state.c b/src/backend/postmaster/datachecksum_state.c index 68557c16cb9..2b3e1df6cf1 100644 --- a/src/backend/postmaster/datachecksum_state.c +++ b/src/backend/postmaster/datachecksum_state.c @@ -1329,9 +1329,12 @@ ProcessAllDatabases(void) * When one database has completed, it will have done shared catalogs * so we don't have to process them again. */ - LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE); - DataChecksumState->process_shared_catalogs = false; - LWLockRelease(DataChecksumsWorkerLock); + if (result == DATACHECKSUMSWORKER_SUCCESSFUL) + { + LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE); + DataChecksumState->process_shared_catalogs = false; + LWLockRelease(DataChecksumsWorkerLock); + } } FreeDatabaseList(DatabaseList); @@ -1469,11 +1472,11 @@ FreeDatabaseList(List *dblist) * Compile a list of relations in the database * * Returns a list of OIDs for the requested relation types. If temp_relations - * is True then only temporary relations are returned. If temp_relations is - * False then non-temporary relations which have data checksums are returned. - * If include_shared is True then shared relations are included as well in a - * non-temporary list. include_shared has no relevance when building a list of - * temporary relations. + * is True then only temporary relations with storage are returned. If + * temp_relations is False then non-temporary relations with storage are + * returned. If include_shared is True then shared relations are included as + * well in a non-temporary list. include_shared has no relevance when building + * a list of temporary relations. */ static List * BuildRelationList(bool temp_relations, bool include_shared) @@ -1499,6 +1502,9 @@ BuildRelationList(bool temp_relations, bool include_shared) { if (!temp_relations) continue; + + if (!RELKIND_HAS_STORAGE(pgc->relkind)) + continue; } else { -- 2.55.0 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Fix data checksum processing for temp relations and dropped databases @ 2026-07-09 07:15 Kyotaro Horiguchi <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Kyotaro Horiguchi @ 2026-07-09 07:15 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] Hello, At Thu, 9 Jul 2026 11:24:31 +0900, Fujii Masao <[email protected]> wrote in > I think BuildRelationList() should exclude temporary relations without > RELKIND_HAS_STORAGE(), matching the existing behavior for > non-temporary relations. Attached patch implements this. ... > fails without processing the shared catalogs, yet they are still marked > as processed. As a result, later workers skip them, and checksum > enabling can complete successfully even though the shared catalogs > were never processed. > > To fix this, we should mark the shared catalogs as processed only after > a worker completes successfully. Attached patch also implements this. > > Thoughts? The changes look functionally correct to me. A couple of minor comments: 1. In BuildRelationList(), if relations without storage should be excluded regardless of whether they are temporary or non-temporary, I would personally prefer moving the RELKIND_HAS_STORAGE() check before the persistence check. That would make the intent a bit clearer. 2. In ProcessAllDatabases(), I find it slightly easier to read if the dropped-database case is handled explicitly first, perhaps by continuing early, so that DATACHECKSUMSWORKER_SUCCESSFUL remains the default result for the normal path. I'm not completely sure whether that check should go just before or just after pgstat_progress_update_param(), though. Both are mostly matters of style, so please feel free to ignore them if you prefer the current structure. Regards, -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Fix data checksum processing for temp relations and dropped databases @ 2026-07-09 07:56 Daniel Gustafsson <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Daniel Gustafsson @ 2026-07-09 07:56 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: Fujii Masao <[email protected]>; [email protected] > On 9 Jul 2026, at 09:15, Kyotaro Horiguchi <[email protected]> wrote: > The changes look functionally correct to me. Agreed. > 1. In BuildRelationList(), if relations without storage should be > excluded regardless of whether they are temporary or non-temporary, I > would personally prefer moving the RELKIND_HAS_STORAGE() check before > the persistence check. That would make the intent a bit clearer. Makes sense, though I am fine with either approach. > 2. In ProcessAllDatabases(), I find it slightly easier to read if the > dropped-database case is handled explicitly first, perhaps by > continuing early, so that DATACHECKSUMSWORKER_SUCCESSFUL remains the > default result for the normal path. I'm not completely sure whether > that check should go just before or just after > pgstat_progress_update_param(), though. I like this suggestion, though I think it should go after the call to pgstat_progress_update_param since the db in question reached ProcessDatabase. I would add it as an else-if case to the conditional testing we have for _FAILED and _ABORTED. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-07-09 07:56 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-07-09 02:24 Fix data checksum processing for temp relations and dropped databases Fujii Masao <[email protected]> 2026-07-09 07:15 ` Kyotaro Horiguchi <[email protected]> 2026-07-09 07:56 ` Daniel Gustafsson <[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