On Mon, Feb 26, 2018 at 02:03:19PM -0500, Tom Lane wrote:
> I'm not sure that other patch will get in; AFAICS it's incomplete and
> rather controversial. But I guess we could put this issue on the
> open-items list so we don't forget.
+1. We already know that we want to do a switch to prokind anyway,
while the other patch is still pending (don't think much about it
myself, I would just recommend users to use a version of psql matching
the one of the server instead of putting an extra load of maintenance
into psql for years to come). So I would recommend to push forward with
this one and fix what we know we have to fix, then request a rebase. We
gain nothing by letting things in a semi-broken state. I'll be happy to
help those folks rebase and/or write the patch to update psql's tab
completion for prokind as need be.
> Anyway, once the release dust settles, I'll try to do a proper review
> of this patch. It'd be good if we could get it in this week before
> the CF starts, so that any affected patches could be rebased.
Here is some input from me. I don't like either that prorettype is used
to determine if the object used is a procedure or a function. The patch
series is very sensitive to changes in pg_proc.h, still those apply
correctly when using bc1adc65 as base commit.
The original commit adding support for procedures had this diff in
clauses.c:
@@ -4401,6 +4401,7 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
if (funcform->prolang != SQLlanguageId ||
funcform->prosecdef ||
funcform->proretset ||
+ funcform->prorettype == InvalidOid ||
funcform->prorettype == RECORDOID ||
Perhaps this should be replaced with a check on prokind?
It seems to me that the same applies to fmgr_sql_validator(). In
information_schema.sql, type_udt_catalog uses a similar comparison so
this should have a comment about why using prorettype makes more sense.
In short for all those tings, it is fine to use prorettype when directly
working on the result type, like what is done in plperl and plpython.
For all the code paths working on the object type, I think that using
prokind should be the way to go.
getProcedureTypeDescription() should use an enum instead, complaining
with elog(ERROR) if the type found is something else?
I think that get_func_isagg should be dropped and replaced by
get_func_prokind.
--
Michael