[Pgpool-hackers] patch "Caching query results in pgpool-II"

Tue Jun 28 23:35:41 UTC 2011

So this is my understanding regarding your progress. Please correct me
if I am wrong.

legend:

None: nothing is done
Coding: coding in progress
Working: coding done but not working as advertised
Done: coding done and working as advertised

1) Shared memory cache
   None
2) Cache data when extended protocol is used
   Coding(memcached), None(shmem)
3) Cache invalidation when receives DDL/DML related to query cache
   Coding
4) Cache should not be registered if transaction aborts
   None
5) Cache should not be registered if the table is temporary table
   None
6) configure does not recognize "--with-memcached" option.
   Done
7) All memory cache codes using memcached should be surrounded by
   #ifdef USE_MEMCACHED ... #endif
   None
8) directive "memqcache_dir" is missing
   None
9) Documentation patches are not included
   Coding
10) Patches for pgpool.conf.sample-master-slave
    pgpool.conf.sample-stream pgpool.conf.sample-replication are not
    included
	None

BTW I saw many compiler wornings in your patches. Please fix them
before posting patches(as long as you introduced).
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

From: Masanori Yamazaki <m.yamazaki23 at gmail.com>
Subject: Re: [Pgpool-hackers] patch "Caching query results in pgpool-II"
Date: Wed, 29 Jun 2011 02:54:27 +0900
Message-ID: <BANLkTi=B3E43dO6N9yJo363rpZTKq8envw at mail.gmail.com>

>> First of all I want to make sure that what are not included in the
>> patches:
> 
>> 1) Shared memory cache
>> 2) Cache data when extended protocol is used
>> 3) Cache invalidation when receives DDL/DML related to query cache
>> 4) Cache should not be registered if transaction aborts
>> 5) Cache should not be registered if the table is temporary table
> -6) configure does not recognize "--with-memcached" option. This part,
>   I will write a patch.
> thanks, I could compile.
> 
>> 7) All memory cache codes using memcached should be surrounded by
>>   #ifdef USE_MEMCACHED ... #endif
>> 8) directive "memqcache_dir" is missing
>> 9) Documentation patches are not included
>> 10) Patches for pgpool.conf.sample-master-slave
>        pgpool.conf.sample-stream pgpool.conf.sample-replication are not
>        included
> 
> Yes, these functions are not included in patch now.
> 1), 2) are halfway, so these functions don't work now.
> First of all, I want to complete memcached function.
> 
> On the other hand, functions which are already included in patch is the
> following.
> (These functions are not complete yet)
> - memcached cache
> - If a table is dropped or modified, related cache is deleted.
> - If a table definition is modified, related cache data is deleted.
> - If a schema or database is dropped, related cache data is deleted.
> - If memory cache entry life time passed, related cache data is deleted.
> - The result of SELECT using none IMMUTABLE functions is not cached.
> 
>> 1) No memory allocation error check in add_buf.
> I add the error check in add_buf function.
> 
>> 2) Excess memory allocation in add_buf. If it needs n byte, it allways
>>   allocate memqcache_total_size + n bytes. Why do we need extra
>>   memqcache_total_size?
> bug.I correct.
> 
>> 3) What is this code doing? "con" is never
>>   used. memqcache_total_size's 3rd arg is "oid", while you are
>>   passing a file descriptor.
>>        /* save the received result to buf for each kind */
>>        if (pool_config->memory_cache_enabled)
>>        {
>>                char* con;
>>                memqcache_register(kind, frontend, frontend->fd, p1, len1);
>>        }
> 
> Sorry, I forget delete "char* con",
> Running GDB, I saw in values of variables, structure such as "frontend",
> "backend" or "p1"....It is because I wanted to get tableoid, but don't
> understand how to get it. By the way, It was intended that 4th arg "p1" is
> "SELECT Results(=DataRow)" when message type is "D".
> 
>> 4) write_tableoid()
>>   - directory should be obtained from memqcache_dir directive
>>   - file extention ."txt" is not appropreate becauase the file does
>>     not need to be a text file.
>>   - writing md5 hash key as a string is wate of space. Just use fwrite().
> I am correcting them.
> 
>>   - The function should store md5 hash key + cache ids. Without cache
>>     ids, how can you invalidate cache entries?
> 
>> 5) About cache key
>>   Cache key should be md5(username+query_string+database_name), not
>>   just query_string. Also we need to store username at the same time
>>   to make sure that the cache data is not accessed by someone
>>   else. Otherwise we will have a security concern. See discussions in
>>   pgpool-hackers.
>>
> http://lists.pgfoundry.org/pipermail/pgpool-hackers/2011-June/000817.html
> 
> I correct the cache key format and frequently see it.
> There are some bad points in patch, therefore I correct them for the
> present.
> 
> 
> 2011/6/29 Masanori Yamazaki <m.yamazaki23 at gmail.com>
> 
>> Thank you very much for lots of useful advice.
>>
>>
>> > If the meaning of the flag "is_select_non_immutable_flag" is "true if
>> > the SELECT statement includes non IMMUTABLE function", the apparently
>> > the test is wrong.
>>
>> I mistook how to use non IMMUTABLE function, first I corrected code
>> regarding it.
>>
>>
>> > Did you turn on -d while testing your code?
>>
>> Yes, I do. But I mainly saw and check the values of variables running GDB.
>> for example.
>>
>> (gdb) b ProcessBackendResponse
>> Breakpoint 1 at 0x8089895: file pool_proto_modules.c, line 2092.
>> (gdb) n
>> Single stepping until exit from function __kernel_vsyscall,
>> which has no line number information.
>> 0x00272fbd in ___newselect_nocancel () from /lib/libc.so.6
>> (gdb) n
>> ...
>> (gdb) s
>>
>> Breakpoint 1, ProcessBackendResponse (frontend=0x8648640,
>> backend=0x8647e28,
>>     state=0xbfe4af74, num_fields=0xbfe4af7a) at pool_proto_modules.c:2092
>> 2092    {
>> (gdb) n
>> 2098            session_context = pool_get_session_context();
>> (gdb) p *backend->info
>> $2 = {database = "sample", '\000' <repeats 57 times>,
>>   user = "postgres", '\000' <repeats 23 times>, major = 3, minor = 0,
>>   pid = 1948712960, key = -604132037, counter = 1, create_time =
>> 1307271851,
>>   load_balancing_node = 0, connected = 1 '\001'}
>> (gdb)
>> ...
>>
>> I will take advantage of the debug log file or pool_debug from now on.
>> I refer to other proper comments and correct some comments like "@param",
>> and add more comments to code because it is true that there are few
>> comments in code.
>>
>>
>>
>>
>>
>>
>> 2011/6/28 Tatsuo Ishii <ishii at sraoss.co.jp>
>>
>>> > Other points after reviewing your code.
>>> >
>>> > First of all I want to make sure that what are not included in the
>>> > patches:
>>> >
>>> > 1) Shared memory cache
>>> > 2) Cache data when extended protocol is used
>>> > 3) Cache invalidation when receives DDL/DML related to query cache
>>> > 4) Cache should not be registered if transaction aborts
>>> > 5) Cache should not be registered if the table is temporary table
>>> > 6) configure does not recognize "--with-memcached" option. This part,
>>> >    I will write a patch.
>>>
>>> I have written patches for this. Attached is the patches *including*
>>> Masanori's patches against CVS HEAD (memqcache.[ch] are not included).
>>>
>>> Usage:
>>>
>>> ./configure --with-memcached=/usr/local
>>>
>>> Masanori, you need to do this:
>>>
>>> > 7) All memory cache codes using memcached should be surrounded by
>>> >    #ifdef USE_MEMCACHED ... #endif
>>> >
>>> > 8) directive "memqcache_dir" is missing
>>> >
>>> > Second, questionable codes in the patches:
>>> >
>>> > 1) No memory allocation error check in add_buf.
>>> >
>>> > 2) Excess memory allocation in add_buf. If it needs n byte, it allways
>>> >    allocate memqcache_total_size + n bytes. Why do we need extra
>>> >    memqcache_total_size?
>>> >
>>> > 3) What is this code doing? "con" is never
>>> >    used. memqcache_total_size's 3rd arg is "oid", while you are
>>> >    passing a file descriptor.
>>> >
>>> >       /* save the received result to buf for each kind */
>>> >       if (pool_config->memory_cache_enabled)
>>> >       {
>>> >               char* con;
>>> >               memqcache_register(kind, frontend, frontend->fd, p1,
>>> len1);
>>> >       }
>>> >
>>> > 4) write_tableoid()
>>> >
>>> >    - directory should be obtained from memqcache_dir directive
>>> >
>>> >    - file extention ."txt" is not appropreate becauase the file does
>>> >      not need to be a text file.
>>> >
>>> >    - writing md5 hash key as a string is wate of space. Just use
>>> fwrite().
>>> >
>>> >    - The function should store md5 hash key + cache ids. Without cache
>>> >      ids, how can you invalidate cache entries?
>>> >
>>> > 5) About cache key
>>> >
>>> >    Cache key should be md5(username+query_string+database_name), not
>>> >    just query_string. Also we need to store username at the same time
>>> >    to make sure that the cache data is not accessed by someone
>>> >    else. Otherwise we will have a security concern. See discussions in
>>> >    pgpool-hackers.
>>> >
>>> >
>>> http://lists.pgfoundry.org/pipermail/pgpool-hackers/2011-June/000817.html
>>> > --
>>> > Tatsuo Ishii
>>> > SRA OSS, Inc. Japan
>>> > English: http://www.sraoss.co.jp/index_en.php
>>> > Japanese: http://www.sraoss.co.jp
>>>
>>
>>