[pgpool-hackers: 2816] Re: [New feature] Enable specifying SQL patterns lists that should not be load-balanced.

[Bo Peng]

Ishii-san,

Thank you for reviewing my patch.
I fixed that and attached the new patch.

On Wed, 23 May 2018 09:48:15 +0900 (JST)
Tatsuo Ishii <ishii at sraoss.co.jp> wrote:

> Peng,
> 
> Thanks for the new patch!
> 
> > +特定の SQL をプライマリノードに送信するように <xref linkend="guc-black-query-pattern-list">を設定します。SQL パターンをセミコロン区切りで指定します。マスタースレーブモードのみで動作します。
> 
> Please insert a new line between each sentence.
> 
> > +static char **get_list_from_string_regex_delim(const char *input, const char *delimi, int *n)
> > +{
> > +#ifndef POOL_PRIVATE
> > +    int j = 0;
> > +    char *output;
> > +    char *str;
> > +    char *buf;
> > +    char **tokens;
> > +	const int MAXTOKENS = 256;
> > +	*n = 0;
> > +
> > +	if (input == NULL || *input == '\0')
> > +		return NULL;
> > +
> > +	tokens = palloc(MAXTOKENS * sizeof(char *));
> > +	if (*(input + strlen(input) - 1) != *delimi)
> > +	{
> > +		str = palloc(strlen(input) + 1);
> > +		sprintf(str, "%s;", input);
> 
> Use of sprintf is not always recommended because there could be a
> chance for memory overrun. Instead, please use snprintf.
> 
> > --- a/src/include/pool_config.h
> > +++ b/src/include/pool_config.h
> > @@ -181,10 +181,11 @@ typedef struct {
> >  	char **reset_query_list;		/* comma separated list of queries to be issued at the end of session */
> >  	char **white_function_list;		/* list of functions with no side effects */
> >  	char **black_function_list;		/* list of functions with side effects */
> > +	char **black_query_pattern_list;	/* list of query patterns that should be sent to primary node */
> >  	char *log_line_prefix;			/* printf-style string to output at beginning of each log line */
> > -    int log_error_verbosity;		/* controls how much detail about error should be emitted */
> 
> Why do you touch irrelevant portion of the existing code? If your
> intention is fixing exiting indentation, I recommend you to do it as a
> separate commit.
> 
> > --- a/src/utils/pool_select_walker.c
> > +++ b/src/utils/pool_select_walker.c
> > @@ -203,6 +203,11 @@ int pattern_compare(char *str, const int type, const char *param_name)
> >  		lists_patterns = pool_config->lists_memqcache_table_patterns;
> >  		pattc = &pool_config->memqcache_table_pattc;
> >  
> > +	} else if (strcmp(param_name, "black_query_pattern_list") == 0)
> 
> This is not a recommended coding style in this project. Please write
> like below.
> 
> }
> else if (strcmp(param_name, "black_query_pattern_list") == 0)
> 
> > +	{
> > +		lists_patterns = pool_config->lists_query_patterns;
> > +		pattc = &pool_config->query_pattc;
> > +
> >  	} else {
> 
> This is also needed to be fixed by the same reason above. But since
> you did not code it, it's not your fault of course. It is up to you if
> you want to fix this as well.
> 
> >  		ereport(WARNING,
> >  				(errmsg("pattern_compare: unknown paramname %s", param_name)));
> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp
> 


-- 
Bo Peng <pengbo at sraoss.co.jp>
SRA OSS, Inc. Japan
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: black_query_v2.diff
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20180604/010edf35/attachment-0001.ksh>