Subject: Re: [PROPOSAL] Shared Ispell dictionaries
To: Arthur Zakirov <a.zakirov@postgrespro.ru>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <20171226164825.GA29922@zakirov.localdomain>
 <20171231152811.GA4233@arthur.localdomain>
 <20180107190526.GA27803@arthur.localdomain>
 <d12d9395-922c-64c9-c87d-dd0e1d31440e@2ndquadrant.com>
 <CAKNkYnzQ8SJCiFityn+4B_0zZKOmfw1R1M0KHXaFF8BYf9DJ=g@mail.gmail.com>
 <20180124172039.GA11210@zakirov.localdomain>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <79abaac7-fbc1-05ef-342e-102001af4fca@2ndquadrant.com>
Date: Wed, 24 Jan 2018 18:57:08 +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: <20180124172039.GA11210@zakirov.localdomain>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 01/24/2018 06:20 PM, Arthur Zakirov wrote:
> On Sat, Jan 13, 2018 at 06:22:41PM +0300, Arthur Zakirov wrote:
>> I think your proposals may be implemented in several patches, so
>> they can be applyed independently but consistently. I suppose I
>> will prepare new version of the patch with fixes and with initial
>> design of new functions and commands soon.
> 
> I attached new version of the patch.
> 

Thanks. I don't have time to review/test this before FOSDEM, but a
couple of comments regarding some of the points you mentioned.

>> 3) How do I unload a dictionary from the shared memory?
>> ...
>>     ALTER TEXT SEARCH DICTIONARY x UNLOAD
>>
>> 4) How do I reload a dictionary?
>> ...
>>     ALTER TEXT SEARCH DICTIONARY x RELOAD
> 
> I thought about it. And it seems to me that we can use functions 
> ts_unload() and ts_reload() instead of new syntax. We already have
> text search functions like ts_lexize() and ts_debug(), and it is
> better to keep consistency.

This argument seems a bit strange. Both ts_lexize() and ts_debug() are
operating on text values, and are meant to be executed as functions from
SQL - particularly ts_lexize(). It's hard to imagine this implemented as
DDL commands.

The unload/reload is something that operates on a database object
(dictionary), which already has create/drop/alter DDL. So it seems
somewhat natural to treat unload/reload as another DDL action.

Taken to an extreme, this argument would essentially mean we should not
have any DDL commands because we have SQL functions.

That being said, I'm not particularly attached to having this DDL now.
Implementing it seems straight-forward (particularly when we already
have the stuff implemented as functions), and some of the other open
questions seem more important to tackle now.

> I think there are to approach for ts_unload():> - use DSM's pin and unpin methods and the invalidation callback, as
> it done during fixing memory leak. It has the drawback that it won't
> have an immediate effect, because DSM will be released only when all
> backends unpin DSM mapping.
> - use DSA and dsa_free() method. As far as I understand dsa_free() 
> frees allocated memory immediatly. But it requires more work to do, 
> because we will need some more locks. For instance, what happens
> when someone calls ts_lexize() and someone else calls dsa_free() at
> the same time.


No opinion on this yet, I have to think about it for a bit and look at
the code first.

>> 7) You mentioned you had to get rid of the compact_palloc0 - can you
>> elaborate a bit why that was necessary? Also, when benchmarking the
>> impact of this make sure to measure not only the time but also memory
>> consumption.
> 
> It seems to me that there is no need compact_palloc0() anymore. Tests
> show that czech dictionary doesn't consume more memory after the
> patch.
> 

That's interesting. I'll do some additional tests to verify the finding.


regards

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