[pgpool-hackers: 3759] Re: Using volatile property instead of black_function_list

[Hou, Zhijie]

Hi Ishii

> Hi Hou,
> 
> >> So I try to add a formatted flag in sql comment to avoid this.
> >> A little change in your patch.
> >
> > Unfortunately this will not work. If you do not allow snprintf in
> > pool_search_relcache, it could use wrong SQL. For example we have
> > following query when we call pool_create_relcache:
> >
> > /*FORMATTED*/SELECT count(*) FROM pg_catalog.pg_proc AS p,
> > pg_catalog.pg_namespace AS n WHERE p.proname = '%s' AND n.oid =
> > p.pronamespace AND n.nspname = 'public' AND p.provolatile = 'v"
> >
> > Next pool_search_relcache() may be called with public.f1.
> > pool_search_relcache() will find a cache entry for public.%s and
> > returns the result for public.f1.
> >
> > Also my patch was wrong. Suppose we have below at the when relcache is
> > first created:
> >
> > SELECT count(*) FROM pg_catalog.pg_proc AS p, pg_catalog.pg_namespace
> > AS n WHERE p.proname = 'f1' AND n.oid = p.pronamespace AND n.nspname =
> > 'public' AND p.provolatile = 'v"
> >
> > Next pool_search_relcache() may be called with public.f2.
> > pool_search_relcache() will find a cache entry for
> > public.f1 and returns the result for public.f2.
> >
> > Let me think how to solve these problems.
> 
> I have come up with attached patches. Only
> 0001-check-writing-function-using-volatility.patch has been changed.
> 
> My idea is, let SQL template passed to pool_create_relache be "%s", and
> pass the actual SQL string created by function_volatile_property to "table"
> parameter of pool_search_relcache. So practically formatting using
> snprintf happens only once in function_volatile_property. This way, we can
> handle function whose name includes "%" such as "%s".
> 
> What do you think?

I think it works.

And I hava some comments on the previous patch which use FORMATTED flag.

Currently in function pool_search_relcache, first we use the function(table) name to match the right cache.

		if (strcasecmp(relcache->cache[i].dbname, dbname) == 0 &&
			strcasecmp(relcache->cache[i].relname, table) == 0)

if cache does not exists, we use "relcache->sql" to check the system catalog.

		snprintf(query, sizeof(query), relcache->sql, table);

In function is_immutable_function, we change the "relcache->sql" every time we call is_immutable_function.
(when relcache has not been inited, we change local variables "query", but "relcache->sql" will be indirectly modified with the local 
variables "query" in pool_create_relcache)

		if(!relcache) 
		{
			snprintf(query, sizeof(query), IS_STABLE_FUNCTION_QUERY, 
					"=", (char *) lsecond(names), (char *) linitial(names)); 
		}
		else 
		{
			snprintf(relcache->sql, sizeof(relcache->sql), IS_STABLE_FUNCTION_QUERY, 
					"=", (char *) lsecond(names), (char *) linitial(names)); 

		}

So ,I think it did use the correct sql, and the result is correct, what do you think?

If both methods work, may be we'd better use the shorter cache key?

Best regards.
houzj