From: | Dominique Devienne <ddevienne(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Identifying function-lookup failures due to argument name mismatches |
Date: | 2025-08-18 13:40:28 |
Message-ID: | CAFCRh-_zopD=KvZXyA62ox2iXoThjFj_E34Nq2gGjyHxa2QJgQ@mail.gmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Aug 14, 2025 at 9:18 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
> > Another thing not to like is that it seems like this is doing violence
> > to several APIs in exchange for not very much improvement in the error
> > messages. I feel like maybe we ought to be trying for more
> > specificity about additional cases, but I'm not very sure what else
> > to improve or what the API could look like.
>
> I couldn't quite let go of this, and after some thought I hit on the
> idea of making FuncnameGetCandidates pass back a bitmask of flags
> showing how far the match succeeded. This seems to work pretty
> nicely, allowing quite-detailed reports with only minimal added
> overhead or code restructuring. There's probably room for further
> improvement, but it has less of a whiff of "quick single-purpose
> hack". See draft commit message for more details.
As the original "complainer", I felt compelled to have a look at this
patch. Not to gauge its quality, mind you; I'm not qualified for
that. But to look at the flags, and the error messages. Below are my
notes observations:
From the use in code:
FGC_SCHEMA_MATCH; /* report that the schema is valid */
FGC_NAME_MATCH; /* we found a matching function name */
FGC_ARGNAMES_VALID; /* We found a fully-valid call using argument names */
FGC_ARGNAMES_MATCH; /* the function did match all the supplied argnames */
FGC_ARGNAMES_PLACED; /* call doesn't violate the rules for mixed notation */
FGC_ARGNAMES_ALL; /* call supplies all the required arguments */
FGC_VARIADIC_FAIL;
From the declaration:
+/*
+ * FuncnameGetCandidates also returns a bitmask containing these flags,
+ * which report on what it found or didn't find. They can help callers
+ * produce better error reports after a function lookup failure.
+ */
+#define FGC_SCHEMA_MATCH 0x0001 /* Found the explicitly-specified schema */
+#define FGC_NAME_MATCH 0x0002 /* Found a function name match */
+/* These bits relate only to calls using named or mixed arguments: */
+#define FGC_ARGNAMES_MATCH 0x0004 /* Found a func matching all argnames */
+#define FGC_ARGNAMES_PLACED 0x0008 /* Found argnames validly placed */
+#define FGC_ARGNAMES_ALL 0x0010 /* Found a function with no missing args */
+#define FGC_ARGNAMES_VALID 0x0020 /* Found a fully-valid use of argnames */
+/* These bits are actually filled by func_get_detail: */
+#define FGC_VARIADIC_FAIL 0x0040 /* Disallowed VARIADIC with named args */
I like the new messages in func_lookup_failure_details(). Very much
so in fact. BUT I still don't like the fallback "traditional"
message, because the way I read it, it fails to mention argument
*names* could be the reason for the lookup failure. Now maybe that's
moot, because of earlier messages. But I can't know that myself, thus
I'm still re-iterating my feeling on this. In my reading, "given
name" applies to "function" or "procedure" before it, and not to
"argument" after it. Thus I'd go with "and argument types or names".
One thing I find missing are flags about the actual syntax used by the
user, i.e.
is it schema-qualified?
does it use named arguments?
If some flags you've added Tom, are TRUE, then that's implied.
But is the converse true? i.e. if the flag is FALSE, can you know
whether named-args were used? (schema-qualified seems special, as
fails earlier, if I read you right). Because it could have some
bearing on the errors, no?
In any case, what you are proposing goes a LONG WAY to improve the
current situation, IMHO. Thank you VERY MUCH for pursuing this. And
I very much hope something like it goes through for v19 next year.
--DD
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2025-08-18 13:57:21 | Re: Retail DDL |
Previous Message | Tom Lane | 2025-08-18 13:34:34 | Re: Compilation issues for HASH_STATISTICS and HASH_DEBUG options |