(Just relalized this was sent to chap in private, resent it again).

On Mon, Aug 21, 2023 at 6:50 PM Andy Fan <zhihui.fan1...@gmail.com> wrote:

>
>
> On Mon, Aug 21, 2023 at 11:19 AM Chapman Flack <c...@anastigmatix.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: v10-0003-relabel-the-arg-and-resultvalue-with-INTERNALOID.patch
Description: Binary data

Attachment: v10-0001-optimize-casting-jsonb-to-a-given-type.patch
Description: Binary data

Attachment: v10-0002-convert-anyelement-to-internal.patch
Description: Binary data

Reply via email to