On Thu, Feb 9, 2023 at 9:55 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> 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);
> }
>
I have addressed this by changing similar occurrences accordingly for
instances with bool "present" - true and false.
This has also changed the test output and I have changed the test patch as well.
Mu changes are in patch 0003 and patch 0004.
regards,
Ajin Cherian
Fujitsu Australia
