From: | Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
Subject: | Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits |
Date: | 2019-04-05 03:55:18 |
Message-ID: | CABOikdPXd+EGuUSF5h_RAcPrBnwQPA3YKLiu--_8b4is-92naQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Apr 5, 2019 at 8:37 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> Hi,
>
> On 2019-04-05 00:06:04 -0300, Alvaro Herrera wrote:
>
> >
> > Hmm, isn't there already a critical section in visibilitymap_set itself?
>
> There is, but the proposed code sets all visible on the page, and marks
> the buffer dirty, before calling visibilitymap_set.
>
How's it any different than what we're doing at vacuumlazy.c:1322? We set
the page-level bit, mark the buffer dirty and then call
visibilitymap_set(), all outside a critical section.
1300 /* mark page all-visible, if appropriate */
1301 if (all_visible && !all_visible_according_to_vm)
1302 {
1303 uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
1304
1305 if (all_frozen)
1306 flags |= VISIBILITYMAP_ALL_FROZEN;
1307
1308 /*
1309 * It should never be the case that the visibility map
page is set
1310 * while the page-level bit is clear, but the reverse is
allowed
1311 * (if checksums are not enabled). Regardless, set the
both bits
1312 * so that we get back in sync.
1313 *
1314 * NB: If the heap page is all-visible but the VM bit is
not set,
1315 * we don't need to dirty the heap page. However, if
checksums
1316 * are enabled, we do need to make sure that the heap page
is
1317 * dirtied before passing it to visibilitymap_set(),
because it
1318 * may be logged. Given that this situation should only
happen in
1319 * rare cases after a crash, it is not worth optimizing.
1320 */
1321 PageSetAllVisible(page);
1322 MarkBufferDirty(buf);
1323 visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
1324 vmbuffer, visibility_cutoff_xid, flags);
1325 }
As the first para in that comment says, I thought it's ok for page-level
bit to be set but the visibility bit to be clear, but not the vice versa.
The proposed code does not introduce any new behaviour AFAICS. But I might
be missing something.
Thanks,
Pavan
--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2019-04-05 03:55:20 | Re: fix the spelling mistakes of comments |
Previous Message | Pavan Deolasee | 2019-04-05 03:50:36 | Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits |