[pgpool-general: 8305] Re: Timestamp cast not cached
Avi Raboah
avi.raboah at gmail.com
Tue Jul 5 17:46:21 JST 2022
Thanks a lot.
Really appreciate it.
Avi
On Tue, 5 Jul 2022 at 11:10 Tatsuo Ishii <ishii at sraoss.co.jp> wrote:
> > 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
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.pgpool.net/pipermail/pgpool-general/attachments/20220705/66dee919/attachment.htm>
More information about the pgpool-general
mailing list