On 2023-08-17 05:07, Andy Fan wrote:
Thanks for the review, v9 attached!
From the earliest iterations of this patch, I seem to recall a couple of designs being considered: In one, the type-specific cast function would only be internally usable, would take a type oid as an extra parameter (supplied in the SupportRequestSimplify rewriting), and would have to be declared with some nonspecific return type; 'internal' was mentioned. The idea of an 'internal' return type with no 'internal' parameter was quickly and rightly shot down. But it would have seemed to me enough to address that objection by using 'internal' also in its parameter list. I could imagine a function declared with two 'internal' parameters, one understood to be a JsonbValue and one understood to be a type oid, and an 'internal' result, treated in the rewritten expression tree as binary-coercible to the desired result. Admittedly, I have not tried to implement that myself to see what unexpected roadblocks might exist on that path. Perhaps there are parts of that rewriting that no existing node type can represent? Someone more familiar with those corners of PostgreSQL may immediately see other difficulties I do not. But I have the sense that that approach was abandoned early, in favor of the current approach using user-visible polymorphic types, and supplying typed dummy constants for use in the resolution of those types, with a new function introduced to create said dummy constants, including allocation and input conversion in the case of numeric, just so said dummy constants can be passed into functions that have no use for them other than to call get_fn_expr_argtype to recover the type oid, which was the only thing needed in the first place. Compared to the initial direction I thought this was going, none of that strikes me as better. Nothing makes my opinion authoritative here, and there may indeed be reasons it is better, known to others more familiar with that code than I am. But it bugs me. If the obstacles to the earlier approach came down to needing a new type of expression node, something like "assertion of 'internal'-to-foo binary coercibility, vouched by a prosupport function", would that be a bad thing? Regards, -Chap
