[Pgpool-hackers] patch "Caching query results in pgpool-II"
Masanori Yamazaki
m.yamazaki23 at gmail.com
Wed Jun 29 15:55:08 UTC 2011
> So this is my understanding regarding your progress. Please correct me
> if I am wrong.
That's right, from 1) to 10)
At present
7) All memory cache codes using memcached should be surrounded by
#ifdef USE_MEMCACHED ... #endif
Working
8) directive "memqcache_dir" is missing
Working
Besides
>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?
Coding
>> 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);
>> }
I corrected it. I think that "backend->info->key" is table oid.
Is it wrong?
>> 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().
Working
>> - The function should store md5 hash key + cache ids.
Coding.
>> 5) About cache key
>> Cache key should be md5(username+query_string+
database_name), not
>> just query_string
Coding done. I made new function "create_cachekey".
Args are two parameters, "query_string" and "backend(username and database
name)", and return md5(username+query_string+database_name).
> I saw many compiler wornings in your patches. Please fix them
> before posting patches(as long as you introduced).
I understood it well. I am correcting them.
Thank you.
2011/6/29 Tatsuo Ishii <ishii at sraoss.co.jp>
> 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
> >>>
> >>
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://pgfoundry.org/pipermail/pgpool-hackers/attachments/20110630/d1ea100e/attachment-0001.html>
More information about the Pgpool-hackers
mailing list