I happened to notice that MINVFUNC in 0003 displays like this
"fmt": "MINVFUNC==%{type}T",
in some cases; this appears in the JSON that's emitted by the regression
tests at some point. How can we detect this kind of thing, so that
these mistakes become self-evident? I thought the intention of the
regress module was to run the deparsed code, so the syntax error should
have become obvious.
...
Oh, I see the problem. There are two 'fmt' lines for that clause (and
many others), one of which is used when the clause is not present. So
there's never a syntax error, because this one never expands other than
to empty.
AFAICS this defeats the purpose of the 'present' field. I mean, if the
clause is never to deparse, then why have it there in the first place?
If we want to have it, then it has to be correct.
I think we should design the code to avoid the repetition, because that
has an inherent risk of typo bugs and such. Maybe we should set forth
policy that each 'fmt' string should appear in the source code only
once. So instead of this
+ /* MINVFUNC */
+ if (OidIsValid(agg->aggminvtransfn))
+ tmp = new_objtree_VA("MINVFUNC=%{type}T", 1,
+ "type", ObjTypeObject,
+ new_objtree_for_qualname_id(ProcedureRelationId,
+ agg->aggminvtransfn));
+ else
+ {
+ tmp = new_objtree("MINVFUNC==%{type}T");
+ append_bool_object(tmp, "present", false);
+ }
we would have something like
tmp = new_objtree("MINVFUNC=%{type}T");
if (OidIsValid(agg->aggminvtransfn))
{
append_bool_object(tmp, "present", true);
append...(tmp, "type", new_objtree_for_qualname_id(ProcedureRelationId, agg->aggminvtransfn));
}
else
{
append_bool_object(tmp, "present", false);
}
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"No necesitamos banderas
No reconocemos fronteras" (Jorge González)
