On Wed, Apr 16, 2025 at 12:31 AM Thomas Munro wrote:
> More or less, yeah, just put the whole ReadStream object in shared
> memory, pin an LWLock on it and call it a parallel-aware or shared
> ReadStream. But how do you make the locking not terrible?
>
> My "work stealing" brain dump was imaginin
On Tue, Apr 15, 2025 at 5:44 AM Robert Haas wrote:
> On Thu, Apr 10, 2025 at 11:15 PM Thomas Munro wrote:
> > The new streaming BHS isn't just issuing probabilistic hints about
> > future access obtained from a second iterator. It has just one shared
> > iterator connected up to the workers' Rea
Thanks for the comments!
On Tue, Apr 15, 2025 at 3:11 AM Andres Freund wrote:
>
> Hi,
>
> On 2025-04-14 09:58:19 -0700, James Hunter wrote:
> > I see two orthogonal problems, in processing Bitmap Heap pages in
> > parallel: (1) we need to prefetch enough pages, far enough in advance,
> > to hide
Hi,
On 2025-04-14 09:58:19 -0700, James Hunter wrote:
> Of course, late feedback is a burden, but I think our discussion here
> (and, in the future, if you try to "ReadStream" BTree index pages,
> themselves) illustrates my point.
FWIW, it's quite conceivable that we'll want to use non-readstream
On Thu, Apr 10, 2025 at 11:15 PM Thomas Munro wrote:
> The new streaming BHS isn't just issuing probabilistic hints about
> future access obtained from a second iterator. It has just one shared
> iterator connected up to the workers' ReadStreams. Each worker pulls
> a disjoint set of blocks out
On Thu, Apr 10, 2025 at 8:15 PM Thomas Munro wrote:
>
> On Fri, Apr 11, 2025 at 5:50 AM James Hunter
> wrote:
> > I am looking at the pre-streaming code, in PG 17, as I am not familiar
> > with the PG 18 "streaming" code. Back in PG 17, nodeBitmapHeapscan.c
> > maintained two shared TBM iterator
On Fri, Apr 11, 2025 at 5:50 AM James Hunter wrote:
> I am looking at the pre-streaming code, in PG 17, as I am not familiar
> with the PG 18 "streaming" code. Back in PG 17, nodeBitmapHeapscan.c
> maintained two shared TBM iterators, for PQ. One of the iterators was
> the actual, "fetch" iterator
On Wed, Apr 9, 2025 at 11:00 PM Thomas Munro wrote:
>
> On Wed, Apr 9, 2025 at 1:46 PM James Hunter wrote:
> > On Mon, Apr 7, 2025 at 7:34 PM Thomas Munro wrote:
> > > On Thu, Feb 13, 2025 at 1:40 PM Melanie Plageman
> > > wrote:
> > > > Thomas mentioned this to me off-list, and I think he's ri
On Wed, Apr 9, 2025 at 1:46 PM James Hunter wrote:
> On Mon, Apr 7, 2025 at 7:34 PM Thomas Munro wrote:
> > On Thu, Feb 13, 2025 at 1:40 PM Melanie Plageman
> > wrote:
> > > Thomas mentioned this to me off-list, and I think he's right. We
> > > likely need to rethink the way parallel bitmap heap
On Mon, Apr 7, 2025 at 7:34 PM Thomas Munro wrote:
>
> On Thu, Feb 13, 2025 at 1:40 PM Melanie Plageman
> wrote:
> > Thomas mentioned this to me off-list, and I think he's right. We
> > likely need to rethink the way parallel bitmap heap scan workers get
> > block assignments for reading and pref
On Thu, Feb 13, 2025 at 1:40 PM Melanie Plageman
wrote:
> Thomas mentioned this to me off-list, and I think he's right. We
> likely need to rethink the way parallel bitmap heap scan workers get
> block assignments for reading and prefetching to make it more similar
> to parallel sequential scan. T
On Mon, Mar 24, 2025 at 12:14 PM Melanie Plageman
wrote:
>
> This is the patch I intend to commit to fix this assuming my CI passes
> and there are no objections.
And pushed in aea916fe555a3
- Melanie
On Sun, Mar 23, 2025 at 1:27 PM Melanie Plageman
wrote:
>
> Perhaps it is better I just fix it since ripping out the skip fetch
> optimization has to be backported and even though that will look very
> different on master than on backbranches, I wonder if people will look
> for a "clean" commit on
On Sat, Mar 22, 2025 at 5:04 PM Andres Freund wrote:
>
> The problem is that sometimes recheck is performed for pending empty
> tuples. The comment about empty tuples says:
>
> /*
> * Bitmap is exhausted. Time to emit empty tuples if relevant. We emit
> * all empty tuples
Hi,
On 2025-03-22 16:14:11 -0400, Andres Freund wrote:
> On 2025-03-22 22:00:00 +0200, Alexander Lakhin wrote:
> > @@ -24,14 +24,14 @@
> > SELECT count(*) FROM bmscantest WHERE a = 1 AND b = 1;
> > count
> > ---
> > - 23
> > + 18
> > (1 row)
>
> Is it possible that this is the bug
Hi,
On 2025-03-22 16:42:42 -0400, Andres Freund wrote:
> Hm, it's clearly related to the all-visible path, but I think the bug is
> actually independent of what was reported there. The bug you reported is
> perfectly reproducible, without any concurrency, after all.
>
> Here's a smaller reproduce
Hi,
On 2025-03-22 22:00:00 +0200, Alexander Lakhin wrote:
> 15.03.2025 16:43, Melanie Plageman wrote:
> > On Thu, Mar 13, 2025 at 5:41 PM Melanie Plageman
> > wrote:
> > > Overall, I feel pretty good about merging this once Thomas merges the
> > > read stream patches.
> > This was committed in 94
Hello Melanie,
15.03.2025 16:43, Melanie Plageman wrote:
On Thu, Mar 13, 2025 at 5:41 PM Melanie Plageman
wrote:
Overall, I feel pretty good about merging this once Thomas merges the
read stream patches.
This was committed in 944e81bf99db2b5b70b, 2b73a8cd33b745c, and
c3953226a07527a1e2.
I've
On Mon, Mar 17, 2025 at 10:46 PM Melanie Plageman
wrote:
>
> On Mon, Mar 17, 2025 at 2:55 PM Andres Freund wrote:
> >
> > On 2025-03-17 14:52:02 -0400, Melanie Plageman wrote:
> > > I don't feel strongly that we need to be as rigorous for
> > > maintenance_io_concurrency, but I'm also not sure 16
On Mon, Mar 17, 2025 at 2:55 PM Andres Freund wrote:
>
> On 2025-03-17 14:52:02 -0400, Melanie Plageman wrote:
> > I don't feel strongly that we need to be as rigorous for
> > maintenance_io_concurrency, but I'm also not sure 160 seems reasonable
> > (which would be the same ratio as before).
>
>
Hi,
On 2025-03-17 14:52:02 -0400, Melanie Plageman wrote:
> I don't feel strongly that we need to be as rigorous for
> maintenance_io_concurrency, but I'm also not sure 160 seems reasonable
> (which would be the same ratio as before).
I'd lean towards just setting them to the same value for now.
On Mon, Mar 17, 2025 at 3:44 AM Jakub Wartak
wrote:
>
> dunno, I've just asked if it isn't suspicious to anyone except me else
> that e_io_c > m_io_c rather than e_io_c <= m_io_c. My understanding
> was always that to tune max IO queue depth you would do this:
> a. up to N backends (up to max_conn
On Thu, Mar 13, 2025 at 9:34 PM Melanie Plageman
wrote:
> On Thu, Mar 13, 2025 at 5:46 AM Jakub Wartak
> wrote:
> >
> > Cool, anything > 1 is just better. Just quick question, so now we have:
> >
> > #define DEFAULT_EFFECTIVE_IO_CONCURRENCY 16
> > #define DEFAULT_MAINTENANCE_IO_CONCURRENCY 10
> >
Hi,
On 2025-03-15 10:43:45 -0400, Melanie Plageman wrote:
> On Thu, Mar 13, 2025 at 5:41 PM Melanie Plageman
> wrote:
> >
> > Overall, I feel pretty good about merging this once Thomas merges the
> > read stream patches.
>
> This was committed in 944e81bf99db2b5b70b, 2b73a8cd33b745c, and
> c3953
On Thu, Mar 13, 2025 at 5:41 PM Melanie Plageman
wrote:
>
> Overall, I feel pretty good about merging this once Thomas merges the
> read stream patches.
This was committed in 944e81bf99db2b5b70b, 2b73a8cd33b745c, and
c3953226a07527a1e2.
I've marked it as committed in the CF app.
As we continue t
On Thu, Mar 13, 2025 at 5:46 AM Jakub Wartak
wrote:
>
> Cool, anything > 1 is just better. Just quick question, so now we have:
>
> #define DEFAULT_EFFECTIVE_IO_CONCURRENCY 16
> #define DEFAULT_MAINTENANCE_IO_CONCURRENCY 10
>
> Shouldn't maintenance be now also at the same value (16) instead of
>
On Wed, Mar 12, 2025 at 9:02 PM Melanie Plageman
wrote:
>
> Thanks for taking a look. I've pushed the patch to increase the
> default effective_io_concurrency.
Cool, anything > 1 is just better. Just quick question, so now we have:
#define DEFAULT_EFFECTIVE_IO_CONCURRENCY 16
#define DEFAULT_MAIN
Thanks for taking a look. I've pushed the patch to increase the
default effective_io_concurrency.
On Tue, Mar 11, 2025 at 8:07 PM Andres Freund wrote:
>
> On 2025-03-10 19:45:38 -0400, Melanie Plageman wrote:
> > From 7b35b1144bddf202fb4d56a9b783751a0945ba0e Mon Sep 17 00:00:00 2001
> > From: Mel
Hi,
On 2025-03-10 19:45:38 -0400, Melanie Plageman wrote:
> From 7b35b1144bddf202fb4d56a9b783751a0945ba0e Mon Sep 17 00:00:00 2001
> From: Melanie Plageman
> Date: Mon, 10 Mar 2025 17:17:38 -0400
> Subject: [PATCH v35 1/5] Increase default effective_io_concurrency to 16
>
> The default effective
On Mon, Mar 10, 2025 at 3:06 PM Melanie Plageman
wrote:
>
> The takeaway is that I think 16 is a good default effectio_io_concurrency
> value.
Attached v35 0003-0005 is the same as v34 but is on top of two new
commits: 0001 increases the default effective_io_concurrency to 16 and
0002 adds a REA
On Mon, Feb 24, 2025 at 11:18 AM Melanie Plageman
wrote:
>
> I changed my mind. I think since the struct I added was only used for
> tbm_extract_page_tuple(), it was a bit weird. I also think it is okay
> for callers to use TBM_MAX_TUPLES_PER_PAGE. I ended up revising this
> to use the same API yo
On Fri, Feb 21, 2025 at 5:00 PM Melanie Plageman
wrote:
>
> I thought about doing this, but in the end I decided against this
> approach. If we wanted to make it easy to palloc arrays of different
> sizes and have tbm_extract_page_tuple extract that many tuples into
> the array, we'd have to chang
Thanks for the review!
While editing this, I found a few things I could improve. For one, I
actually wasn't using the parallel state member that I had added as a
parameter to one of the table AM callbacks. So, I was able to remove
that.
On Thu, Feb 20, 2025 at 7:32 PM Thomas Munro wrote:
>
> On
On Fri, Feb 21, 2025 at 11:17 AM Melanie Plageman
wrote:
> [v31-0001-Delay-extraction-of-TIDBitmap-per-page-offsets.patch]
Nice patch, seems like a straightforward win with the benefits you
explained: less work done under a lock, less work done in the second
iterator if the rest of this stuff doe
On Wed, Feb 19, 2025 at 4:14 PM Melanie Plageman
wrote:
>
> Attached v30 makes the tuple offset extraction happen later as you
> suggested. It turns out that none of the users need to worry much
> about allocating and freeing -- I was able to have all users make an
> offsets array on the stack. Pe
On Wed, Feb 19, 2025 at 8:29 AM Jakub Wartak
wrote:
>
> On Fri, Feb 14, 2025 at 7:16 PM Andres Freund wrote:
>
> > Melanie has worked on this a fair bit, fwiw.
> >
> > My current thinking is that we'd want something very roughly like TCP
> > BBR. Basically, it predicts the currently available ban
On Sun, Feb 16, 2025 at 7:29 AM Tomas Vondra wrote:
>
> On 2/16/25 02:15, Tomas Vondra wrote:
> >
> > ...
> >
> > OK, I've uploaded the results to the github repository as usual
> >
> > https://github.com/tvondra/bitmapscan-tests/tree/main/20250214-184807
> >
> > and I've generated the same PDF
On Wed, Feb 19, 2025 at 6:03 PM Thomas Munro wrote:
>
> On Fri, Feb 14, 2025 at 9:32 AM Andres Freund wrote:
> > I think we'll need to add some logic in read stream that only disables
> > advice
> > after a longer sequential sequence. Writing logic for that shouldn't be too
> > hard, I think? De
On Fri, Feb 14, 2025 at 9:32 AM Andres Freund wrote:
> I think we'll need to add some logic in read stream that only disables advice
> after a longer sequential sequence. Writing logic for that shouldn't be too
> hard, I think? Determining the concrete cutoffs is probably harder, although I
> thin
On Thu, Feb 13, 2025 at 5:51 PM Thomas Munro wrote:
>
> On Fri, Feb 14, 2025 at 5:52 AM Melanie Plageman
> wrote:
> > On Thu, Feb 13, 2025 at 11:28 AM Tomas Vondra wrote:
> > > On 2/13/25 17:01, Melanie Plageman wrote:
> > > I know it's not changing how much memory we allocate (compared to
> > >
On Fri, Feb 14, 2025 at 7:16 PM Andres Freund wrote:
Hi,
> On 2025-02-14 18:36:37 +0100, Tomas Vondra wrote:
> > All of this is true, ofc, but maybe it's better to have a tool providing
> > at least some advice
>
> I agree, a tool like that would be useful!
>
> One difficulty is that the relevan
Hi,
On 2025-02-14 18:36:37 +0100, Tomas Vondra wrote:
> All of this is true, ofc, but maybe it's better to have a tool providing
> at least some advice
I agree, a tool like that would be useful!
One difficulty is that the relevant parameter space is really large, making it
hard to keep the runti
Hi,
On 2025-02-14 18:18:47 +0100, Tomas Vondra wrote:
> FWIW this does not change anything in the detection of sequential access
> patterns, discussed nearby, because the benchmarks started before Andres
> looked into that. If needed, I can easily rerun these tests, I just need
> a patch to apply.
On 2/14/25 18:31, Melanie Plageman wrote:
> io_combine_limit 1, effective_io_concurrency 16, read ahead kb 16
>
> On Fri, Feb 14, 2025 at 12:18 PM Tomas Vondra wrote:
>>
>> Based on off-list discussion with Melanie, I ran a modified version of
>> the benchmark, with these changes:
>
> Thanks! It
On 2/14/25 18:14, Andres Freund wrote:
> Hi,
>
> On 2025-02-14 10:04:41 +0100, Jakub Wartak wrote:
>> Is there any reason we couldn't have new pg_test_iorates (similiar to
>> other pg_test_* proggies), that would literally do this and calibrate
>> best e_io_c during initdb and put the result into
io_combine_limit 1, effective_io_concurrency 16, read ahead kb 16
On Fri, Feb 14, 2025 at 12:18 PM Tomas Vondra wrote:
>
> Based on off-list discussion with Melanie, I ran a modified version of
> the benchmark, with these changes:
Thanks! It looks like the worst offender is io_combine_limit 1 (1
Hi,
On 2025-02-14 10:04:41 +0100, Jakub Wartak wrote:
> Is there any reason we couldn't have new pg_test_iorates (similiar to
> other pg_test_* proggies), that would literally do this and calibrate
> best e_io_c during initdb and put the result into postgresql.auto.conf
> (pg_test_iorates --adjust
On Wed, Feb 12, 2025 at 9:57 PM Robert Haas wrote:
>
> On Wed, Feb 12, 2025 at 3:07 PM Tomas Vondra wrote:
> > AFAICS the "1" value is simply one of the many "defensive" defaults in
> > our sample config. It's much more likely to help than cause harm, even
> > on smaller/older systems, but for ma
On Fri, Feb 14, 2025 at 11:50 AM Thomas Munro wrote:
> Yeah I guess you could in theory also stream pointers to individual
> uncompressed result objects allocated with palloc(), that is point a
> point in the per-buffer-data and make the consumer free it, but that
> has other problems (less locali
On Fri, Feb 14, 2025 at 5:52 AM Melanie Plageman
wrote:
> On Thu, Feb 13, 2025 at 11:28 AM Tomas Vondra wrote:
> > On 2/13/25 17:01, Melanie Plageman wrote:
> > I know it's not changing how much memory we allocate (compared to
> > master). I haven't thought about the GinScanEntry - yes, flexible
Hi,
Thomas, there's a bit relevant to you at the bottom.
Melanie chatted with me about the performance regressions in Tomas' benchmarks
of the patch. I experimented some and I think I found a few interesting
pieces.
I looked solely at cyclic, wm=4096, matches=8, eic=16 as I wanted to
narrow do
On Thu, Feb 13, 2025 at 11:28 AM Tomas Vondra wrote:
>
> On 2/13/25 17:01, Melanie Plageman wrote:
> > On Thu, Feb 13, 2025 at 10:46 AM Tomas Vondra wrote:
> >>
> >> I reviewed v29 today, and I think it's pretty much ready to go.
> >>
> >> The one part where I don't quite get is 0001, which repla
On 2/13/25 17:01, Melanie Plageman wrote:
> On Thu, Feb 13, 2025 at 10:46 AM Tomas Vondra wrote:
>>
>> I reviewed v29 today, and I think it's pretty much ready to go.
>>
>> The one part where I don't quite get is 0001, which replaces a
>> FLEXIBLE_ARRAY_MEMBER array with a fixed-length array. It's
On Thu, Feb 13, 2025 at 10:46 AM Tomas Vondra wrote:
>
> I reviewed v29 today, and I think it's pretty much ready to go.
>
> The one part where I don't quite get is 0001, which replaces a
> FLEXIBLE_ARRAY_MEMBER array with a fixed-length array. It's not wrong,
> but I don't quite see the benefits
On 2/10/25 23:31, Melanie Plageman wrote:
> On Mon, Feb 10, 2025 at 4:22 PM Melanie Plageman
> wrote:
>>
>> I don't really know what to do about this. The behavior of master
>> parallel bitmap heap scan can be emulated with the patch by increasing
>> effective_io_concurrency. But, IIRC we didn't w
On Thu, Feb 13, 2025 at 7:08 AM Tomas Vondra wrote:
>
>
>
> On 2/13/25 01:40, Melanie Plageman wrote:
> > On Sun, Feb 9, 2025 at 9:27 AM Tomas Vondra wrote:
> >>
> >> For the nvme RAID (device: raid-nvme), it's looks almost exactly the
> >> same, except that with parallel query (page 27) there's
On 2/13/25 05:15, Thomas Munro wrote:
> On Thu, Feb 13, 2025 at 1:40 PM Melanie Plageman
> wrote:
>> On Sun, Feb 9, 2025 at 9:27 AM Tomas Vondra wrote:
>>> For the nvme RAID (device: raid-nvme), it's looks almost exactly the
>>> same, except that with parallel query (page 27) there's a clear area
On 2/13/25 01:40, Melanie Plageman wrote:
> On Sun, Feb 9, 2025 at 9:27 AM Tomas Vondra wrote:
>>
>> For the nvme RAID (device: raid-nvme), it's looks almost exactly the
>> same, except that with parallel query (page 27) there's a clear area of
>> regression with eic=1 (look for "column" of red
On Thu, Feb 13, 2025 at 1:40 PM Melanie Plageman
wrote:
> On Sun, Feb 9, 2025 at 9:27 AM Tomas Vondra wrote:
> > For the nvme RAID (device: raid-nvme), it's looks almost exactly the
> > same, except that with parallel query (page 27) there's a clear area of
> > regression with eic=1 (look for "co
On Sun, Feb 9, 2025 at 9:27 AM Tomas Vondra wrote:
>
> For the nvme RAID (device: raid-nvme), it's looks almost exactly the
> same, except that with parallel query (page 27) there's a clear area of
> regression with eic=1 (look for "column" of red cells). That's a bit
> unfortunate, because eic=1
On Wed, Feb 12, 2025 at 3:07 PM Tomas Vondra wrote:
> AFAICS the "1" value is simply one of the many "defensive" defaults in
> our sample config. It's much more likely to help than cause harm, even
> on smaller/older systems, but for many systems a higher value would be
> more appropriate. There's
On 2/12/25 20:08, Robert Haas wrote:
> On Mon, Feb 10, 2025 at 4:41 PM Melanie Plageman
> wrote:
>> I had taken to thinking of it as "queue depth". But I think that's not
>> really accurate. Do you know why we set it to 1 as the default? I
>> thought it was because the default should be just pr
On Mon, Feb 10, 2025 at 4:41 PM Melanie Plageman
wrote:
> I had taken to thinking of it as "queue depth". But I think that's not
> really accurate. Do you know why we set it to 1 as the default? I
> thought it was because the default should be just prefetch one block
> ahead. But if it was meant t
On Mon, Feb 10, 2025 at 4:22 PM Melanie Plageman
wrote:
>
> I don't really know what to do about this. The behavior of master
> parallel bitmap heap scan can be emulated with the patch by increasing
> effective_io_concurrency. But, IIRC we didn't want to do that for some
> reason?
> Not only does
Hi,
On 2025-02-10 16:24:25 -0500, Robert Haas wrote:
> On Mon, Feb 10, 2025 at 1:11 PM Tomas Vondra wrote:
> > Certainly for the "localized" regressions, and cases when bitmapheapscan
> > would not be picked. The eic=1 case makes me a bit more nervous, because
> > it's default and affects NVMe st
On Mon, Feb 10, 2025 at 4:24 PM Robert Haas wrote:
>
> On Mon, Feb 10, 2025 at 1:11 PM Tomas Vondra wrote:
> > Certainly for the "localized" regressions, and cases when bitmapheapscan
> > would not be picked. The eic=1 case makes me a bit more nervous, because
> > it's default and affects NVMe st
On Mon, Feb 10, 2025 at 1:11 PM Tomas Vondra wrote:
> Certainly for the "localized" regressions, and cases when bitmapheapscan
> would not be picked. The eic=1 case makes me a bit more nervous, because
> it's default and affects NVMe storage. Would be good to know why is
> that, or perhaps conside
On Mon, Feb 10, 2025 at 1:02 PM Melanie Plageman
wrote:
>
> It'll be hard to look into all of these, so I think I'll focus on
> trying to reproduce something with eic=1 that I can reproduce on my
> machine. So far, I can reproduce a regression with the following and
> the data file attached.
>
> #
On 2/10/25 19:02, Melanie Plageman wrote:
> On Sun, Feb 9, 2025 at 9:27 AM Tomas Vondra wrote:
>>
>>
>> 2) ryzen
>>
>>
>> This "new" machine has multiple types of storage. The cached results (be
>> it in shared buffers or in page cache) are not very interesting. 0003
>> helps a bit (~15%)
On Sun, Feb 9, 2025 at 9:27 AM Tomas Vondra wrote:
>
>
> 2) ryzen
>
>
> This "new" machine has multiple types of storage. The cached results (be
> it in shared buffers or in page cache) are not very interesting. 0003
> helps a bit (~15%), but other than that it's just random noise.
>
> Th
On 1/30/25 21:36, Melanie Plageman wrote:
>
> ...
>
> Attached v28 is rebased and has a few updates/cleanup. All patches in
> the set need review and I need to do some benchmarking of v28-0003.
>
> - Melanie
Hi,
I've been re-running the benchmarks on v28 on Melanie's request. The
tests are stil
On Tue, Mar 19, 2024 at 4:34 PM Tomas Vondra
wrote:
>
> On 3/18/24 16:55, Tomas Vondra wrote:
> >
> > ...
> >
> > OK, I've restarted the tests for only 0012 and 0014 patches, and I'll
> > wait for these to complete - I don't want to be looking for patterns
> > until we have enough data to smooth t
On Thu, Jan 30, 2025 at 3:02 AM Nazir Bilal Yavuz wrote:
>
> On Thu, 30 Jan 2025 at 00:38, Melanie Plageman
> wrote:
> >
> > On Wed, Jan 22, 2025 at 4:24 PM Melanie Plageman
> > wrote:
> > >
> > > 0001 is just the refactoring to push the setup into a helper.
> > > 0002-0003 are required refactor
Hi,
On Thu, 30 Jan 2025 at 00:38, Melanie Plageman
wrote:
>
> On Wed, Jan 22, 2025 at 4:24 PM Melanie Plageman
> wrote:
> >
> > 0001 is just the refactoring to push the setup into a helper.
> > 0002-0003 are required refactoring of the TBMIterateResult and
> > TBMIterator to support the read str
On Wed, Jan 22, 2025 at 4:24 PM Melanie Plageman
wrote:
>
> On Mon, Dec 9, 2024 at 1:22 AM Dilip Kumar wrote:
> >
> > Are we planning to commit this refactoring? I think this refactoring
> > makes the overall code of BitmapHeapNext() quite clean and readable.
> > I haven't read all patches but 00
On Mon, Dec 9, 2024 at 1:22 AM Dilip Kumar wrote:
>
> Are we planning to commit this refactoring? I think this refactoring
> makes the overall code of BitmapHeapNext() quite clean and readable.
> I haven't read all patches but 0001-0006 including 0009 makes this
> code quite clean and readable. I
On Thu, Dec 19, 2024 at 10:23 AM Melanie Plageman
wrote:
>
> On Thu, Dec 19, 2024 at 10:12 AM Richard Guo wrote:
> >
> > On Thu, Dec 19, 2024 at 6:15 PM Richard Guo wrote:
> > > I think we need to check whether rs_tbmiterator is NULL before calling
> > > tbm_end_iterate on it, like below.
> > >
On Thu, Dec 19, 2024 at 10:12 AM Richard Guo wrote:
>
> On Thu, Dec 19, 2024 at 6:15 PM Richard Guo wrote:
> > I think we need to check whether rs_tbmiterator is NULL before calling
> > tbm_end_iterate on it, like below.
> >
> > --- a/src/backend/executor/nodeBitmapHeapscan.c
> > +++ b/src/backen
On Thu, Dec 19, 2024 at 6:15 PM Richard Guo wrote:
> I think we need to check whether rs_tbmiterator is NULL before calling
> tbm_end_iterate on it, like below.
>
> --- a/src/backend/executor/nodeBitmapHeapscan.c
> +++ b/src/backend/executor/nodeBitmapHeapscan.c
> @@ -572,9 +572,11 @@ ExecReScanBi
On Sat, Oct 19, 2024 at 5:48 AM Melanie Plageman
wrote:
> I plan to commit 0002 and 0003 next week. I'm interested if you think
> 0001 is correct.
> I may also commit 0004-0006 as I feel they are ready too.
I noticed an oversight on master, which I think was introduced by the
0005 patch. In Exec
On Sat, Oct 19, 2024 at 2:18 AM Melanie Plageman
wrote:
>
> On Wed, Oct 16, 2024 at 11:09 AM Tomas Vondra wrote:
> >
>
> Thanks for the review!
>
> >
> > 1) 0001
> >
> > I find it a bit weird that we use ntuples to determine if it's exact or
> > lossy. Not an issue caused by this patch, of course
Hi,
I took a quick look on the first couple parts of this series, up to
0007. I only have a couple minor review comments:
1) 0001
I find it a bit weird that we use ntuples to determine if it's exact or
lossy. Not an issue caused by this patch, of course, but maybe we could
improve that somehow?
On Tue, Oct 8, 2024 at 4:56 PM Thomas Munro wrote:
>
> Just by the way, you can't use anonymous structs or unions in C99
> (that was added to C11), but most compilers accept them silently
> unless you use eg -std=c99. Some buildfarm animal or other would
> bleat about that (ask me how I know), bu
On Sat, Sep 28, 2024 at 8:13 AM Melanie Plageman
wrote:
> For the top-level TableScanDescData, I suggest we use a union with the
> members of each scan type in it in anonymous structs (see 0001).
Just by the way, you can't use anonymous structs or unions in C99
(that was added to C11), but most c
On Wed, Jun 19, 2024 at 2:13 PM Melanie Plageman
wrote:
>
> On Wed, Jun 19, 2024 at 12:38 PM Tomas Vondra
> wrote:
>
> > > + * XXX I don't understand why we should have this special node if we
> > > + * don't have special nodes for other scan types.
> > >
> > > In this case, up until the final co
On Mon, Aug 26, 2024 at 10:49 AM Tom Lane wrote:
>
> Robert Haas writes:
> > On Wed, Jun 19, 2024 at 2:21 PM Melanie Plageman
> > wrote:
> >> If we want to make it possible to use no tools and only manually grep
> >> for struct members, that means we can never reuse struct member names.
> >> Acr
Robert Haas writes:
> On Wed, Jun 19, 2024 at 2:21 PM Melanie Plageman
> wrote:
>> If we want to make it possible to use no tools and only manually grep
>> for struct members, that means we can never reuse struct member names.
>> Across a project of our size, that seems like a very serious
>> res
On Wed, Jun 19, 2024 at 2:21 PM Melanie Plageman
wrote:
> If we want to make it possible to use no tools and only manually grep
> for struct members, that means we can never reuse struct member names.
> Across a project of our size, that seems like a very serious
> restriction. Adding prefixes in
On 19/06/2024 18:55, Melanie Plageman wrote:
On Tue, Jun 18, 2024 at 6:02 PM Tomas Vondra
wrote:
I went through v22 to remind myself of what the patches do and do some
basic review - I have some simple questions / comments for now, nothing
major. I've kept the comments in separate 'review' pat
Thanks for taking a look at my patches, Álvaro!
On Wed, Jun 19, 2024 at 12:51 PM Alvaro Herrera wrote:
>
> On 2024-Jun-14, Melanie Plageman wrote:
>
> > Subject: [PATCH v21 12/20] Update variable names in bitmap scan descriptors
> >
> > The previous commit which added BitmapTableScanDesc and
> >
On Wed, Jun 19, 2024 at 12:38 PM Tomas Vondra
wrote:
>
>
>
> On 6/19/24 17:55, Melanie Plageman wrote:
> > On Tue, Jun 18, 2024 at 6:02 PM Tomas Vondra
> > wrote:
>
> > From your v22b-0017-review.patch
> >
> > diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h
> > index 036e
On 2024-Jun-14, Melanie Plageman wrote:
> Subject: [PATCH v21 12/20] Update variable names in bitmap scan descriptors
>
> The previous commit which added BitmapTableScanDesc and
> BitmapHeapScanDesc used the existing member names from TableScanDescData
> and HeapScanDescData for diff clarity. This
On 6/19/24 17:55, Melanie Plageman wrote:
> On Tue, Jun 18, 2024 at 6:02 PM Tomas Vondra
> wrote:
>>
>> I went through v22 to remind myself of what the patches do and do some
>> basic review - I have some simple questions / comments for now, nothing
>> major. I've kept the comments in separate
On 2024-May-14, Melanie Plageman wrote:
> On Tue, May 14, 2024 at 4:05 PM Alvaro Herrera
> wrote:
> > I do wonder how do we _know_ that the test is testing what it wants to
> > test:
> We include the explain output (the plan) to ensure it is still
> using a bitmap heap scan. The test exercises
Procedural comment:
It's better to get this patch committed with an imperfect test case
than to have it miss beta1.
...Robert
On Tue, May 14, 2024 at 4:05 PM Alvaro Herrera wrote:
>
> On 2024-May-14, Tomas Vondra wrote:
>
> I wonder why it resets enable_indexscan at all. I see that this query
> first tries a seqscan, then if you disable that it tries an index only
> scan, and if you disable that you get the expected bit
On 2024-May-14, Alvaro Herrera wrote:
> BTW, I was running the explain while desultorily enabling and disabling
> these GUCs and hit this assertion failure:
>
> #4 0x55e6c72afe28 in ExceptionalCondition
> (conditionName=conditionName@entry=0x55e6c731a928
> "scan->rs_empty_tuples_pending =
On 2024-May-14, Tomas Vondra wrote:
> On 5/14/24 19:42, Melanie Plageman wrote:
>
> >>> +SET enable_indexonlyscan = off;
> >>> +set enable_indexscan = off;
> >>> +SET enable_seqscan = off;
> >>
> >> Nit: adjusting the casing of the second SET here.
> >
> > I've fixed this. I've also set enable_m
On Tue, May 14, 2024 at 2:44 PM Tomas Vondra
wrote:
>
> On 5/14/24 20:40, Melanie Plageman wrote:
> > On Tue, May 14, 2024 at 2:33 PM Tomas Vondra
> > wrote:
> >>
> >> On 5/14/24 19:42, Melanie Plageman wrote:
> >>> I've fixed this. I've also set enable_material off as I mentioned I
> >>> might i
On 5/14/24 20:40, Melanie Plageman wrote:
> On Tue, May 14, 2024 at 2:33 PM Tomas Vondra
> wrote:
>>
>> On 5/14/24 19:42, Melanie Plageman wrote:
>>> I've fixed this. I've also set enable_material off as I mentioned I
>>> might in my earlier mail.
>>>
>>
>> I'm not sure this (setting more and more
1 - 100 of 209 matches
Mail list logo
