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
DictPointerData):
https://www.postgresql.org/message-id/20180319110648.GA32319%40zakirov.localdomain
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
ALTER TEXT SEARCH TEMPLATE. Thoughts?
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: http://www.postgrespro.com
Russian Postgres Company
