Re: prokind column (was Re: [HACKERS] SQL procedures)
On 2/28/18 17:37, Peter Eisentraut wrote: > On 2/28/18 15:45, Tom Lane wrote: >> I have reviewed this patch and attach an updated version below. >> I've rebased it up to today, fixed a few minor errors, and adopted >> most of Michael's suggestions. Also, since I remain desperately >> unhappy with putting zeroes into prorettype, I changed it to not >> do that ;-) ... now procedures have VOIDOID as their prorettype, >> and it will be substantially less painful to allow them to return >> some other scalar result in future, should we wish to. I believe >> I've found all the places that were relying on prorettype == 0 as >> a substitute for prokind == 'p'. > > I have just posted "INOUT parameters in procedures", which contains some > of those same changes. So I think we're on the same page. I will work > on consolidating this. Committed this so far. More to come on touching this up. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: prokind column (was Re: [HACKERS] SQL procedures)
On Wed, Feb 28, 2018 at 05:37:11PM -0500, Peter Eisentraut wrote: > On 2/28/18 15:45, Tom Lane wrote: >> I have reviewed this patch and attach an updated version below. >> I've rebased it up to today, fixed a few minor errors, and adopted >> most of Michael's suggestions. Also, since I remain desperately >> unhappy with putting zeroes into prorettype, I changed it to not >> do that ;-) ... now procedures have VOIDOID as their prorettype, >> and it will be substantially less painful to allow them to return >> some other scalar result in future, should we wish to. I believe >> I've found all the places that were relying on prorettype == 0 as >> a substitute for prokind == 'p'. > > I have just posted "INOUT parameters in procedures", which contains some > of those same changes. So I think we're on the same page. I will work > on consolidating this. Thanks Peter. I have read the patch set that Tom has posted here and hunted for other inconsistencies. It seems to me that you have spotted everything. The changes in ProcedureCreate() are nice. I was wondering as well if it would be worth checking the contents of pg_proc with prorettype = 0 in the regression tests to always make sure that this never happens... Until I saw that opr_sanity.sql was actually doing already that so procedures actually are breaking that check on HEAD. -- Michael signature.asc Description: PGP signature
Re: prokind column (was Re: [HACKERS] SQL procedures)
On 2/28/18 15:45, Tom Lane wrote: > I have reviewed this patch and attach an updated version below. > I've rebased it up to today, fixed a few minor errors, and adopted > most of Michael's suggestions. Also, since I remain desperately > unhappy with putting zeroes into prorettype, I changed it to not > do that ;-) ... now procedures have VOIDOID as their prorettype, > and it will be substantially less painful to allow them to return > some other scalar result in future, should we wish to. I believe > I've found all the places that were relying on prorettype == 0 as > a substitute for prokind == 'p'. I have just posted "INOUT parameters in procedures", which contains some of those same changes. So I think we're on the same page. I will work on consolidating this. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: prokind column (was Re: [HACKERS] SQL procedures)
On Tue, Feb 27, 2018 at 11:50:31AM -0500, Tom Lane wrote: > We support the various psql/describe.c features against old servers, > so it would be quite inconsistent for tab completion not to work > similarly. There are some gaps in that already, as per the other > thread, but we should clean those up not add more. I'll try to study this thread a bit more as I lack context. So I'll reply there. What I am scared of is that Tomas Munro has done a heroic effort to increase the readability of psql's tab completion during the dev cycle of 9.6. And it would be sad to reduce the quality of this code. -- Michael signature.asc Description: PGP signature
Re: prokind column (was Re: [HACKERS] SQL procedures)
Catalin Iacob writes: > On Tue, Feb 27, 2018 at 4:03 AM, Michael Paquier wrote: >> 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 > Breaking tab completion in new psql against old servers might be > acceptable as it's a fringe feature, but I don't think your > recommendation of matching versions is practical. Lots of people > manage multiple server versions and using the latest psql for all of > them is currently, as far as I know, a perfectly supported way of > doing that, getting new psql features and keeping compatibility. I > think it would be a pity to loose that. We support the various psql/describe.c features against old servers, so it would be quite inconsistent for tab completion not to work similarly. There are some gaps in that already, as per the other thread, but we should clean those up not add more. regards, tom lane
Re: prokind column (was Re: [HACKERS] SQL procedures)
On Tue, Feb 27, 2018 at 4:03 AM, Michael Paquier wrote: > 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 Breaking tab completion in new psql against old servers might be acceptable as it's a fringe feature, but I don't think your recommendation of matching versions is practical. Lots of people manage multiple server versions and using the latest psql for all of them is currently, as far as I know, a perfectly supported way of doing that, getting new psql features and keeping compatibility. I think it would be a pity to loose that.
Re: prokind column (was Re: [HACKERS] SQL procedures)
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 signature.asc Description: PGP signature
Re: prokind column (was Re: [HACKERS] SQL procedures)
Peter Eisentraut writes: > On 2/24/18 14:08, Tom Lane wrote: >> I took a quick look through this and noted a few small problems; the >> only one worth mentioning here is that you've broken psql tab completion >> for functions and aggregates when talking to a pre-v11 server. >> I don't think that's acceptable; however, since tab-complete.c has no >> existing provisions for adjusting its queries based on server version, >> it's not really clear what to do to fix it. I'm sure that's soluble >> (and I think I recall a recent thread bumping up against the same >> problem for another change), but it needs a bit more sweat. > The patches proposed in the thread "PATCH: psql tab completion for > SELECT" appear to add support for version-dependent tab completion, so > this could be resolved "later". 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. 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. regards, tom lane
Re: prokind column (was Re: [HACKERS] SQL procedures)
On 2/24/18 14:08, Tom Lane wrote: > I took a quick look through this and noted a few small problems; the > only one worth mentioning here is that you've broken psql tab completion > for functions and aggregates when talking to a pre-v11 server. > I don't think that's acceptable; however, since tab-complete.c has no > existing provisions for adjusting its queries based on server version, > it's not really clear what to do to fix it. I'm sure that's soluble > (and I think I recall a recent thread bumping up against the same > problem for another change), but it needs a bit more sweat. The patches proposed in the thread "PATCH: psql tab completion for SELECT" appear to add support for version-dependent tab completion, so this could be resolved "later". -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: prokind column (was Re: [HACKERS] SQL procedures)
On 2/25/18, Tom Lane wrote: > We need a plan for when/how to apply this, along with the proposed > bootstrap data conversion patch, which obviously conflicts with it > significantly. The bulk changes in the bootstrap data patch are scripted rather than patched, so the prokind patch will pose little in the way of conflicts. I can't verify this just yet since Peter's second patch doesn't apply for me against c4ba1bee68ab. Also, as of version 7 my patch left out default values and human-readable oids, since I wanted to get the new generated headers reviewed and up to project standards first. Since I'll likely have to adjust the patches for those features anyway, there's plenty of room for me to adjust to the changes to pg_proc.h as well. > My thought here is that the data conversion patch is going to break > basically every pending patch that touches src/include/catalog/, > so we ought to apply it at a point where that list of patches is short > and there's lots of time for people to redo them. Hence, end of the > dev cycle is the right time. I agree. -John Naylor
Re: prokind column (was Re: [HACKERS] SQL procedures)
Peter Eisentraut writes: > Here is this patch updated. The client changes are now complete and all > the tests pass. I have also rolled back the places where the code used > prorettype to detect procedures and replaced this by the new facility. I took a quick look through this and noted a few small problems; the only one worth mentioning here is that you've broken psql tab completion for functions and aggregates when talking to a pre-v11 server. I don't think that's acceptable; however, since tab-complete.c has no existing provisions for adjusting its queries based on server version, it's not really clear what to do to fix it. I'm sure that's soluble (and I think I recall a recent thread bumping up against the same problem for another change), but it needs a bit more sweat. We need a plan for when/how to apply this, along with the proposed bootstrap data conversion patch, which obviously conflicts with it significantly. Looking through the stuff pending in next month's commitfest, we are fortunate in that only a few of those patches seem to need to touch pg_proc.h at all, and none have really large deltas AFAICS (I might've missed something though). I propose proceeding as follows: 1. Get this patch in as soon as we can resolve its remaining weak spots. That will force rebasing of pending patches that touch pg_proc.h, but I don't think it'll be painful, since the needed changes are pretty small and obvious. 2. During the March commitfest, adjust the bootstrap data conversion patch to handle this change, and review it generally. 3. At the end of the 'fest, or whenever we've dealt with all other patches that need to touch the bootstrap source data, apply the data conversion patch. My thought here is that the data conversion patch is going to break basically every pending patch that touches src/include/catalog/, so we ought to apply it at a point where that list of patches is short and there's lots of time for people to redo them. Hence, end of the dev cycle is the right time. regards, tom lane
