From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Melanie Plageman <melanieplageman(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de> |
Subject: | Re: Fix some inconsistencies with open-coded visibilitymap_set() callers |
Date: | 2025-07-10 14:35:48 |
Message-ID: | CA+TgmobpW7ROye9R66kqueRos2emHG68W5z=CcN15nxwdCjM6A@mail.gmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Jul 10, 2025 at 9:57 AM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
> A direct translation of this would be to add a boolean parameter to
> visibilitymap_set() like "heap_page_modified" or "set_heap_lsn". Is
> that along the lines you were thinking?
Yeah, something like that. I haven't thought through the details.
> > If it's useful to combine things as a precursor to future work, that
> > may be fair enough, but otherwise I don't see the point. It's not
> > "technically fine;" it's just straight-up fine. It's not desirable, in
> > the sense that we might end up having to do extra page reads later to
> > correct the situation, but crashes won't intervene between those two
> > critical sections often enough to matter. Unifying those critical
> > sections won't change anything about what states code elsewhere in the
> > tree must be prepared to see on disk.
>
> I think one argument for having it in the same critical section is the
> defensive coding perspective. Our rule is that you make changes to the
> heap page, mark it dirty, emit WAL, and then stamp it with that LSN
> all in the same critical section.
I agree that we need to follow this rule.
> In the case of PD_ALL_VISIBLE, it is okay to break this rule because
> it is okay to lose the PD_ALL_VISIBLE hint as long as the VM bit
> doesn't get set. But, future programmers could see this and make other
> modifications to the heap page in the same area we set PD_ALL_VISIBLE
> (the one outside of a critical section).
But this is saying that PD_ALL_VISIBLE isn't really covered by the WAL
record in question -- instead, it's a related hint. Or really
semi-hint, since it's only conditionally OK to lose it. So the rule
above doesn't necessarily fully apply to this situation.
> Anyway, I'm now convinced that the way forward for this particular
> issue is to represent the VM changes in the same WAL record that has
> the heap page changes, so I won't further belabor the issue.
Makes sense.
--
Robert Haas
EDB: http://www.enterprisedb.com
From | Date | Subject | |
---|---|---|---|
Next Message | ivan.kush | 2025-07-10 14:41:07 | Re: [PoC] Federated Authn/z with OAUTHBEARER |
Previous Message | Mihail Nikalayeu | 2025-07-10 14:30:15 | Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements |