[pgpool-general: 8304] Re: Timestamp cast not cached
Tatsuo Ishii
ishii at sraoss.co.jp
Tue Jul 5 17:10:47 JST 2022
> So the last patch have the fixed?
Yes.
> Do you know how postgres decide which cast function to use?
I just have a vague image:
It's done in the alalyze phase. In my understaing PostgreSQL does it by
using pg_cast system catalog plus I/O functions which are defined when
the types are defined. See src/backend/parser/analyze.c for more details.
> I can see that if you do the following (without the fix from yesterday) -
>
> Select to_timestamp(150000);
>
> The above query cached.
> Than I check in the pg_proc what function was running and I found that the
> function that running is float8_timestamptz
No. The function Pgpool-II chose was to_timestamp with 1 argument
(there is another to_timestamp function which accepts 2 args) . The
reason why to_timestamp(150000) is cached is, pg_proc has wrong
provolatile value for the function:
select provolatile, proargtypes from pg_proc where proname = 'to_timestamp' and proargtypes[0] = 701;
provolatile | proargtypes
-------------+-------------
i | 701
(1 row)
As you can see, the provolatile value of the function is 'i'
(immutable), which is clearly wrong because if the time zone is
changed, the value would be changed too. And Pgpool-II believes that
the function result can be cached.
I am going to report this to pgsql-hackers.
> Thanks,
>
> Avi.
>
> On Tue, 5 Jul 2022 at 7:24 Tatsuo Ishii <ishii at sraoss.co.jp> wrote:
>
>> Oops.
>>
>> The patch lacked a patch for pool_timestamp.c. Please disregard it.
>>
>> > Attached is a patch to not cache timestamptz and timetz cast (should
>> > be applied on top of the previous patch taken from the git repo).
>> >
>> >>> Agree but for some reason timestamptz cached and timestamp not.
>> >>
>> >> Yes.
>> >>
>> >>> I believe it’s happened because the selected cast function for
>> timestamp
>> >>> has provolatile = “s” and the selected cast function for timestamptz
>> has
>> >>> provolatile = “i”.
>> >>>
>> >>> Both castType have multiple cast function configured. Those functions
>> that
>> >>> are using time zone has provolatile “s” and those are not using time
>> zone
>> >>> have provolatile “i”.
>> >>>
>> >>> I think in order to fix this issue in a generic way. The solution
>> should
>> >>> understand which cast function are fetched by postgres when the client
>> ask
>> >>> for casting.
>> >>
>> >> PostgreSQL's parser (and Pgpool-II parser) do not work that way for
>> >> type cast. The cast is transformed to a type cast node, which does not
>> >> involve function call at all. So looking into pg_proc is not
>> >> possible. Probably I can add some codes to handle type cast node to
>> >> deal with the issue.
>> >>
>> >>> Thanks,
>> >>>
>> >>> Avi
>> >>>
>> >>> On Tue, 5 Jul 2022 at 0:38 Tatsuo Ishii <ishii at sraoss.co.jp> wrote:
>> >>>
>> >>>> provolatile column of pg_proc has been already considered in the
>> >>>> existing code. See the code block started with "if (IsA(node,
>> >>>> FuncCall))" in non_immutable_function_call_walker().
>> >>>>
>> >>>> > I thought the provolatile should be considered. Because I saw in
>> the
>> >>>> block
>> >>>> > code you disable today in order to fix the issue, a method which
>> called
>> >>>> > isSystemType.
>> >>>> >
>> >>>> > That function has condition which compare some value with the
>> pg_catalog
>> >>>> so
>> >>>> > I thought it’s could be related.
>> >>>> >
>> >>>> > Thanks,
>> >>>> >
>> >>>> > Avi
>> >>>> >
>> >>>> > On Mon, 4 Jul 2022 at 15:34 Tatsuo Ishii <ishii at sraoss.co.jp>
>> wrote:
>> >>>> >
>> >>>> >> provolatile column of pg_proc is not involved here. After
>> PostgreSQL
>> >>>> >> (Pgpool-II) parses "Select ‘2022-02-18
>> >>>> >> 07:00:00.006547+02’::timestamptz;" it produces a parse tree like
>> >>>> >> below. As you can see, there's no function call in it. It is
>> >>>> >> essentially a "type cast" node with type name "timestamptz".
>> >>>> >>
>> >>>> >> I think what Pgpool-II needs to do here is, finding a type cast
>> node
>> >>>> >> with its data type "timestamptz" (probably "timetz" should be
>> >>>> >> considered as well). If it finds, mark the SQL as "we should not
>> cache
>> >>>> >> it".
>> >>>> >>
>> >>>> >> test=# Select '2022-02-18 07:00:00.006547+02'::timestamptz;
>> >>>> >> 2022-07-04 20:16:13.571 JST [896725] LOG: raw parse tree:
>> >>>> >> 2022-07-04 20:16:13.571 JST [896725] DETAIL: (
>> >>>> >> {RAWSTMT
>> >>>> >> :stmt
>> >>>> >> {SELECT
>> >>>> >> :distinctClause <>
>> >>>> >> :intoClause <>
>> >>>> >> :targetList (
>> >>>> >> {RESTARGET
>> >>>> >> :name <>
>> >>>> >> :indirection <>
>> >>>> >> :val
>> >>>> >> {TYPECAST
>> >>>> >> :arg
>> >>>> >> {A_CONST
>> >>>> >> :val "\2022-02-18\ 07
>> >>>> >> :00
>> >>>> >> :00.006547+02"
>> >>>> >> :location 7
>> >>>> >> }
>> >>>> >> :typeName
>> >>>> >> {TYPENAME
>> >>>> >> :names ("timestamptz")
>> >>>> >> :typeOid 0
>> >>>> >> :setof false
>> >>>> >> :pct_type false
>> >>>> >> :typmods <>
>> >>>> >> :typemod -1
>> >>>> >> :arrayBounds <>
>> >>>> >> :location 40
>> >>>> >> }
>> >>>> >> :location 38
>> >>>> >> }
>> >>>> >> :location 7
>> >>>> >> }
>> >>>> >> )
>> >>>> >> :fromClause <>
>> >>>> >> :whereClause <>
>> >>>> >> :groupClause <>
>> >>>> >> :groupDistinct false
>> >>>> >> :havingClause <>
>> >>>> >> :windowClause <>
>> >>>> >> :valuesLists <>
>> >>>> >> :sortClause <>
>> >>>> >> :limitOffset <>
>> >>>> >> :limitCount <>
>> >>>> >> :limitOption 0
>> >>>> >> :lockingClause <>
>> >>>> >> :withClause <>
>> >>>> >> :op 0
>> >>>> >> :all false
>> >>>> >> :larg <>
>> >>>> >> :rarg <>
>> >>>> >> }
>> >>>> >> :stmt_location 0
>> >>>> >> :stmt_len 51
>> >>>> >> }
>> >>>> >> )
>> >>>> >>
>> >>>> >> Best reagards,
>> >>>> >> --
>> >>>> >> Tatsuo Ishii
>> >>>> >> SRA OSS, Inc. Japan
>> >>>> >> English: http://www.sraoss.co.jp/index_en.php
>> >>>> >> Japanese:http://www.sraoss.co.jp
>> >>>> >>
>> >>>> >> > Maybe it’s not a big issue.
>> >>>> >> >
>> >>>> >> > But more than that the way postgresql using casting function is
>> >>>> related
>> >>>> >> to
>> >>>> >> > the procvolatile.
>> >>>> >> >
>> >>>> >> > I believe the solution for pgpool should be related to which
>> casting
>> >>>> >> > function postgres using.
>> >>>> >> >
>> >>>> >> > In case the procvolatile for that cast function is “i” the query
>> >>>> should
>> >>>> >> be
>> >>>> >> > cached and if the procvolatile is “s” it shouldn’t.
>> >>>> >> >
>> >>>> >> > But maybe I am wrong.
>> >>>> >> >
>> >>>> >> > Do your magic.
>> >>>> >> >
>> >>>> >> > Thanks,
>> >>>> >> >
>> >>>> >> > Avi
>> >>>> >> >
>> >>>> >> > On Mon, 4 Jul 2022 at 14:36 Tatsuo Ishii <ishii at sraoss.co.jp>
>> wrote:
>> >>>> >> >
>> >>>> >> >> > Hi,
>> >>>> >> >> >
>> >>>> >> >> > I found some issue with timestamptz cast.
>> >>>> >> >> > See the following:
>> >>>> >> >> >
>> >>>> >> >> > 1.
>> >>>> >> >> >
>> >>>> >> >> > 2. Select ‘2022-02-18 07:00:00.006547+02’::timestamptz; ―>
>> will
>> >>>> >> retrieved
>> >>>> >> >> > from cache
>> >>>> >> >> >
>> >>>> >> >> > 3. Set time zone to ‘Some time zone’;
>> >>>> >> >> >
>> >>>> >> >> > 4. Select ‘2022-02-18 07:00:00.006547+02’::timestamptz; ―>
>> will
>> >>>> >> returned
>> >>>> >> >> > from cache but shouldn’t because the time zone has been
>> changed.
>> >>>> >> >> >
>> >>>> >> >> > I think the right behaviour should be that if we using cast
>> which
>> >>>> >> >> involved
>> >>>> >> >> > timezone like timestamptz or timetz these queries shouldn’t
>> saved
>> >>>> in
>> >>>> >> >> cache.
>> >>>> >> >> >
>> >>>> >> >> > What are your thoughts?
>> >>>> >> >>
>> >>>> >> >> You are right. Let me think about how to deal with the case.
>> >>>> >> >>
>> >>>> >> >> > Thanks,
>> >>>> >> >> >
>> >>>> >> >> > Avi.
>> >>>> >> >> >
>> >>>> >> >> > On Mon, 4 Jul 2022 at 11:41 Tatsuo Ishii <ishii at sraoss.co.jp>
>> >>>> wrote:
>> >>>> >> >> >
>> >>>> >> >> >> Glad to hear that :-)
>> >>>> >> >> >>
>> >>>> >> >> >> > My mistake it’s working like a charm :)
>> >>>> >> >> >> >
>> >>>> >> >> >> > On Mon, 4 Jul 2022 at 11:32 Avi Raboah <
>> avi.raboah at gmail.com>
>> >>>> >> wrote:
>> >>>> >> >> >> >
>> >>>> >> >> >> >> I added the patch and it still not working.
>> >>>> >> >> >> >> After your change the query
>> >>>> >> >> >> >> Select ‘2022-02-18 07:00:00.006547’::timestamp;
>> >>>> >> >> >> >>
>> >>>> >> >> >> >> Still not cached
>> >>>> >> >> >> >>
>> >>>> >> >> >> >> On Mon, 4 Jul 2022 at 11:21 Tatsuo Ishii <
>> ishii at sraoss.co.jp>
>> >>>> >> wrote:
>> >>>> >> >> >> >>
>> >>>> >> >> >> >>> Hi,
>> >>>> >> >> >> >>>
>> >>>> >> >> >> >>> > Hi,
>> >>>> >> >> >> >>> >
>> >>>> >> >> >> >>> > I saw your patch thanks for that.
>> >>>> >> >> >> >>>
>> >>>> >> >> >> >>> You are welcome.
>> >>>> >> >> >> >>>
>> >>>> >> >> >> >>> > One question, in order to enable the not_used block
>> you add,
>> >>>> >> Do I
>> >>>> >> >> >> need
>> >>>> >> >> >> >>> to
>> >>>> >> >> >> >>> > define this macro in the same page?
>> >>>> >> >> >> >>>
>> >>>> >> >> >> >>> No. You should *not* define NOT_USED symbol. Otherwise,
>> the
>> >>>> block
>> >>>> >> >> will
>> >>>> >> >> >> >>> be enabled, which is opposite to what the patch wants to
>> do.
>> >>>> >> >> >> >>>
>> >>>> >> >> >> >>> > For example:
>> >>>> >> >> >> >>> > #define NOT_USED
>> >>>> >> >> >> >>> > #ifdef NOT_USED
>> >>>> >> >> >> >>> > …
>> >>>> >> >> >> >>> > …
>> >>>> >> >> >> >>> > #endif
>> >>>> >> >> >> >>> >
>> >>>> >> >> >> >>> > Or I don’t need to add that ?
>> >>>> >> >> >> >>> >
>> >>>> >> >> >> >>> > Thanks,
>> >>>> >> >> >> >>> >
>> >>>> >> >> >> >>> > Avi.
>> >>>> >> >> >> >>> >
>> >>>> >> >> >> >>> > On Mon, 4 Jul 2022 at 9:54 Avi Raboah <
>> avi.raboah at gmail.com
>> >>>> >
>> >>>> >> >> wrote:
>> >>>> >> >> >> >>> >
>> >>>> >> >> >> >>> >> Awesome, thanks!
>> >>>> >> >> >> >>> >>
>> >>>> >> >> >> >>> >> On Mon, 4 Jul 2022 at 9:52 Tatsuo Ishii <
>> >>>> ishii at sraoss.co.jp>
>> >>>> >> >> wrote:
>> >>>> >> >> >> >>> >>
>> >>>> >> >> >> >>> >>> > Thanks a lot for your fast reaponse!
>> >>>> >> >> >> >>> >>> >
>> >>>> >> >> >> >>> >>> > Do you know when the fix will be available?
>> >>>> >> >> >> >>> >>>
>> >>>> >> >> >> >>> >>> I have just pushed the fix. It will available in the
>> next
>> >>>> >> >> scheduled
>> >>>> >> >> >> >>> >>> release (Aug 18).
>> >>>> >> >> >> >>> >>>
>> >>>> >> >> >> >>> >>> https://pgpool.net/mediawiki/index.php/Roadmap
>> >>>> >> >> >> >>> >>>
>> >>>> >> >> >> >>> >>> If you need patches, you can grab from the git
>> repository.
>> >>>> >> >> >> >>> >>>
>> >>>> >> >> >> >>> >>>
>> >>>> >> https://pgpool.net/mediawiki/index.php/Source_code_repository
>> >>>> >> >> >> >>> >>>
>> >>>> >> >> >> >>> >>> Best reagards,
>> >>>> >> >> >> >>> >>> --
>> >>>> >> >> >> >>> >>> Tatsuo Ishii
>> >>>> >> >> >> >>> >>> SRA OSS, Inc. Japan
>> >>>> >> >> >> >>> >>> English: http://www.sraoss.co.jp/index_en.php
>> >>>> >> >> >> >>> >>> Japanese:http://www.sraoss.co.jp
>> >>>> >> >> >> >>> >>>
>> >>>> >> >> >> >>> >>
>> >>>> >> >> >> >>>
>> >>>> >> >> >> >>
>> >>>> >> >> >>
>> >>>> >> >>
>> >>>> >>
>> >>>>
>> >> _______________________________________________
>> >> pgpool-general mailing list
>> >> pgpool-general at pgpool.net
>> >> http://www.pgpool.net/mailman/listinfo/pgpool-general
>>
More information about the pgpool-general
mailing list