Re: brininsert optimization opportunity

On Fri, Apr 19, 2024 at 04:14:00PM +0200, Tomas Vondra wrote: > FWIW I've pushed both patches, which resolves the open item, so I've > moved it to the "resolved" part on wiki. Thanks, Tomas! -- Michael signature.asc Description: PGP signature

Re: brininsert optimization opportunity

On 4/19/24 00:13, Tomas Vondra wrote: > ... > > It's a bit too late for me to push this now, I'll do so early tomorrow. > FWIW I've pushed both patches, which resolves the open item, so I've moved it to the "resolved" part on wiki. thanks -- Tomas Vondra EnterpriseDB:

Re: brininsert optimization opportunity

Hi, Here's two patched to deal with this open item. 0001 is a trivial fix of typos and wording, I moved it into a separate commit for clarity. 0002 does the actual fix - adding the index_insert_cleanup(). It's 99% the patch Alvaro shared some time ago, with only some minor formatting tweaks by

Re: brininsert optimization opportunity

On 4/18/24 09:07, Michael Paquier wrote: > On Thu, Feb 29, 2024 at 01:20:39PM +0100, Alvaro Herrera wrote: >> I think it's not an issue, or rather that we should not try to guess. >> Instead make it a simple rule: if aminsert is called, then >> aminsertcleanup must be called afterwards, period. >>

Re: brininsert optimization opportunity

On Thu, Feb 29, 2024 at 01:20:39PM +0100, Alvaro Herrera wrote: > I think it's not an issue, or rather that we should not try to guess. > Instead make it a simple rule: if aminsert is called, then > aminsertcleanup must be called afterwards, period. > > I agree it would be nice to have a way to

Re: brininsert optimization opportunity

On 2024-Feb-13, Tomas Vondra wrote: > One thing that is not very clear to me is that I don't think there's a > very good way to determine which places need the cleanup call. Because > it depends on (a) what kind of index is used and (b) what happens in the > code called earlier (which may easily

Re: brininsert optimization opportunity

On 1/8/24 16:51, Alvaro Herrera wrote: > On 2023-Dec-12, Tomas Vondra wrote: > >> I propose we do a much simpler thing instead - allow the cache may be >> initialized / cleaned up repeatedly, and make sure it gets reset at >> convenient place (typically after index_insert calls that don't go >>

Re: brininsert optimization opportunity

On 2024-Jan-08, Alvaro Herrera wrote: > So I think we should do 0001 and perhaps some further tweaks to the > original brininsert optimization commit: [...] So I propose the attached patch, which should fix the reported bug and the things I mentioned above, and also the typos Alexander mentioned

Re: brininsert optimization opportunity

On 2023-Dec-12, Tomas Vondra wrote: > I propose we do a much simpler thing instead - allow the cache may be > initialized / cleaned up repeatedly, and make sure it gets reset at > convenient place (typically after index_insert calls that don't go > through execIndexing). That'd mean the cleanup

Re: brininsert optimization opportunity

On 2023-Dec-21, James Wang wrote: > Hi All, not sure how to "Specify thread msgid" - choose one which i think > is close to my new feature request. Hello James, based on the "Specify thread msgid" message it looks like you were trying to request a feature using the Commitfest website? That

Re: brininsert optimization opportunity

Hi All, not sure how to "Specify thread msgid" - choose one which i think is close to my new feature request. query: SELECT count(1) FROM table1 t1 JOIN table2 t2 ON t1.id = t2.id WHERE t1.a_indexed_col='some_value' OR t2.a_indexed_col='some_vable'; can the server automatically replace the

Re: brininsert optimization opportunity

On 12/13/23 09:12, Soumyadeep Chakraborty wrote: > On Tue, Dec 12, 2023 at 3:25 AM Tomas Vondra > wrote: > > >> The attached 0001 patch fixes this by adding the call to validate_index, > which seems like the proper place as it's where the indexInfo is > allocated and destroyed. > > Yes, and

Re: brininsert optimization opportunity

On Tue, Dec 12, 2023 at 3:25 AM Tomas Vondra wrote: > The attached 0001 patch fixes this by adding the call to validate_index, which seems like the proper place as it's where the indexInfo is allocated and destroyed. Yes, and by reading validate_index's header comment, there is a clear

Re: brininsert optimization opportunity

On 12/11/23 16:41, Tomas Vondra wrote: > On 12/11/23 16:00, Alexander Lakhin wrote: >> Hello Tomas and Soumyadeep, >> >> 25.11.2023 23:06, Tomas Vondra wrote: >>> I've done a bit more cleanup on the last version of the patch (renamed >>> the fields to start with bis_ as agreed, rephrased the

Re: brininsert optimization opportunity

On 12/11/23 16:00, Alexander Lakhin wrote: > Hello Tomas and Soumyadeep, > > 25.11.2023 23:06, Tomas Vondra wrote: >> I've done a bit more cleanup on the last version of the patch (renamed >> the fields to start with bis_ as agreed, rephrased the comments / docs / >> commit message a bit) and

Re: brininsert optimization opportunity

Hello Tomas and Soumyadeep, 25.11.2023 23:06, Tomas Vondra wrote: I've done a bit more cleanup on the last version of the patch (renamed the fields to start with bis_ as agreed, rephrased the comments / docs / commit message a bit) and pushed. Please look at a query, which produces warnings

Re: brininsert optimization opportunity

On 11/27/23 11:34, Tomas Vondra wrote: > > > On 11/27/23 08:37, Richard Guo wrote: >> >> On Mon, Nov 27, 2023 at 1:53 PM Soumyadeep Chakraborty >> mailto:soumyadeep2...@gmail.com>> wrote: >> >> On Sun, Nov 26, 2023 at 9:28 PM Richard Guo > > wrote: >> >

Re: brininsert optimization opportunity

On 11/27/23 08:37, Richard Guo wrote: > > On Mon, Nov 27, 2023 at 1:53 PM Soumyadeep Chakraborty > mailto:soumyadeep2...@gmail.com>> wrote: > > On Sun, Nov 26, 2023 at 9:28 PM Richard Guo > wrote: > > It seems that we have an oversight in this

Re: brininsert optimization opportunity

On Mon, Nov 27, 2023 at 1:53 PM Soumyadeep Chakraborty < soumyadeep2...@gmail.com> wrote: > On Sun, Nov 26, 2023 at 9:28 PM Richard Guo > wrote: > > It seems that we have an oversight in this commit. If there is no tuple > > that has been inserted, we wouldn't have an available insert state in

Re: brininsert optimization opportunity

On Sun, Nov 26, 2023 at 9:28 PM Richard Guo wrote: > > > On Sun, Nov 26, 2023 at 4:06 AM Tomas Vondra > wrote: >> >> I've done a bit more cleanup on the last version of the patch (renamed >> the fields to start with bis_ as agreed, rephrased the comments / docs / >> commit message a bit) and

Re: brininsert optimization opportunity

On Sun, Nov 26, 2023 at 4:06 AM Tomas Vondra wrote: > I've done a bit more cleanup on the last version of the patch (renamed > the fields to start with bis_ as agreed, rephrased the comments / docs / > commit message a bit) and pushed. It seems that we have an oversight in this commit. If

Re: brininsert optimization opportunity

Thanks a lot for reviewing and pushing! :) Regards, Soumyadeep (VMware)

Re: brininsert optimization opportunity

On Sat, Nov 25, 2023 at 12:06 PM Tomas Vondra wrote: > I've done a bit more cleanup on the last version of the patch (renamed > the fields to start with bis_ as agreed, rephrased the comments / docs / > commit message a bit) and pushed. Thanks a lot Tomas for helping to drive the patch to

Re: brininsert optimization opportunity

I've done a bit more cleanup on the last version of the patch (renamed the fields to start with bis_ as agreed, rephrased the comments / docs / commit message a bit) and pushed. Thanks for the patch! -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company

Re: brininsert optimization opportunity

On Fri, 3 Nov 2023 at 19:37, Tomas Vondra wrote: > The one thing I'm not entirely sure about is adding new stuff to the > IndexAmRoutine. I don't think we want to end up with too many callbacks > that all AMs have to initialize etc. I can't think of a different/better > way to do this, though.

Re: brininsert optimization opportunity

On Fri, 3 Nov 2023 at 19:37, Tomas Vondra wrote: > > Hi, > > I took a look at this patch today. I had to rebase the patch, due to > some minor bitrot related to 9f0602539d (but nothing major). I also did > a couple tiny cosmetic tweaks, but other than that the patch seems OK. > See the attached

Re: brininsert optimization opportunity

Hi, I took a look at this patch today. I had to rebase the patch, due to some minor bitrot related to 9f0602539d (but nothing major). I also did a couple tiny cosmetic tweaks, but other than that the patch seems OK. See the attached v6. I did some simple performance tests too, similar to those

Re: brininsert optimization opportunity

Rebased against latest HEAD. Regards, Soumyadeep (VMware) From 94a8acd3125aa4a613c238e435ed78dba9f40625 Mon Sep 17 00:00:00 2001 From: Soumyadeep Chakraborty Date: Sat, 29 Jul 2023 09:17:49 -0700 Subject: [PATCH v5 1/1] Reuse revmap and brin desc in brininsert brininsert() used to have code

Re: brininsert optimization opportunity

Created an entry for the Sep CF: https://commitfest.postgresql.org/44/4484/ Regards, Soumyadeep (VMware) On Sat, Jul 29, 2023 at 9:28 AM Soumyadeep Chakraborty wrote: > > Attached v4 of the patch, rebased against latest HEAD. > > Regards, > Soumyadeep (VMware)

Re: brininsert optimization opportunity

Attached v4 of the patch, rebased against latest HEAD. Regards, Soumyadeep (VMware) From b5fb12fbb8b0c1b2963a05a2877b5063bbc75f58 Mon Sep 17 00:00:00 2001 From: Soumyadeep Chakraborty Date: Sat, 29 Jul 2023 09:17:49 -0700 Subject: [PATCH v4 1/1] Reuse revmap and brin desc in brininsert

Re: brininsert optimization opportunity

On Wed, Jul 5, 2023 at 3:16 AM Tomas Vondra wrote: > > I'll try this out and introduce a couple of new index AM callbacks. I > > think it's best to do it before releasing the locks - otherwise it > > might be weird > > to manipulate buffers of an index relation, without having some sort of > >

Re: brininsert optimization opportunity

On 7/5/23 02:35, Soumyadeep Chakraborty wrote: > On Tue, Jul 4, 2023 at 2:54 PM Tomas Vondra > wrote: > >> I'm not sure if memory context callbacks are the right way to rely on >> for this purpose. The primary purpose of memory contexts is to track >> memory, so using them for this seems a

Re: brininsert optimization opportunity

On Tue, Jul 4, 2023 at 2:54 PM Tomas Vondra wrote: > I'm not sure if memory context callbacks are the right way to rely on > for this purpose. The primary purpose of memory contexts is to track > memory, so using them for this seems a bit weird. Yeah, this just kept getting dirtier and dirtier.

Re: brininsert optimization opportunity

On 7/4/23 21:25, Soumyadeep Chakraborty wrote: > Thank you both for reviewing! > > On Tue, Jul 4, 2023 at 4:24AM Alvaro Herrera wrote: > >> Hmm, yeah, I remember being bit bothered by this repeated >> initialization. Your patch looks reasonable to me. I would set >> bistate->bs_rmAccess to NULL

Re: brininsert optimization opportunity

Thank you both for reviewing! On Tue, Jul 4, 2023 at 4:24AM Alvaro Herrera wrote: > Hmm, yeah, I remember being bit bothered by this repeated > initialization. Your patch looks reasonable to me. I would set > bistate->bs_rmAccess to NULL in the cleanup callback, just to be sure. > Also, please

Re: brininsert optimization opportunity

On 7/4/23 13:23, Alvaro Herrera wrote: > On 2023-Jul-03, Soumyadeep Chakraborty wrote: > >> My colleague, Ashwin, pointed out to me that brininsert's per-tuple init >> of the revmap access struct can have non-trivial overhead. >> >> Turns out he is right. We are saving 24 bytes of memory

Re: brininsert optimization opportunity

On 2023-Jul-03, Soumyadeep Chakraborty wrote: > My colleague, Ashwin, pointed out to me that brininsert's per-tuple init > of the revmap access struct can have non-trivial overhead. > > Turns out he is right. We are saving 24 bytes of memory per-call for > the access struct, and a bit on