Subject: "ALSO" keyword to "CREATE RULE" patch

From: Fabien COELHO <[email-masked]>
Date: Mon, 1 Mar 2004 09:10:54 +0100 (CET)

Dear patchers,

Please find attached a small patch to add an optionnal "ALSO" keyword
to the "CREATE RULE" syntax.

The "ALSO" keyword can be used where "INSTEAD" would be used,
to mean the opposite, i.e. the current default behavior of rules
which adds operations to the current one. IMHO, it makes the
intended behavior much clearer for the basic user (say, me;-).

CREATE RULE some_table_del AS
  ON DELETE TO some_table DO ALSO
  (
    DELETE FROM this_other_table WHERE id=old.id;
  );

Of course, the absence of the "ALSO" keyword preserves the previous
behavior... that is it behaves the same as with the "ALSO" keyword.

This patch was made against 7.4.1 with the "difforig" script
provided by postgresql.

It adds "ALSO" keyword in the parser code (two lines), fixes somehow the
documentation and sql help, and modifies four of the "RULE"
test cases to use this keyword instead of the default nothing-ness.

It validated for me with a "make check".

Have a nice day,

-- 
Fabien Coelho - [email-masked]

*** ./doc/src/sgml/rules.sgml.orig	Sun Feb 29 17:35:15 2004
--- ./doc/src/sgml/rules.sgml	Sun Feb 29 17:38:45 2004
***************
*** 873,879 ****
  
          <ListItem>
  	<Para>
! 	    They can be <literal>INSTEAD</> or not.
  	</Para>
  	</ListItem>
  
--- 873,879 ----
  
          <ListItem>
  	<Para>
! 	    They can be <literal>INSTEAD</> or <literal>ALSO</> (default).
  	</Para>
  	</ListItem>
  
***************
*** 904,910 ****
  <ProgramListing>
  CREATE RULE <replaceable>rule_name</> AS ON <replaceable>event</>
      TO <replaceable>object</> [WHERE <replaceable>rule_qualification</>]
!     DO [INSTEAD] [<replaceable>action</> | (<replaceable>actions</>) | NOTHING];
  </ProgramListing>
  
      in mind.
--- 904,910 ----
  <ProgramListing>
  CREATE RULE <replaceable>rule_name</> AS ON <replaceable>event</>
      TO <replaceable>object</> [WHERE <replaceable>rule_qualification</>]
!     DO [ALSO|INSTEAD] [<replaceable>action</> | (<replaceable>actions</>) | NOTHING];
  </ProgramListing>
  
      in mind.
***************
*** 920,926 ****
      Initially the query-tree list is empty.
      There can be zero (<literal>NOTHING</> key word), one, or multiple actions.
      To simplify, we will look at a rule with one action. This rule
!     can have a qualification or not and it can be <literal>INSTEAD</> or not.
  </Para>
  
  <Para>
--- 920,926 ----
      Initially the query-tree list is empty.
      There can be zero (<literal>NOTHING</> key word), one, or multiple actions.
      To simplify, we will look at a rule with one action. This rule
!     can have a qualification or not and it can be <literal>INSTEAD</> or <literal>ALSO</> (default).
  </Para>
  
  <Para>
***************
*** 937,943 ****
  
      <variablelist>
       <varlistentry>
!       <term>No qualification and not <literal>INSTEAD</></term>
        <listitem>
         <para>
          the query tree from the rule action with the original query
--- 937,943 ----
  
      <variablelist>
       <varlistentry>
!       <term>No qualification and <literal>ALSO</></term>
        <listitem>
         <para>
          the query tree from the rule action with the original query
***************
*** 957,963 ****
       </varlistentry>
  
       <varlistentry>
!       <term>Qualification given and not <literal>INSTEAD</></term>
        <listitem>
         <para>
          the query tree from the rule action with the rule
--- 957,963 ----
       </varlistentry>
  
       <varlistentry>
!       <term>Qualification given and <literal>ALSO</></term>
        <listitem>
         <para>
          the query tree from the rule action with the rule
***************
*** 980,986 ****
       </varlistentry>
      </variablelist>
  
!     Finally, if the rule is not <literal>INSTEAD</>, the unchanged original query tree is
      added to the list. Since only qualified <literal>INSTEAD</> rules already add the
      original query tree, we end up with either one or two output query trees
      for a rule with one action.
--- 980,986 ----
       </varlistentry>
      </variablelist>
  
!     Finally, if the rule is <literal>ALSO</>, the unchanged original query tree is
      added to the list. Since only qualified <literal>INSTEAD</> rules already add the
      original query tree, we end up with either one or two output query trees
      for a rule with one action.
***************
*** 1111,1117 ****
  </Para>
  
  <Para>
!     The rule is a qualified non-<literal>INSTEAD</> rule, so the rule system
      has to return two query trees: the modified rule action and the original
      query tree. In step 1, the range table of the original query is
      incorporated into the rule's action query tree. This results in:
--- 1111,1117 ----
  </Para>
  
  <Para>
!     The rule is a qualified <literal>ALSO</> rule, so the rule system
      has to return two query trees: the modified rule action and the original
      query tree. In step 1, the range table of the original query is
      incorporated into the rule's action query tree. This results in:
***************
*** 1190,1196 ****
     </para>
  
     <para>
!     That's it.  Since the rule is not <literal>INSTEAD</>, we also output the
      original query tree.  In short, the output from the rule system
      is a list of two query trees that correspond to these statements:
  
--- 1190,1196 ----
     </para>
  
     <para>
!     That's it.  Since the rule is <literal>ALSO</>, we also output the
      original query tree.  In short, the output from the rule system
      is a list of two query trees that correspond to these statements:
  
*** ./src/backend/parser/gram.y.orig	Sun Feb 29 17:32:48 2004
--- ./src/backend/parser/gram.y	Sun Feb 29 17:33:21 2004
***************
*** 327,333 ****
  
  /* ordinary key words in alphabetical order */
  %token <keyword> ABORT_P ABSOLUTE_P ACCESS ACTION ADD AFTER
! 	AGGREGATE ALL ALTER ANALYSE ANALYZE AND ANY ARRAY AS ASC
  	ASSERTION ASSIGNMENT AT AUTHORIZATION
  
  	BACKWARD BEFORE BEGIN_P BETWEEN BIGINT BINARY BIT
--- 327,333 ----
  
  /* ordinary key words in alphabetical order */
  %token <keyword> ABORT_P ABSOLUTE_P ACCESS ACTION ADD AFTER
! 	AGGREGATE ALL ALSO ALTER ANALYSE ANALYZE AND ANY ARRAY AS ASC
  	ASSERTION ASSIGNMENT AT AUTHORIZATION
  
  	BACKWARD BEFORE BEGIN_P BETWEEN BIGINT BINARY BIT
***************
*** 3529,3534 ****
--- 3529,3535 ----
  
  opt_instead:
  			INSTEAD									{ $$ = TRUE; }
+ 			| ALSO									{ $$ = FALSE; }
  			| /*EMPTY*/								{ $$ = FALSE; }
  		;
  
*** ./src/backend/parser/keywords.c.orig	Sun Feb 29 17:29:44 2004
--- ./src/backend/parser/keywords.c	Sun Feb 29 17:30:27 2004
***************
*** 38,43 ****
--- 38,44 ----
  	{"after", AFTER},
  	{"aggregate", AGGREGATE},
  	{"all", ALL},
+ 	{"also", ALSO},
  	{"alter", ALTER},
  	{"analyse", ANALYSE},		/* British spelling */
  	{"analyze", ANALYZE},
*** ./src/bin/psql/sql_help.h.orig	Sun Feb 29 17:42:06 2004
--- ./src/bin/psql/sql_help.h	Sun Feb 29 17:42:20 2004
***************
*** 159,165 ****
  
      { "CREATE RULE",
        N_("define a new rewrite rule"),
!       N_("CREATE [ OR REPLACE ] RULE name AS ON event\n    TO table [ WHERE condition ]\n    DO [ INSTEAD ] { NOTHING | command | ( command ; command ... ) }") },
  
      { "CREATE SCHEMA",
        N_("define a new schema"),
--- 159,165 ----
  
      { "CREATE RULE",
        N_("define a new rewrite rule"),
!       N_("CREATE [ OR REPLACE ] RULE name AS ON event\n    TO table [ WHERE condition ]\n    DO [ ALSO | INSTEAD ] { NOTHING | command | ( command ; command ... ) }") },
  
      { "CREATE SCHEMA",
        N_("define a new schema"),
*** ./src/test/regress/expected/rules.out.orig	Sun Feb 29 17:58:54 2004
--- ./src/test/regress/expected/rules.out	Sun Feb 29 17:59:10 2004
***************
*** 29,47 ****
  create table rtest_interface (sysname text, ifname text);
  create table rtest_person (pname text, pdesc text);
  create table rtest_admin (pname text, sysname text);
! create rule rtest_sys_upd as on update to rtest_system do (
  	update rtest_interface set sysname = new.sysname 
  		where sysname = old.sysname;
  	update rtest_admin set sysname = new.sysname 
  		where sysname = old.sysname
  	);
! create rule rtest_sys_del as on delete to rtest_system do (
  	delete from rtest_interface where sysname = old.sysname;
  	delete from rtest_admin where sysname = old.sysname;
  	);
! create rule rtest_pers_upd as on update to rtest_person do 
  	update rtest_admin set pname = new.pname where pname = old.pname;
! create rule rtest_pers_del as on delete to rtest_person do 
  	delete from rtest_admin where pname = old.pname;
  --
  -- Tables and rules for the logging test
--- 29,47 ----
  create table rtest_interface (sysname text, ifname text);
  create table rtest_person (pname text, pdesc text);
  create table rtest_admin (pname text, sysname text);
! create rule rtest_sys_upd as on update to rtest_system do also (
  	update rtest_interface set sysname = new.sysname 
  		where sysname = old.sysname;
  	update rtest_admin set sysname = new.sysname 
  		where sysname = old.sysname
  	);
! create rule rtest_sys_del as on delete to rtest_system do also (
  	delete from rtest_interface where sysname = old.sysname;
  	delete from rtest_admin where sysname = old.sysname;
  	);
! create rule rtest_pers_upd as on update to rtest_person do also
  	update rtest_admin set pname = new.pname where pname = old.pname;
! create rule rtest_pers_del as on delete to rtest_person do also
  	delete from rtest_admin where pname = old.pname;
  --
  -- Tables and rules for the logging test
*** ./src/test/regress/sql/rules.sql.orig	Sun Feb 29 17:43:59 2004
--- ./src/test/regress/sql/rules.sql	Sun Feb 29 17:45:47 2004
***************
*** 33,54 ****
  create table rtest_person (pname text, pdesc text);
  create table rtest_admin (pname text, sysname text);
  
! create rule rtest_sys_upd as on update to rtest_system do (
  	update rtest_interface set sysname = new.sysname 
  		where sysname = old.sysname;
  	update rtest_admin set sysname = new.sysname 
  		where sysname = old.sysname
  	);
  
! create rule rtest_sys_del as on delete to rtest_system do (
  	delete from rtest_interface where sysname = old.sysname;
  	delete from rtest_admin where sysname = old.sysname;
  	);
  
! create rule rtest_pers_upd as on update to rtest_person do 
  	update rtest_admin set pname = new.pname where pname = old.pname;
  
! create rule rtest_pers_del as on delete to rtest_person do 
  	delete from rtest_admin where pname = old.pname;
  
  --
--- 33,54 ----
  create table rtest_person (pname text, pdesc text);
  create table rtest_admin (pname text, sysname text);
  
! create rule rtest_sys_upd as on update to rtest_system do also (
  	update rtest_interface set sysname = new.sysname 
  		where sysname = old.sysname;
  	update rtest_admin set sysname = new.sysname 
  		where sysname = old.sysname
  	);
  
! create rule rtest_sys_del as on delete to rtest_system do also (
  	delete from rtest_interface where sysname = old.sysname;
  	delete from rtest_admin where sysname = old.sysname;
  	);
  
! create rule rtest_pers_upd as on update to rtest_person do also
  	update rtest_admin set pname = new.pname where pname = old.pname;
  
! create rule rtest_pers_del as on delete to rtest_person do also
  	delete from rtest_admin where pname = old.pname;
  
  --
*** ./doc/src/sgml/keywords.sgml.orig	Sun Feb 29 18:44:44 2004
--- ./doc/src/sgml/keywords.sgml	Sun Feb 29 18:45:40 2004
***************
*** 171,176 ****
--- 171,182 ----
      <entry>reserved</entry>
     </row>
     <row>
+     <entry><token>ALSO</token></entry>
+     <entry>non-reserved</entry>
+     <entry></entry>
+     <entry></entry>
+    </row>
+    <row>
      <entry><token>ALTER</token></entry>
      <entry>non-reserved</entry>
      <entry>reserved</entry>

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 3 Mar 2004 14:03:15 -0500 (EST)

Is ALSO part of the SQL standard?  I can't imagine it is because there
is no rule mention.  

I guess if were were coding from scratch, we could make the syntax
INSTEAD or ALSO, but at this point, I don't see adding ALSO as a useful
change.

---------------------------------------------------------------------------

> It validated for me with a "make check".

Content-Description: 

[ Attachment, skipping... ]

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 8: explain analyze is your friend

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  [email-masked]               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

---

From: Fabien COELHO <[email-masked]>
Date: Thu, 4 Mar 2004 09:40:04 +0100 (CET)

Dear Bruce,

> Is ALSO part of the SQL standard?  I can't imagine it is because there

As RULE is not in the SQL standard, ALSO is sure no part of it.

> I guess if we were coding from scratch, we could make the syntax INSTEAD
> or ALSO, but at this point, I don't see adding ALSO as a useful change.

It depends on what you mean by "useful". It is certainly non essential.
I just think it is "user-friendly".

It took me some time to understand that rules were appended by default.
With the "ALSO" keyword it would have been instantaneous.

When I first looked at the syntax with the "INSTEAD" which is optionnal
and without alternative, I thought that it was one of those useless
keywords that are allowed so as to make SQL more English-like, as the
"COLUMN" keyword in the ALTER TABLE syntax. You may argue that I did
not read the documentation in depth, but I'm not sure I'm the only one;-)

The change is very small (2 lines added, 2 lines changed), and upward
compatible, provided that the user has not used ALSO as an object name,
what seems rather unlikely IMVVHO.

Most of the patch deals with the documentation, which is rather ugly
because it keeps telling about "INSTEAD" vs "non-INSTEAD" rules, as there
is no name for the default behavior. I think "ALSO" fixes this issue as it
clarifies the explanations.

Have a nice day,

-- 
Fabien.

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 4 Mar 2004 09:40:13 -0500 (EST)

> clarifies the explanations.

Actually, our real problem is that the INSTEAD keyword wasn't mentioned
as part of instead rules (second paragraph), and there was no mention of
INSTEAD in the parameter section.  I just fixed this in current CVS:

	http://candle.pha.pa.us/main/writings/pgsql/sgml/sql-createrule.html

This should prevent misunderstanding in the future without adding an
additional keyword.

If we were starting from scratch, we would perhaps require either ALSO
or INSTEAD, but since we already have existing users, clarifiying it
seems better.

---

From: Tom Lane <[email-masked]>
Date: Thu, 04 Mar 2004 09:46:48 -0500

> clarifies the explanations.

Hmm ... I find that argument much more convincing than any of the others
...

Jan, what do you think?  You invented this command's syntax IIRC.

			regards, tom lane

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 4 Mar 2004 09:52:56 -0500 (EST)

> Jan, what do you think?  You invented this command's syntax IIRC.

I thought the syntax came from Berkeley.  We can add ALSO if folks like
it.  I can't think of cases where we have keywords for both on and off
behavior, and allow a default if the keyword is missing.

---

From: Fabien COELHO <[email-masked]>
Date: Thu, 4 Mar 2004 16:07:54 +0100 (CET)

> behavior, and allow a default if the keyword is missing.

ALTER TABLE ... DROP CONSTRAINT ... [ RESTRICT | CASCADE ]

CREATE TABLE ... [ WITH OIDS | WITHOUT OIDS ]

CREATE USER [ CREATEDB | NOCREATEDB ] ...

IMHO, from the language design point of view, it seems better if all
options have a name.

-- 
Fabien.

---

From: Fabien COELHO <[email-masked]>
Date: Thu, 4 Mar 2004 16:13:19 +0100 (CET)

> behavior, and allow a default if the keyword is missing.

ALTER TABLE ... DROP CONSTRAINT ... [ RESTRICT | CASCADE ] ;

CREATE TABLE ... [ WITH OIDS | WITHOUT OIDS ] ... ;

CREATE USER ... [ CREATEDB | NOCREATEDB ] ... ;

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 4 Mar 2004 10:36:22 -0500 (EST)

> IMHO, from the language design point of view, it seems better if all

Makes sense to me now.  No one liked the non-INSTEAD rule description,
for sure.

Shoot me the patch again and I will put in the the queue.  Thanks.

---

From: Fabien COELHO <[email-masked]>
Date: Thu, 4 Mar 2004 16:52:24 +0100 (CET)

> Shoot me the patch again and I will put in the the queue.  Thanks.

Please find attached (again) the patch I sent. It is against 7.4.1.
If necessary, I can redo the job against current head.

Have a nice day,

  
          <ListItem>
  	<Para>
  
--- 873,879 ----
  
          <ListItem>
  	<Para>
  
***************
*** 904,910 ****
  <ProgramListing>
  
      in mind.
--- 904,910 ----
  <ProgramListing>
  
      in mind.
***************
*** 920,926 ****
  
  <Para>
--- 920,926 ----
  
  <Para>
***************
*** 937,943 ****
  
      <variablelist>
       <varlistentry>
  
      <variablelist>
       <varlistentry>
  
       <varlistentry>
  
       <varlistentry>

  
  <Para>
  
  <Para>
  
     <para>
  
--- 1190,1196 ----
     </para>
  
     <para>

  

  
  opt_instead:
  			INSTEAD									{ $$ = TRUE; }
+ 			| ALSO									{ $$ = FALSE; }

      { "CREATE RULE",
  
      { "CREATE SCHEMA",
  
      { "CREATE RULE",
  
      { "CREATE SCHEMA",

---

From: Jan Wieck <[email-masked]>
Date: Thu, 04 Mar 2004 18:24:11 -0500

> Jan, what do you think?  You invented this command's syntax IIRC.

I did not. We inherited it from Postgres 4.2. If people think an ALSO 
keyword will clearify things, what about renaming the whole CREATE RULE 
into something along the line CREATE QUERY REWRITE MACRO?

Jan

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== [email-masked] #

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 4 Mar 2004 18:31:54 -0500 (EST)

> into something along the line CREATE QUERY REWRITE MACRO?

Are you saying you don't want ALSO added?

---

From: Jan Wieck <[email-masked]>
Date: Thu, 04 Mar 2004 19:08:19 -0500

> Are you saying you don't want ALSO added?

No, I am saying that CREATE RULE is so often misinterpreted to work like 
a trigger and when we explain that it is more like Macro expansion 
before the query is executed they understand better. CREATE RULE itself 
is just vague, with or without ALSO.

Jan

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 4 Mar 2004 19:19:52 -0500 (EST)

> is just vague, with or without ALSO.

True.

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 9 Mar 2004 00:05:18 -0500 (EST)

Patch applied.  Thanks.

Content-Description: 

> TIP 4: Don't 'kill -9' the postmaster