From: | Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> |
---|---|
To: | Chapman Flack <chap(at)anastigmatix(dot)net> |
Cc: | jian he <jian(dot)universality(at)gmail(dot)com>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Extract numeric filed in JSONB more effectively |
Date: | 2023-08-22 03:14:48 |
Message-ID: | CAKU4AWrs4Pzajm2_tgtUTf=CWfDJEx=3h45Lhqg7tNOVZw5YxA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
(Just relalized this was sent to chap in private, resent it again).
On Mon, Aug 21, 2023 at 6:50 PM Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> wrote:
>
>
> On Mon, Aug 21, 2023 at 11:19 AM Chapman Flack <chap(at)anastigmatix(dot)net>
> wrote:
>
>> On 2023-08-20 21:31, Andy Fan wrote:
>> > Highlighting the user case of makeRelableType is interesting! But using
>> > the Oid directly looks more promising for this question IMO, it looks
>> > like:
>> > "you said we can put anything in this arg, so I put an OID const
>> > here",
>> > seems nothing is wrong.
>>
>> Perhaps one of the more senior developers will chime in, but to me,
>> leaving out the relabel nodes looks more like "all of PostgreSQL's
>> type checking happened before the SupportRequestSimplify, so nothing
>> has noticed that we rewrote the tree with mismatched types, and as
>> long as nothing crashes we sort of got away with it."
>>
>> Suppose somebody writes an extension to double-check that plan
>> trees are correctly typed. Or improves EXPLAIN to check a little more
>> carefully than it seems to. Omitting the relabel nodes could spell
>> trouble then.
>>
>> Or, someone more familiar with the code than I am might say "oh,
>> mismatches like that are common in rewritten trees, we live with it."
>> But unless somebody tells me that, I'm not believing it.
>>
>
> Well, this sounds long-lived. I kind of prefer to label it now. Adding
> the 3rd commit to relabel the arg and return value.
>
>
>> But I would say we have proved the concept of SupportRequestSimplify
>> for this task. :)
>>
>
> Yes, this is great!
>
>
>> Now, it would make me happy to further reduce some of the code
>> duplication between the original and the _type versions of these
>> functions. I see that you noticed the duplication in the case of
>> jsonb_extract_path, and you factored out jsonb_get_jsonbvalue so
>> it could be reused. There is also some duplication with object_field
>> and array_element.
>
>
Yes, compared with jsonb_extract_path, object_field and array_element
just have much less duplication, which are 2 lines and 6 lines separately.
> (Also, we may have overlooked jsonb_path_query
>> and jsonb_path_query_first as candidates for the source of the
>> cast. Two more candidates; five total.)
>>
>
I can try to add them at the same time when we talk about the
infrastruct, thanks for the hint!
>> Here is one way this could be structured. Observe that every one
>> of those five source candidates operates in two stages:
>>
>
> I'm not very excited with this manner, reasons are: a). It will have
> to emit more steps in ExprState->steps which will be harmful for
> execution. The overhead is something I'm not willing to afford.
> b). this manner requires more *internal*, which is kind of similar
> to "void *" in C. Could you explain more about the benefits of this?
>
> --
> Best Regards
> Andy Fan
>
--
Best Regards
Andy Fan
Attachment | Content-Type | Size |
---|---|---|
v10-0003-relabel-the-arg-and-resultvalue-with-INTERNALOID.patch | application/octet-stream | 6.1 KB |
v10-0001-optimize-casting-jsonb-to-a-given-type.patch | application/octet-stream | 31.7 KB |
v10-0002-convert-anyelement-to-internal.patch | application/octet-stream | 11.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | John Naylor | 2023-08-22 03:17:45 | Re: [PATCH] Add function to_oct |
Previous Message | Richard Guo | 2023-08-22 03:12:07 | Re: Oversight in reparameterize_path_by_child leading to executor crash |