Re: Cache lookup errors with functions manipulation object addresses

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