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?