Re: Pre-v11 appearances of the word "procedure" in v11 docs

> On Aug 17, 2018, at 10:15 AM, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)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