Date: Thu, 25 Jan 2018 15:26:46 +0300
From: Ildus Kurbangaliev <i.kurbangaliev@postgrespro.ru>
To: Arthur Zakirov <a.zakirov@postgrespro.ru>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>, pgsql-hackers
 <pgsql-hackers@postgresql.org>
Subject: Re: [PROPOSAL] Shared Ispell dictionaries
Message-ID: <20180125152646.3b4d2850@wp.localdomain>
In-Reply-To: <20180124172039.GA11210@zakirov.localdomain>
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>
Organization: Postgres Professional
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 24 Jan 2018 20:20:41 +0300
Arthur Zakirov <a.zakirov@postgrespro.ru> wrote:

Hi, I did some review of the patch.

In 0001 there are few lines where is only indentation has changed.

0002:
- TsearchShmemSize - calculating size using hash_estimate_size seems
redundant since you use DSA hash now.
- ts_dict_shmem_release - LWLockAcquire in the beginning makes no
  sense, since dict_table couldn't change anyway.

0003:
- ts_dict_shmem_location could return IspellDictData, it makes more
  sense.

0006:
It's very subjective, but I think it would nicer to call option as
Shared (as property of dictionary) or UseSharedMemory, the boolean
option called SharedMemory sounds weird.

Overall the patches look good, all tests passed. I tried to broke it in
few places where I thought it could be unsafe but not succeeded.

-- 
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company