Обсуждение: Pre-v11 appearances of the word "procedure" in v11 docs
I noticed that the word "procedure" appears in at least a few places in the v11 docs as a not-quite-apt synonym of "function". For example, https://www.postgresql.org/docs/11/static/plpgsql-trigger.html talks about "PL/pgSQL Trigger Procedures" which are actually all functions in practice. I think that this has the potential to confuse users, since, of course, a function is a distinct variety of object to a procedure in v11. Tightening up the wording seems like a good idea. I'm not sure if this was discussed already, but didn't find anything during a quick search. -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes: > I noticed that the word "procedure" appears in at least a few places > in the v11 docs as a not-quite-apt synonym of "function". For example, > https://www.postgresql.org/docs/11/static/plpgsql-trigger.html talks > about "PL/pgSQL Trigger Procedures" which are actually all functions > in practice. Agreed, we'd better stop being cavalier about whether that's an exact synonym or not. regards, tom lane
On Tue, Aug 14, 2018 at 10:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Agreed, we'd better stop being cavalier about whether that's an > exact synonym or not. I made an open item for this. -- Peter Geoghegan
What should we do with the use of the keyword PROCEDURE in the CREATE OPERATOR and CREATE TRIGGER syntaxes? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> What should we do with the use of the keyword PROCEDURE in the CREATE
> OPERATOR and CREATE TRIGGER syntaxes?
We're kinda stuck with that. We could add FUNCTION as a preferred
synonym, perhaps, but I doubt that'd really be worth the trouble.
regards, tom lane
On Tue, Aug 14, 2018 at 1:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > We're kinda stuck with that. We could add FUNCTION as a preferred > synonym, perhaps, but I doubt that'd really be worth the trouble. Seems sufficient to note in the relevant docs that it's a legacy thing. -- Peter Geoghegan
On 14/08/18 22:39, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> What should we do with the use of the keyword PROCEDURE in the CREATE >> OPERATOR and CREATE TRIGGER syntaxes? > > We're kinda stuck with that. We could add FUNCTION as a preferred > synonym, perhaps, but I doubt that'd really be worth the trouble. If someone were to waste their time doing it, would we want FUNCTION or ROUTINE? -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attached are my proposed patches. The first is the documentation change, which basically just substitutes the words, with some occasional rephrasing. And then patches to extend the syntaxes of CREATE OPERATOR, CREATE TRIGGER, and CREATE EVENT TRIGGER to accept FUNCTION in place of PROCEDURE. I decided to do that because with the adjustments from the first patch, the documentation had become comically inconsistent in some places and it was easier to just fix the underlying problem than to explain the reasons for the inconsistencies everywhere. I didn't go around change all the commands in contrib modules etc. to keep the patch size under control. This could perhaps be done later. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Fri, Aug 17, 2018 at 7:15 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Attached are my proposed patches. I take it that you propose all 3 for backpatch to v11? -- Peter Geoghegan
> On Aug 17, 2018, at 10:15 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> Attached are my proposed patches. The first is the documentation
> change, which basically just substitutes the words, with some occasional
> rephrasing. And then patches to extend the syntaxes of CREATE OPERATOR,
> CREATE TRIGGER, and CREATE EVENT TRIGGER to accept FUNCTION in place of
> PROCEDURE. I decided to do that because with the adjustments from the
> first patch, the documentation had become comically inconsistent in some
> places and it was easier to just fix the underlying problem than to
> explain the reasons for the inconsistencies everywhere. I didn't go
> around change all the commands in contrib modules etc. to keep the patch
> size under control. This could perhaps be done later.
Thank for going through this, I know it’s an arduous task.
I reviewed the patches, here are my comments.
Patch 1
======
Built ok. All tests passed. Manual checks passed. I would recommend
backpatching this to 11.
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index d6688e13f4..abe67fa50e 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -19,7 +19,7 @@ <title>Overview</title>
<itemizedlist>
<listitem>
<para>
- can be used to create functions and trigger procedures,
+ can be used to create functions and triggers,
</para>
Perhaps: “can be used to create functions used in queries and triggers” or
just “can be used to create functions and trigger functions.”
Similarly for:
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
index 01f6207d36..97906a67df 100644
--- a/doc/src/sgml/pltcl.sgml
+++ b/doc/src/sgml/pltcl.sgml
@@ -16,7 +16,7 @@ <title>PL/Tcl - Tcl Procedural Language</title>
<productname>PostgreSQL</productname> database system
that enables the <ulink url="http://www.tcl.tk/">
Tcl language</ulink> to be used to write functions and
- trigger procedures.
+ triggers.
</para>
I’d be more specific, as the trigger executes the trigger function.
Patch 2
======
Built ok. Tests passed. Manual tests passed. Did a few pg_dump / restore
scenarios as well.
Code looked ok to me, though I’d consider adding a comment in
“operatorcmds.c" about how function / procedure are interchangeable,
which is why both outcomes are identical:
@@ -120,6 +120,8 @@ DefineOperator(List *names, List *parameters)
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("SETOF type not allowed for operator argument")));
}
+ else if (strcmp(defel->defname, "function") == 0)
+ functionName = defGetQualifiedName(defel);
else if (strcmp(defel->defname, "procedure") == 0)
functionName = defGetQualifiedName(defel);
else if (strcmp(defel->defname, "commutator") == 0)
I can also make arguments both ways about backpatching Patch 2 to v11. It
would be less confusing for users (“I have to create a procedure to create an
operator??”), though the type of user who is creating a custom operator is more
sophisticated.
I think it would be better to backpatch to v11, but I can be convinced otherwise
by a strong argument.
Patch 3
======
I was trying to determine if changing to “EXECUTE FUNCTION” would be part
of the SQL standard, which with some (poorly done?) searching it looks like it is.
Before commenting further, this patch would give me the most cause-for-pause about
backporting to v11 at this point as triggers are highly utilized. Like Patch 2 I can
make arguments for both, but I need to think about it a bit more.
Anyway, builds ok, tests passed.
When manually testing, I noticed pg_dump still outputs “EXECUTE PROCEDURE”
as part of the trigger syntax. Here is the example I used to produce it:
CREATE TABLE a (
x int GENERATED BY DEFAULT AS iDENTITY PRIMARY KEY,
y int NOT NULL
);
CREATE TABLE b (
a_id int PRIMARY KEY REFERENCES a (x) UNIQUE,
created_at timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP
);
CREATE OR REPLACE FUNCTION a_insrt ()
RETURNS trigger
AS $$
BEGIN
IF TG_OP = 'INSERT' THEN
INSERT INTO b VALUES (NEW.x, CURRENT_TIMESTAMP);
RETURN NEW;
END IF;
END
$$ LANGUAGE plpgsql;
CREATE TRIGGER a_insrt
AFTER INSERT ON a
FOR EACH ROW
EXECUTE FUNCTION a_insrt();
Other than that, the patch looked ok to me.
Thanks again for pulling all of this together,
Jonathan
Вложения
On 17/08/2018 21:57, Peter Geoghegan wrote: > On Fri, Aug 17, 2018 at 7:15 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> Attached are my proposed patches. > > I take it that you propose all 3 for backpatch to v11? yes -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 18/08/2018 19:37, Jonathan S. Katz wrote: > I reviewed the patches, here are my comments. Committed all three with some adjustments. Thanks. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On Aug 22, 2018, at 9:15 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > On 18/08/2018 19:37, Jonathan S. Katz wrote: >> I reviewed the patches, here are my comments. > > Committed all three with some adjustments. Thanks. Awesome, thanks! I removed the open item. Jonathan
Вложения
Some things from this thread were left for post-11; see attached patch. Specifically, this changes pg_dump and ruleutils.c to use the FUNCTION keyword instead of PROCEDURE in trigger and event trigger definitions, which was postponed because of the required catversion bump. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 04/02/2019 23:02, Peter Eisentraut wrote: > Some things from this thread were left for post-11; see attached patch. > > Specifically, this changes pg_dump and ruleutils.c to use the FUNCTION > keyword instead of PROCEDURE in trigger and event trigger definitions, > which was postponed because of the required catversion bump. done -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services