Re: [PROPOSAL] Shared Ispell dictionaries

From: Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
To: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PROPOSAL] Shared Ispell dictionaries
Date: 2019-01-16 11:42:44
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello Tomas,

On 16.01.2019 03:23, Tomas Vondra wrote:
> I've looked at the patch today, and in general is seems quite solid to
> me. I do have a couple of minor points
> 1) I think the comments need more work. Instead of describing all the
> individual changes here, I've outlined those improvements in attached
> patches (see the attached "tweaks" patches). Some of it is formatting,
> minor rewording or larger changes. Some comments are rather redundant
> (e.g. the one before calls to release the DSM segment).

Thank you!

> 2) It's not quite clear to me why we need DictInitData, which simply
> combines DictPointerData and list of options. It seems as if the only
> point is to pass a single parameter to the init function, but is it
> worth it? Why not to get rid of DictInitData entirely and pass two
> parameters instead?

In the first place init method had two parameters. But in the v7 patch I
added DictInitData struct instead of two parameters (list of options and

I haven't way to replace template's init method from
init_method(internal) to init_method(internal,internal) in the upgrade
script of extensions. If I'm not mistaken we need new syntax here, like

> 3) I find it a bit cumbersome that before each ts_dict_shmem_release
> call we construct a dummy DickPointerData value. Why not to pass
> individual parameters and construct the struct in the function?

Agree, it may look too verbose. I'll change it.

> 4) The reference to max_shared_dictionaries_size is obsolete, because
> there's no such limit anymore.

Yeah, I'll fix it.

> /* XXX not really a pointer, so the name is misleading */

I think we don't need DictPointerData struct anymore, because only
ts_dict_shmem_release function needs it (see comments above) and we only
need it to hash search. I'll move all fields of DictPointerData to
TsearchDictKey struct.

> XXX "supported" is not the same as "all ispell dicts behave like that".

I'll reword the sentence.

Arthur Zakirov
Postgres Professional:
Russian Postgres Company

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Dmitry Dolgov 2019-01-16 12:16:40 ArchiveEntry optional arguments refactoring
Previous Message Alexander Kuzmenkov 2019-01-16 11:39:53 Redundant filter in index scan with a bool column