Pavel Stehule <pavel.steh...@gmail.com> wrote:

> this version has enhanced AllocSet allocator - it can use a  mmap API.

I review your patch and will report some comments. However, I don't have
test cases for the patch because there is no large dictionaries in the
default postgres installation. I'd like to ask you to supply test data
for the patch.

This patch allocates memory with non-file-based mmap() to preload text search
dictionary files at the server start. Note that dist files are not mmap'ed
directly in the patch; mmap() is used for reallocatable shared memory.

The dictinary loader is also modified a bit to use simple_alloc() instead
of palloc() for long-lived cache. It can reduce calls of AllocSetAlloc(),
that have some overheads to support pfree(). Since the cache is never
released, simple_alloc() seems to have better performance than palloc().
Note that the optimization will also work for non-preloaded dicts.

=== Questions ===
- How do backends share the dict cache? You might expect postmaster's
  catalog is inherited to backends with fork(), but we don't use fork()
  on Windows.

- Why are SQL functions dpreloaddict_init() and dpreloaddict_lexize()
  defined but not used?

=== Design ===
- You added 3 custom parameters (dict_preload.dictfile/afffile/stopwords),
  but I think text search configuration names is better than file names.
  However, it requires system catalog access but we cannot access any
  catalog at the moment of preloading. If config-name-based setting is
  difficult, we need to write docs about where we can get the dict names
  to be preloaded instead. (from \dFd+ ?)

- Do we need to support multiple preloaded dicts? I think dict_preload.*
  should accept a list of items to be loaded. GUC_LIST_INPUT will be a help.

- Server doesn't start when I added dict_preload to
  shared_preload_libraries and didn't add any custom parameters.
    FATAL:  missing AffFile parameter
  But server should start with no effects or print WARNING messages
  for "no dicts are preloaded" in such case.

- We could replace simple_alloc() to a new MemoryContextMethods that
  doesn't support pfree() but has better performance. It doesn't look
  ideal for me to implement simple_alloc() on the top of palloc().

=== Implementation ===
I'm sure that your patch is WIP, but I'll note some issues just in case.

- We need Makefile for contrib/dict_preload.

- mmap() is not always portable. We should check the availability
  in configure, and also have an alternative implementation for Win32.


Regards,
---
Takahiro Itagaki
NTT Open Source Software Center



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to