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>
Cc: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Ildus Kurbangaliev <i(dot)kurbangaliev(at)postgrespro(dot)ru>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PROPOSAL] Shared Ispell dictionaries
Date: 2018-03-20 13:11:51
Message-ID: 20180320131149.GA12884@zakirov.localdomain
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On Mon, Mar 19, 2018 at 08:50:46PM +0100, Tomas Vondra wrote:
> Hi Arthur,
> I went through the patch - just skimming through the diffs, will do more
> testing tomorrow. Here are a few initial comments.

Thank you for the review!

> 1) max_shared_dictionaries_size / PGC_POSTMASTER
> I'm not quite sure why the GUC is defined as PGC_POSTMASTER, i.e. why it
> can't be changed after server start. That seems like a fairly useful
> thing to do (e.g. increase the limit while the server is running), and
> after looking at the code I think it shouldn't be difficult to change.

max_shared_dictionaries_size is defined as PGC_SIGHUP now. Added check
of a new value to disallow to set zero if there are loaded dictionaries
and to decrease maximum allowed size if loaded size is greater than the
new value.

> The other thing I'd suggest is handling "-1" as "no limit".

I added availability to set '-1'. Fixed some comments and the

> 2) max_shared_dictionaries_size / size of number
> Some of the comments dealing with the GUC treat it as a number of
> dictionaries (instead of a size). I suppose that's due to how the
> original patch was implemented.

Fixed. Should be good now.

> 3) Assert(max_shared_dictionaries_size);
> I'd say that assert is not very clear - it should be
> Assert(max_shared_dictionaries_size > 0);
> or something along the lines. It's also a good idea to add a comment
> explaining the assert, say
> /* we can only get here when shared dictionaries are enabled */
> Assert(max_shared_dictionaries_size > 0);

Fixed the assert and added the comment. I extended the assert, it also
takes into account -1 value.

> 4) I took the liberty of rewording some of the docs/comments. See the
> attached diffs, that should apply on top of 0003 and 0004 patches.
> Please, treat those as mere suggestions.

I applied your diffs and added changes to max_shared_dictionaries_size.

Please find the attached new version of the patch.

Arthur Zakirov
Postgres Professional:
Russian Postgres Company

Attachment Content-Type Size
0001-Fix-ispell-memory-handling-v8.patch text/plain 1.2 KB
0002-Change-tmplinit-argument-v8.patch text/plain 7.9 KB
0003-Retreive-shared-location-for-dict-v8.patch text/plain 19.6 KB
0004-Store-ispell-in-shared-location-v8.patch text/plain 89.3 KB
0005-pg-ts-shared-dictinaries-view-v8.patch text/plain 9.3 KB
0006-Shared-memory-ispell-option-v8.patch text/plain 9.3 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-03-20 13:55:52 Re: configure's checks for --enable-tap-tests are insufficient
Previous Message Torsten Grust 2018-03-20 12:58:46