Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Nov-12, Kyotaro Horiguchi wrote:
>> We can change the condition with "nargs <= 0" and it should return the
>> only element in clist. If the catalog is broken we may get
>> FUNCLOOKUP_AMBIGUOUS but it seems rather the correct behavior.
> Yeah, essentially this reverts 0a52d378b03b. Here's another version of
> this I wrote last night.
I really dislike the s/nargs < 0/nargs <= 0/ aspect of these proposals.
That's conflating two fundamentally different corner cases. We have
(1) for the nargs<0 case, there must not be more than one match,
else it's a user mistake (and not an unlikely one).
(2) for the nargs==0 case, if there's more than one match, we either
have corrupted catalogs or some kind of software bug.
The argument for changing the code like this seems to be "we'll
assume the possibility of a bug/corruption is so small that it's
okay to treat it as a user mistake". I reject that line of thinking.
And I especially reject conflating the two cases without bothering
to fix the adjacent comment that describes only case 1.
If we're going to revert 0a52d378b03b we should just treat the
problem straightforwardly. I was imagining
- if (memcmp(argtypes, clist->args, nargs * sizeof(Oid)) == 0)
+ /* if nargs==0, argtypes can be null; don't pass that to memcmp */
+ if (nargs == 0 ||
+ memcmp(argtypes, clist->args, nargs * sizeof(Oid)) == 0)
It's really stretching credulity to imagine that one more test-and-branch
in this loop costs anything worth noticing, especially compared to the
costs of having built the list to begin with. So I'm now feeling that
0a52d378b03b was penny-wise and pound-foolish.
Or, in other words: the OP's complaint here is basically that the
existing code isn't being straightforward about how it handles this
scenario. Changing to a different non-straightforward fix isn't
much of an improvement.
regards, tom lane
