public inbox for [email protected]
help / color / mirror / Atom feedFrom: Robert Haas <[email protected]>
To: Melanie Plageman <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: Andrey Borodin <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access)
Date: Mon, 8 Sep 2025 16:14:47 -0400
Message-ID: <CA+TgmoasgmY7mzZutGisD2=3y7BwwPUS=oNsQoORKRg1r69fEA@mail.gmail.com> (raw)
In-Reply-To: <CAAKRu_Y=QZ5iD7zt1AHcG3_G_iMR0w6ApGPgr8FKcDn-YLFvuQ@mail.gmail.com>
References: <[email protected]>
<CAAKRu_a-aVGxNEdkJt+96HGryQXuQNuXe+PhR0KcnUhXSOPBJw@mail.gmail.com>
<[email protected]>
<CAAKRu_ZH8kL0Zm0j7m7DC9fzk7ru7yf9rm2pEQRvx1iXX25aPQ@mail.gmail.com>
<CAAKRu_bGCgUuhmy1Mzkw3yCmbGcjNQAjV=OmjuW6hz90PuXKWA@mail.gmail.com>
<CALdSSPhAU56g1gGVT0+wG8RrSWE6qW8TOfNJS1HNAWX6wPgbFA@mail.gmail.com>
<CAAKRu_YD0ecXeAh+DmJpzQOJwcRzmMyGdcc5W_0pEF78rYSJkQ@mail.gmail.com>
<CALdSSPhu7WZd+EfQDha1nz=DC93OtY1=UFEdWwSZsASka_2eRQ@mail.gmail.com>
<CAAKRu_a2zU7672weJCGzAE2K44cCwnvsb-BwPh8ET3n1bsKfPQ@mail.gmail.com>
<CAAKRu_Yc1VKM+iuKuJzncPXCYNqQz_jUFBYXuDiPC5k9sUiiQQ@mail.gmail.com>
<tvvtfoxz5ykpsctxjbzxg3nldnzfc7geplrt2z2s54pmgto27y@hbijsndifu45>
<CAAKRu_Yz9x0sejBa5ov_LJ5sMOSKM3AeKOFUg+fQpNqyMmxwRA@mail.gmail.com>
<CAAKRu_Y=QZ5iD7zt1AHcG3_G_iMR0w6ApGPgr8FKcDn-YLFvuQ@mail.gmail.com>
Reviewing 0003:
+ /*
+ * If we're only adding already frozen rows to a
previously empty
+ * page, mark it as all-frozen and update the
visibility map. We're
+ * already holding a pin on the vmbuffer.
+ */
else if (all_frozen_set)
+ {
PageSetAllVisible(page);
+ LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE);
+ visibilitymap_set_vmbits(relation,
+
BufferGetBlockNumber(buffer),
+
vmbuffer,
+
VISIBILITYMAP_ALL_VISIBLE |
+
VISIBILITYMAP_ALL_FROZEN);
Locking a buffer in a critical section violates the order of
operations proposed in the 'Write-Ahead Log Coding' section of
src/backend/access/transam/README.
+ * Now read and update the VM block. Even if we skipped
updating the heap
+ * page due to the file being dropped or truncated later in
recovery, it's
+ * still safe to update the visibility map. Any WAL record that clears
+ * the visibility map bit does so before checking the page LSN, so any
+ * bits that need to be cleared will still be cleared.
+ *
+ * It is only okay to set the VM bits without holding the heap page lock
+ * because we can expect no other writers of this page.
The first paragraph of this paraphrases a similar content in
xlog_heap_visible(), but I don't see the variation in phrasing as an
improvement.
The second paragraph does not convince me at all. I see no reason to
believe that this is safe, or that it is a good idea. The code in
xlog_heap_visible() thinks its OK to unlock and relock the page to
make visibilitymap_set() happy, which is cringy but probably safe for
lack of concurrent writers, but skipping locking altogether seems
deeply unwise.
- * visibilitymap_set - set a bit in a previously pinned page
+ * visibilitymap_set - set bit(s) in a previously
pinned page and log
+ * visibilitymap_set_vmbits - set bit(s) in a pinned page
I suspect the indentation was done with a different mix of spaces and
tabs here, because this doesn't align for me.
In general, this idea makes some sense to me -- there doesn't seem to
be any particularly good reason why the visibility-map update should
be handled by a different WAL record than the all-visible flag on the
page itself. It's a little hard for me to make that statement too
conclusively without studying more of the patches than I've had time
to do today, but off the top of my head it seems to make sense.
However, I'm not sure you've taken enough care with the details here.
--
Robert Haas
EDB: http://www.enterprisedb.com
view thread (143+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access)
In-Reply-To: <CA+TgmoasgmY7mzZutGisD2=3y7BwwPUS=oNsQoORKRg1r69fEA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox