Subject: Re: [PROPOSAL] Shared Ispell dictionaries
To: Arthur Zakirov <a.zakirov@postgrespro.ru>
Cc: Pavel Stehule <pavel.stehule@gmail.com>,
 Andres Freund <andres@anarazel.de>,
 Ildus Kurbangaliev <i.kurbangaliev@postgrespro.ru>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <20180302043149.tn2xjgt2vcigknhe@alap3.anarazel.de>
 <20180307085553.GA5126@zakirov.localdomain>
 <3195b6e6-4026-d7e2-ffa9-3780bc3cca6f@2ndquadrant.com>
 <20180307115528.GA10093@zakirov.localdomain>
 <CAFj8pRDOesYrROCDd-6YQzQcBQhZfD980zsGsfuu4NrRS-Gb1A@mail.gmail.com>
 <20180307124353.GA12054@zakirov.localdomain>
 <CAFj8pRDZOMQOmMhX4KSn+KTXbdB24ZaJxTxnYCHms_ya2Z2cOQ@mail.gmail.com>
 <20180307125847.GA12617@zakirov.localdomain>
 <CAFj8pRB=FWzq=FfoSPeoJfXG4a7P0Abp+0XKmet3E34tiuTUFA@mail.gmail.com>
 <CAFj8pRDm9nhZCf16XDer-eeciqy0wipR0Y4+ZdwGMAWuv-k6ag@mail.gmail.com>
 <20180307131819.GA13301@zakirov.localdomain>
 <3c7910ba-2a1b-5b73-71c6-d037ba1e89bd@2ndquadrant.com>
 <CAKNkYnzWuSV-JoMBPDrWPjomUC4n1QtYd8Yugo9hpJrbtHcALg@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <d4d52484-e269-9d7b-cef5-c1e95a03d9c6@2ndquadrant.com>
Date: Mon, 19 Mar 2018 01:52:41 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: 
 <CAKNkYnzWuSV-JoMBPDrWPjomUC4n1QtYd8Yugo9hpJrbtHcALg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 03/17/2018 05:43 AM, Arthur Zakirov wrote:
> Hello Tomas,
> 
>     Arthur, what are your plans with this patch in the current CF?
> 
> 
> I think dsm-based approach is in good shape already and works nice.
> I've planned only to improve the documentation a little. Also it seems I
> should change 0004 part, I found that extension upgrade scripts may be
> made in wrong way.
> In my opinion RELOAD and UNLOAD commands can be made in next commitfest
> (2018-09).
> Did you look it? Have you arguments about how shared memory allocation
> and releasing functions are made?
>  
> 
> 
>     It does not seem to be moving towards RFC very much, and reworking the
>     patch to use mmap() seems like a quite significant change late in the
>     CF. Which means it's likely to cause the patch get get bumped to the
>     next CF (2018-09).
> 
> 
> Agree. I have a draft version for mmap-based approach which works in
> platforms with mmap. In Windows it is necessary to use another API
> (CreateFileMapping, etc). But this approach requires more work on
> handling processed dictionary files (how name them, when remove).
>  
> 
> 
>     FWIW I am not quite sure if the mmap() approach is better than what was
>     implemented by the patch. I'm not sure how exactly will it behave under
>     memory pressure (AFAIK it goes through page cache, which means random
>     parts of dictionaries might get evicted) or how well is it supported on
>     various platforms (say, Windows).
> 
> 
> Yes, as I wrote mmap-based approach requires more work. The only
> benefit I see is that you don't need to process a dictionary after
> server restart.  I'd vote for dsm-based approach.
> 

I do agree with that. We have a working well-understood dsm-based
solution, addressing the goals initially explained in this thread.

I don't see a reason to stall this patch based on a mere assumption that
the mmap-based approach might be magically better in some unknown
aspects. It might be, but we may as well leave that as a future work.

I wonder how much of this patch would be affected by the switch from dsm
to mmap? I guess the memory limit would get mostly irrelevant (mmap
would rely on the OS to page the memory in/out depending on memory
pressure), and so would the UNLOAD/RELOAD commands (because each backend
would do it's own mmap).

In any case, I suggest to polish the dsm-based patch, and see if we can
get that one into PG11.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services