Re: Incorrect result of bitmap heap scan.

2025-04-05 Thread Andres Freund
Hi, On 2025-04-02 18:58:11 +0200, Matthias van de Meent wrote: > And here it is, v6 of the patchset, rebased up to master @ 764d501d. Thanks! Does anybody have an opinion about how non-invasive to be in the back-branches? The minimal version is something like this diff: diff --git i/src/backend

Re: Incorrect result of bitmap heap scan.

2025-04-05 Thread Matthias van de Meent
On Wed, 2 Apr 2025 at 17:37, Andres Freund wrote: > > Hi, > > Matthias, any chance you can provide a rebased version for master? Sure, I'll try to have it in your inbox later today CEST. > Either way I'm planning to push this fairly soon. OK, thanks! Kind regards, Matthias van de Meent Neon (

Re: Incorrect result of bitmap heap scan.

2025-04-04 Thread Andres Freund
Hi, Matthias, any chance you can provide a rebased version for master? If not, I'll do the rebase. Either way I'm planning to push this fairly soon. Greetings, Andres Freund

Re: Incorrect result of bitmap heap scan.

2025-04-02 Thread Tom Lane
Andres Freund writes: > Does anybody have an opinion about how non-invasive to be in the > back-branches? The minimal version is something like this diff: Minimal is good -- less chance of breaking anything. > - Should we commit the test showing that the naive implementation of > index-only-bi

Re: Incorrect result of bitmap heap scan.

2025-04-02 Thread Andres Freund
Hi, On 2025-04-02 14:17:01 -0400, Tom Lane wrote: > Andres Freund writes: > > Does anybody have an opinion about how non-invasive to be in the > > back-branches? The minimal version is something like this diff: > > Minimal is good -- less chance of breaking anything. > > > - Should we commit th

Re: Incorrect result of bitmap heap scan.

2025-04-02 Thread Matthias van de Meent
On Wed, 2 Apr 2025 at 19:48, Andres Freund wrote: > > Hi, > > On 2025-04-02 18:58:11 +0200, Matthias van de Meent wrote: > > And here it is, v6 of the patchset, rebased up to master @ 764d501d. > > Thanks! > > Does anybody have an opinion about how non-invasive to be in the > back-branches? The mi

Re: Incorrect result of bitmap heap scan.

2025-04-02 Thread Matthias van de Meent
On Wed, 2 Apr 2025 at 18:20, Matthias van de Meent wrote: > > On Wed, 2 Apr 2025 at 17:37, Andres Freund wrote: > > > > Hi, > > > > Matthias, any chance you can provide a rebased version for master? > > Sure, I'll try to have it in your inbox later today CEST. And here it is, v6 of the patchset,

Re: Incorrect result of bitmap heap scan.

2025-03-17 Thread Andres Freund
Hi, On 2025-03-17 16:17:03 +0100, Matthias van de Meent wrote: > As I understand it, Andres agrees that the feature is unsalvageable in > backbranches ("don't think there's a realistic way to fix it"). Andres > then continues to elaborate that even if the feature were salvageable, > it wouldn't co

Re: Incorrect result of bitmap heap scan.

2025-03-17 Thread Matthias van de Meent
On Sun, 16 Mar 2025 at 13:55, vignesh C wrote: > > On Wed, 5 Mar 2025 at 16:43, Matthias van de Meent > wrote: > > > > On Sun, 2 Mar 2025 at 01:35, Tom Lane wrote: > > > > > > Peter Geoghegan writes: > > > > Is everybody in agreement about committing and back patching this fix, > > > > which si

Re: Incorrect result of bitmap heap scan.

2025-03-16 Thread vignesh C
On Wed, 5 Mar 2025 at 16:43, Matthias van de Meent wrote: > > On Sun, 2 Mar 2025 at 01:35, Tom Lane wrote: > > > > Peter Geoghegan writes: > > > Is everybody in agreement about committing and back patching this fix, > > > which simply disables the optimization altogether? > > > I myself don't se

Re: Incorrect result of bitmap heap scan.

2025-03-05 Thread Andres Freund
Hi, On 2025-03-01 19:35:15 -0500, Tom Lane wrote: > Peter Geoghegan writes: > > Is everybody in agreement about committing and back patching this fix, > > which simply disables the optimization altogether? > > I myself don't see a better way, but thought I'd ask before proceeding > > with review

Re: Incorrect result of bitmap heap scan.

2025-03-05 Thread Matthias van de Meent
On Sun, 2 Mar 2025 at 01:35, Tom Lane wrote: > > Peter Geoghegan writes: > > Is everybody in agreement about committing and back patching this fix, > > which simply disables the optimization altogether? > > I myself don't see a better way, but thought I'd ask before proceeding > > with review and

Re: Incorrect result of bitmap heap scan.

2025-03-01 Thread Tom Lane
Peter Geoghegan writes: > Is everybody in agreement about committing and back patching this fix, > which simply disables the optimization altogether? > I myself don't see a better way, but thought I'd ask before proceeding > with review and commit. If you don't see a clear path forward, then "dis

Re: Incorrect result of bitmap heap scan.

2025-03-01 Thread Peter Geoghegan
On Fri, Feb 28, 2025 at 12:53 PM Matthias van de Meent wrote: > Rebased again, now on top of head due to f7a8fc10. Is everybody in agreement about committing and back patching this fix, which simply disables the optimization altogether? I myself don't see a better way, but thought I'd ask before

Re: Incorrect result of bitmap heap scan.

2025-02-28 Thread Matthias van de Meent
On Tue, 7 Jan 2025 at 18:46, Matthias van de Meent wrote: > > Hi, > > I've rebased my earlier patch to fix some minor conflicts with the > work done on bitmaps in December last year. I've also included Andres' > cursor-based isolation test as 0002; which now passes. Rebased again, now on top of h

Re: Incorrect result of bitmap heap scan.

2025-01-07 Thread Matthias van de Meent
Hi, I've rebased my earlier patch to fix some minor conflicts with the work done on bitmaps in December last year. I've also included Andres' cursor-based isolation test as 0002; which now passes. This should take care of cfbot's misidentification of which patch to test, and thus get CFBot to suc

Re: Incorrect result of bitmap heap scan.

2024-12-04 Thread Matthias van de Meent
On Mon, 2 Dec 2024 at 17:31, Andres Freund wrote: > > Hi, > > On 2024-12-02 16:08:02 +0100, Matthias van de Meent wrote: > > Concurrency timeline: > > > > Session 1. amgetbitmap() gets snapshot of index contents, containing > > references to dead tuples on heap P1. > > IIUC, an unstanted important

Re: Incorrect result of bitmap heap scan.

2024-12-04 Thread Matthias van de Meent
On Mon, 2 Dec 2024 at 18:02, Tom Lane wrote: > > Andres Freund writes: > > I think the problematic scenario involves tuples that *nobody* can see. > > During > > the bitmap index scan we don't know that though. Thus the tid gets inserted > > into the bitmap. Then, before we visit the heap, a con

Re: Incorrect result of bitmap heap scan.

2024-12-03 Thread Matthias van de Meent
On Mon, 2 Dec 2024 at 15:25, Konstantin Knizhnik wrote: > > Hi hackers, > > Attached script reproduces the problem with incorrect results of `select > count(*)` (it returns larger number of records than really available in the > table). > It is not always reproduced, so you may need to repeat it

Re: Incorrect result of bitmap heap scan.

2024-12-03 Thread Tom Lane
Peter Geoghegan writes: > On Mon, Dec 2, 2024 at 10:15 AM Matthias van de Meent > wrote: >> The running theory is that bitmap executor nodes incorrectly assume >> that the rows contained in the bitmap all are still present in the >> index, and thus assume they're allowed to only check the visibil

Re: Incorrect result of bitmap heap scan.

2024-12-03 Thread Andres Freund
Hi, On 2024-12-02 11:07:38 -0500, Peter Geoghegan wrote: > Clearly it would be wildly impractical to do the "buffer pin interlock > against TID recycling" thing in the context of bitmap scans. That's certainly true if we do the VM check at the time of the bitmap heap scan. What if we did it when

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 3:56 PM Peter Geoghegan wrote: > I took what you wrote, and repurposed it to prove my old theory about > GiST index-only scans being broken due to the lack of an appropriate > interlock against concurrent TID recycling. See the attached patch. I've moved discussion of this

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 12:15 PM Peter Geoghegan wrote: > The underlying reason why nbtree can discriminate like this is that it > "knows" that plain index scans will always visit the heap proper. If a > TID points to an LP_UNUSED item, then it is considered dead to the > scan (even though in gener

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Tom Lane
Andres Freund writes: > I think the problematic scenario involves tuples that *nobody* can see. During > the bitmap index scan we don't know that though. Thus the tid gets inserted > into the bitmap. Then, before we visit the heap, a concurrent vacuum removes > the tuple from the indexes and then

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 12:02 PM Tom Lane wrote: > Andres Freund writes: > > I think the problematic scenario involves tuples that *nobody* can see. > > During > > the bitmap index scan we don't know that though. Thus the tid gets inserted > > into the bitmap. Then, before we visit the heap, a co

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Tom Lane
Peter Geoghegan writes: > On Mon, Dec 2, 2024 at 12:02 PM Tom Lane wrote: >> Yup. I am saying that that qualifies as too-aggressive setting of the >> all-visible bit. I'm not sure what rule we should adopt instead of >> the current one, but I'd much rather slow down page freezing than >> instit

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 11:32 AM Andres Freund wrote: > On 2024-12-02 16:08:02 +0100, Matthias van de Meent wrote: > > Concurrency timeline: > > > > Session 1. amgetbitmap() gets snapshot of index contents, containing > > references to dead tuples on heap P1. > > IIUC, an unstanted important aspect

Incorrect result of bitmap heap scan.

2024-12-02 Thread Konstantin Knizhnik
Hi hackers, Attached script reproduces the problem with incorrect results of `select count(*)` (it returns larger number of records than really available in the table). It is not always reproduced, so you may need to repeat it multiple times - at my system it failed 3 times from 10. The prob

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Andres Freund
Hi, On 2024-12-02 11:43:42 -0500, Tom Lane wrote: > Peter Geoghegan writes: > > This theory seems very believable. > > I'm not convinced. I think there are two assumptions underlying > bitmap scan: > > 1. Regardless of index contents, it's not okay for vacuum to remove > tuples that an open tr

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 3:55 PM Andres Freund wrote: > I suspect one contributor to this avoiding attention till now was that the > optimization is fairly narrow: > > /* > * We can potentially skip fetching heap pages if we > do not need >

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 3:56 PM Peter Geoghegan wrote: > I took what you wrote, and repurposed it to prove my old theory about > GiST index-only scans being broken due to the lack of an appropriate > interlock against concurrent TID recycling. See the attached patch. BTW, if you change the test ca

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 2:43 PM Andres Freund wrote: > Attached is an isolationtest that reliably shows wrong query results. Nice approach with the cursor! I took what you wrote, and repurposed it to prove my old theory about GiST index-only scans being broken due to the lack of an appropriate in

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Andres Freund
Hi, On 2024-12-02 13:39:43 -0500, Peter Geoghegan wrote: > I guess it's natural to suspect more recent work -- commit 7c70996e is > about 6 years old. But I the race condition that I suspect is at play > here is very narrow. FWIW, the test I just posted shows the issue down to 11 (although for 11

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 11:52 AM Andres Freund wrote: > I think the problematic scenario involves tuples that *nobody* can see. During > the bitmap index scan we don't know that though. Right, exactly. FWIW, this same issue is why it is safe for nbtree to drop its pin early during plain index sca

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Andres Freund
Hi, On 2024-12-02 12:02:39 -0500, Tom Lane wrote: > Andres Freund writes: > > I think the problematic scenario involves tuples that *nobody* can see. > > During > > the bitmap index scan we don't know that though. Thus the tid gets inserted > > into the bitmap. Then, before we visit the heap, a

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 11:43 AM Tom Lane wrote: > Whether that's true or not, it seems like it'd be worth bisecting > to see if we can finger a commit where the behavior changed (and > the same goes for the question of why-isnt-it-an-IOS-scan). However, > the reproducer seems to have quite a low

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Andres Freund
Hi, On 2024-12-02 11:31:48 -0500, Andres Freund wrote: > I think it'd be good if we added a test that shows the failure mechanism so > that we don't re-introduce this in the future. Evidently this failure isn't > immediately obvious... Attached is an isolationtest that reliably shows wrong query

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Andres Freund
Hi, On 2024-12-02 16:08:02 +0100, Matthias van de Meent wrote: > Concurrency timeline: > > Session 1. amgetbitmap() gets snapshot of index contents, containing > references to dead tuples on heap P1. IIUC, an unstanted important aspect here is that these dead tuples are *not* visible to S1. Othe

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 12:11 PM Tom Lane wrote: > > Freezing a page, and setting a page all-visible are orthogonal. > > Sorry, sloppy wording on my part. Freezing doesn't affect the contents of the visibility map in any way that seems relevant. The executor only cares about the all-visible bit (a

Re: Incorrect result of bitmap heap scan.

2024-12-02 Thread Peter Geoghegan
On Mon, Dec 2, 2024 at 10:15 AM Matthias van de Meent wrote: > The running theory is that bitmap executor nodes incorrectly assume > that the rows contained in the bitmap all are still present in the > index, and thus assume they're allowed to only check the visibility > map to see if the referenc