On Thu, Mar 19, 2020 at 08:48:58AM +0100, Daniel Gustafsson wrote:
> Taking a look at this to see if we can achieve closure on this long-running
> patchset. The goal of the patch is IMO a clear win for usability.
>
> The patchset applies with a bit of fuzzing and offsetting, it has tests (which
> pass) as well as the relevant documentation changes. I agree with the previous
> reviewers that the new shape of the test is better, so definitely +1 on that
> change. There are a lot of mechanic changes in this set, but AFAICT they cover
> all the bases.
Thanks. I hope it is helpful.
> Since the patch has been through a lot of review already there isn't a lot to
> say, only a few very minor things that stood out:
>
> * This exports the useful functionality of regoperatorout for use
> * in other backend modules. The result is a palloc'd string.
> format_operator_extended has this comment, but the code can now also return
> NULL and not just a palloc'd string.
>
> + if (!missing_ok)
> + {
> + elog(ERROR, "could not find tuple for cast %u",
> + object->objectId);
> + }
> The other (!missing_ok) blocks in this patch are not encapsulating the elog()
> with braces.
>
> I'm switching this to ready for committer,
The new FORMAT_TYPE_* flag still makes sense to me while reading this
code once again, as well as the extensibility of the new APIs for
operators and procedures. One doubt I still have is if we should
really change the signature of getObjectDescription() to include the
new missing_ok argument or if a new function should be introduced.
I'd rather keep only one function as, even if this is called in many
places in the backend, I cannot track down an extension using it, but
I won't go against Alvaro's will either if he thinks something
different as this is his original design and commit as of f8348ea3.
--
Michael