[Pgpool-hackers] patch "Caching query results in pgpool-II"
Masanori Yamazaki
m.yamazaki23 at gmail.com
Tue Jun 28 17:54:27 UTC 2011
> 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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://pgfoundry.org/pipermail/pgpool-hackers/attachments/20110629/55fbfe87/attachment-0001.html>
More information about the Pgpool-hackers
mailing list