Fix data checksum processing for temp relations and dropped databases
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:
v1-0001-Fix-data-checksum-processing-for-temp-relations-a.patchapplication/octet-stream; name=v1-0001-Fix-data-checksum-processing-for-temp-relations-a.patchDownload+14-9
Hello,
At Thu, 9 Jul 2026 11:24:31 +0900, Fujii Masao <masao.fujii@gmail.com> 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
On 9 Jul 2026, at 09:15, Kyotaro Horiguchi <horikyota.ntt@gmail.com> 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
On Thu, Jul 9, 2026 at 4:56 PM Daniel Gustafsson <daniel@yesql.se> wrote:
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.
Thanks for the review! I've updated the patch based on
your and Horiguchi-san's suggestions.
Regards,
--
Fujii Masao
Attachments:
v2-0001-Fix-data-checksum-processing-for-temp-relations-a.patchapplication/octet-stream; name=v2-0001-Fix-data-checksum-processing-for-temp-relations-a.patchDownload+16-9
On 10 Jul 2026, at 02:12, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Jul 9, 2026 at 4:56 PM Daniel Gustafsson <daniel@yesql.se> wrote:
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.Thanks for the review! I've updated the patch based on
your and Horiguchi-san's suggestions.
+1, v2 LGTM.
--
Daniel Gustafsson
On Fri, Jul 10, 2026 at 5:32 PM Daniel Gustafsson <daniel@yesql.se> wrote:
Thanks for the review! I've updated the patch based on
your and Horiguchi-san's suggestions.+1, v2 LGTM.
Thanks for the review! I've pushed the patch.
Regards,
--
Fujii Masao