Re: Declarative partitioning vs. sql_inheritance

Started by Robert Haasabout 9 years ago15 messages
#1Robert Haas
robertmhaas@gmail.com

On Thu, Dec 15, 2016 at 10:40 AM, Dmitry Ivanov <d.ivanov@postgrespro.ru> wrote:

Hi everyone,

Looks like "sql_inheritance" GUC is affecting partitioned tables:

explain (costs off) select * from test;
QUERY PLAN ------------------------------
Append
-> Seq Scan on test
-> Seq Scan on test_1
-> Seq Scan on test_2
-> Seq Scan on test_1_1
-> Seq Scan on test_1_2
-> Seq Scan on test_1_1_1
-> Seq Scan on test_1_2_1
(8 rows)

set sql_inheritance = off;

explain (costs off) select * from test;
QUERY PLAN ------------------
Seq Scan on test
(1 row)

I might be wrong, but IMO this should not happen. Queries involving update,
delete etc on partitioned tables are basically broken. Moreover, there's no
point in performing such operations on a parent table that's supposed to be
empty at all times.

An earlier version of Amit's patches tried to handle this by forcing
sql_inheritance on for partitioned tables, but it wasn't
well-implemented and I don't see the point anyway. Sure, turning off
sql_inheritance off for partitioned tables produces stupid results.
But turning off sql_inheritance for inheritance hierarchies also
produces stupid results. If we were going to do anything about this,
my vote would be to remove sql_inheritance.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#1)

Robert Haas <robertmhaas@gmail.com> writes:

An earlier version of Amit's patches tried to handle this by forcing
sql_inheritance on for partitioned tables, but it wasn't
well-implemented and I don't see the point anyway. Sure, turning off
sql_inheritance off for partitioned tables produces stupid results.
But turning off sql_inheritance for inheritance hierarchies also
produces stupid results. If we were going to do anything about this,
my vote would be to remove sql_inheritance.

+1. If memory serves, we invented that GUC as a backwards-compatibility
hack, because once upon a time the default behavior was equivalent to
sql_inheritance = off. But that was a long time ago; a bit of digging
in the git history suggests we changed it in 2000. It's hard to believe
that anybody still relies on being able to turn it off.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3David Fetter
david@fetter.org
In reply to: Robert Haas (#1)

On Fri, Dec 16, 2016 at 11:05:21AM -0500, Robert Haas wrote:

On Thu, Dec 15, 2016 at 10:40 AM, Dmitry Ivanov <d.ivanov@postgrespro.ru> wrote:

Hi everyone,

Looks like "sql_inheritance" GUC is affecting partitioned tables:

[breaks literally everything]

I might be wrong, but IMO this should not happen. Queries involving update,
delete etc on partitioned tables are basically broken. Moreover, there's no
point in performing such operations on a parent table that's supposed to be
empty at all times.

An earlier version of Amit's patches tried to handle this by forcing
sql_inheritance on for partitioned tables, but it wasn't
well-implemented and I don't see the point anyway. Sure, turning
off sql_inheritance off for partitioned tables produces stupid
results. But turning off sql_inheritance for inheritance
hierarchies also produces stupid results. If we were going to do
anything about this, my vote would be to remove sql_inheritance.

+1

It occurs to me this probably isn't the only GUC that's basically just
a foot gun at this point.

Is 10 a good time to sweep and clear them?

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Robert Haas
robertmhaas@gmail.com
In reply to: David Fetter (#3)

On Fri, Dec 16, 2016 at 2:34 PM, David Fetter <david@fetter.org> wrote:

It occurs to me this probably isn't the only GUC that's basically just
a foot gun at this point.

Is 10 a good time to sweep and clear them?

We never make any progress trying to do these things "in bulk". If
you think there are other GUCs that need to be removed, start a thread
for each one, or closely related group, and let's talk about it on the
merits. On this thread, let's just decide whether or not to remove
sql_inheritance.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Robert Haas (#1)

On 12/16/16 11:05 AM, Robert Haas wrote:

If we were going to do anything about this,
my vote would be to remove sql_inheritance.

Go for it.

Let's also remove the table* syntax then.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6David Steele
david@pgmasters.net
In reply to: Robert Haas (#1)

On 12/16/16 11:05 AM, Robert Haas wrote:

If we were going to do anything about this,
my vote would be to remove sql_inheritance.

+1. This option is long past the intended shelf life.

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#5)

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 12/16/16 11:05 AM, Robert Haas wrote:

If we were going to do anything about this,
my vote would be to remove sql_inheritance.

Go for it.

Let's also remove the table* syntax then.

Meh --- that might break existing queries, to what purpose?

We certainly shouldn't remove query syntax without a deprecation period.
I'm less concerned about that for GUCs.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#7)
1 attachment(s)

On Fri, Dec 16, 2016 at 7:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 12/16/16 11:05 AM, Robert Haas wrote:

If we were going to do anything about this,
my vote would be to remove sql_inheritance.

Go for it.

Let's also remove the table* syntax then.

Meh --- that might break existing queries, to what purpose?

We certainly shouldn't remove query syntax without a deprecation period.
I'm less concerned about that for GUCs.

I agree. Patch attached, just removing the GUC and a fairly minimal
amount of the supporting infrastructure.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

remove-sql-inheritance.patchtext/x-patch; charset=US-ASCII; name=remove-sql-inheritance.patchDownload
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0fc4e57..605910f 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7366,36 +7366,6 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
       </listitem>
      </varlistentry>
 
-     <varlistentry id="guc-sql-inheritance" xreflabel="sql_inheritance">
-      <term><varname>sql_inheritance</varname> (<type>boolean</type>)
-      <indexterm>
-       <primary><varname>sql_inheritance</> configuration parameter</primary>
-      </indexterm>
-      <indexterm><primary>inheritance</></>
-      </term>
-      <listitem>
-       <para>
-        This setting controls whether undecorated table references are
-        considered to include inheritance child tables.  The default is
-        <literal>on</>, which means child tables are included (thus,
-        a <literal>*</> suffix is assumed by default).  If turned
-        <literal>off</>, child tables are not included (thus, an
-        <literal>ONLY</literal> prefix is assumed).  The SQL standard
-        requires child tables to be included, so the <literal>off</> setting
-        is not spec-compliant, but it is provided for compatibility with
-        <productname>PostgreSQL</> releases prior to 7.1.
-        See <xref linkend="ddl-inherit"> for more information.
-       </para>
-
-       <para>
-        Turning <varname>sql_inheritance</> off is deprecated, because that
-        behavior has been found to be error-prone as well as contrary to SQL
-        standard.  Discussions of inheritance behavior elsewhere in this
-        manual generally assume that it is <literal>on</>.
-       </para>
-      </listitem>
-     </varlistentry>
-
      <varlistentry id="guc-standard-conforming-strings" xreflabel="standard_conforming_strings">
       <term><varname>standard_conforming_strings</varname> (<type>boolean</type>)
       <indexterm><primary>strings</><secondary>standard conforming</></>
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 7e1bc0e..d7117cb 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2529,11 +2529,9 @@ SELECT name, altitude
     WHERE altitude &gt; 500;
 </programlisting>
 
-   Writing <literal>*</> is not necessary, since this behavior is
-   the default (unless you have changed the setting of the
-   <xref linkend="guc-sql-inheritance"> configuration option).
-   However writing <literal>*</> might be useful to emphasize that
-   additional tables will be searched.
+   Writing <literal>*</> is not necessary, since this behavior is always
+   the default.  However, this syntax is still supported for
+   compatibility with older releases where the default could be changed.
   </para>
 
   <para>
diff --git a/doc/src/sgml/queries.sgml b/doc/src/sgml/queries.sgml
index 5cc6dbc..0f84c12 100644
--- a/doc/src/sgml/queries.sgml
+++ b/doc/src/sgml/queries.sgml
@@ -145,11 +145,9 @@ FROM <replaceable>table_reference</replaceable> <optional>, <replaceable>table_r
    <para>
     Instead of writing <literal>ONLY</> before the table name, you can write
     <literal>*</> after the table name to explicitly specify that descendant
-    tables are included.  Writing <literal>*</> is not necessary since that
-    behavior is the default (unless you have changed the setting of the <xref
-    linkend="guc-sql-inheritance"> configuration option).  However writing
-    <literal>*</> might be useful to emphasize that additional tables will be
-    searched.
+    tables are included.  There is no real reason to use this syntax any more,
+    because searching descendent tables is now always the default behavior.
+    However, it is supported for compatibility with older releases.
    </para>
 
    <sect3 id="queries-join">
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 9e62e00..ba1414b 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -54,7 +54,7 @@ LockTableCommand(LockStmt *lockstmt)
 	foreach(p, lockstmt->relations)
 	{
 		RangeVar   *rv = (RangeVar *) lfirst(p);
-		bool		recurse = interpretInhOption(rv->inhOpt);
+		bool		recurse = (rv->inhOpt == INH_YES);
 		Oid			reloid;
 
 		reloid = RangeVarGetRelidExtended(rv, lockstmt->mode, false,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7a574dc..075b68b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1183,7 +1183,7 @@ ExecuteTruncate(TruncateStmt *stmt)
 	{
 		RangeVar   *rv = lfirst(cell);
 		Relation	rel;
-		bool		recurse = interpretInhOption(rv->inhOpt);
+		bool		recurse = (rv->inhOpt == INH_YES);
 		Oid			myrelid;
 
 		rel = heap_openrv(rv, AccessExclusiveLock);
@@ -2654,7 +2654,7 @@ renameatt(RenameStmt *stmt)
 		renameatt_internal(relid,
 						   stmt->subname,		/* old att name */
 						   stmt->newname,		/* new att name */
-						   interpretInhOption(stmt->relation->inhOpt),	/* recursive? */
+						   (stmt->relation->inhOpt == INH_YES),	/* recursive? */
 						   false,		/* recursing? */
 						   0,	/* expected inhcount */
 						   stmt->behavior);
@@ -2806,7 +2806,7 @@ RenameConstraint(RenameStmt *stmt)
 		rename_constraint_internal(relid, typid,
 								   stmt->subname,
 								   stmt->newname,
-		 stmt->relation ? interpretInhOption(stmt->relation->inhOpt) : false,	/* recursive? */
+		 (stmt->relation && stmt->relation->inhOpt == INH_YES),	/* recursive? */
 								   false,		/* recursing? */
 								   0 /* expected inhcount */ );
 
@@ -3049,7 +3049,7 @@ AlterTable(Oid relid, LOCKMODE lockmode, AlterTableStmt *stmt)
 	CheckTableNotInUse(rel, "ALTER TABLE");
 
 	ATController(stmt,
-				 rel, stmt->cmds, interpretInhOption(stmt->relation->inhOpt),
+				 rel, stmt->cmds, (stmt->relation->inhOpt == INH_YES),
 				 lockmode);
 }
 
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 20e2dbd..b64f7c6 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -423,7 +423,7 @@ makeRangeVar(char *schemaname, char *relname, int location)
 	r->catalogname = NULL;
 	r->schemaname = schemaname;
 	r->relname = relname;
-	r->inhOpt = INH_DEFAULT;
+	r->inhOpt = INH_YES;
 	r->relpersistence = RELPERSISTENCE_PERMANENT;
 	r->alias = NULL;
 	r->location = location;
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 5e65fe7..601e22a 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -380,7 +380,7 @@ transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt)
 
 	/* set up range table with just the result rel */
 	qry->resultRelation = setTargetTable(pstate, stmt->relation,
-								  interpretInhOption(stmt->relation->inhOpt),
+								  (stmt->relation->inhOpt == INH_YES),
 										 true,
 										 ACL_DELETE);
 
@@ -2177,7 +2177,7 @@ transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt)
 	}
 
 	qry->resultRelation = setTargetTable(pstate, stmt->relation,
-								  interpretInhOption(stmt->relation->inhOpt),
+								  (stmt->relation->inhOpt == INH_YES),
 										 true,
 										 ACL_UPDATE);
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 2ed7b52..931bc9a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -28,12 +28,11 @@
  *	  current transaction and are just parsing commands to find the next
  *	  ROLLBACK or COMMIT.  If you make use of SET variables, then you
  *	  will do the wrong thing in multi-query strings like this:
- *			SET SQL_inheritance TO off; SELECT * FROM foo;
+ *			SET constraint_exclusion TO off; SELECT * FROM foo;
  *	  because the entire string is parsed by gram.y before the SET gets
  *	  executed.  Anything that depends on the database or changeable state
  *	  should be handled during parse analysis so that it happens at the
- *	  right time not the wrong time.  The handling of SQL_inheritance is
- *	  a good example.
+ *	  right time not the wrong time.
  *
  * WARNINGS
  *	  If you use a list, make sure the datum is a node so that the printing
@@ -11249,9 +11248,9 @@ join_qual:	USING '(' name_list ')'					{ $$ = (Node *) $3; }
 relation_expr:
 			qualified_name
 				{
-					/* default inheritance */
+					/* inheritance query, implicitly */
 					$$ = $1;
-					$$->inhOpt = INH_DEFAULT;
+					$$->inhOpt = INH_YES;
 					$$->alias = NULL;
 				}
 			| qualified_name '*'
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 751de4b..a96b3f9 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -229,30 +229,6 @@ setTargetTable(ParseState *pstate, RangeVar *relation,
 }
 
 /*
- * Simplify InhOption (yes/no/default) into boolean yes/no.
- *
- * The reason we do things this way is that we don't want to examine the
- * SQL_inheritance option flag until parse_analyze() is run.    Otherwise,
- * we'd do the wrong thing with query strings that intermix SET commands
- * with queries.
- */
-bool
-interpretInhOption(InhOption inhOpt)
-{
-	switch (inhOpt)
-	{
-		case INH_NO:
-			return false;
-		case INH_YES:
-			return true;
-		case INH_DEFAULT:
-			return SQL_inheritance;
-	}
-	elog(ERROR, "bogus InhOption value: %d", inhOpt);
-	return false;				/* keep compiler quiet */
-}
-
-/*
  * Given a relation-options list (of DefElems), return true iff the specified
  * table/result set should be created with OIDs. This needs to be done after
  * parsing the query string because the return value can depend upon the
@@ -437,7 +413,7 @@ transformTableEntry(ParseState *pstate, RangeVar *r)
 
 	/* We need only build a range table entry */
 	rte = addRangeTableEntry(pstate, r, r->alias,
-							 interpretInhOption(r->inhOpt), true);
+							 (r->inhOpt == INH_YES), true);
 
 	return rte;
 }
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a025117..946ba9e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -440,7 +440,6 @@ char	   *event_source;
 bool		row_security;
 bool		check_function_bodies = true;
 bool		default_with_oids = false;
-bool		SQL_inheritance = true;
 
 int			log_min_error_statement = ERROR;
 int			log_min_messages = WARNING;
@@ -1322,15 +1321,6 @@ static struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 	{
-		{"sql_inheritance", PGC_USERSET, COMPAT_OPTIONS_PREVIOUS,
-			gettext_noop("Causes subtables to be included by default in various commands."),
-			NULL
-		},
-		&SQL_inheritance,
-		true,
-		NULL, NULL, NULL
-	},
-	{
 		{"transform_null_equals", PGC_USERSET, COMPAT_OPTIONS_CLIENT,
 			gettext_noop("Treats \"expr=NULL\" as \"expr IS NULL\"."),
 			gettext_noop("When turned on, expressions of the form expr = NULL "
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 7f9acfd..46a7c17 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -605,7 +605,6 @@
 #lo_compat_privileges = off
 #operator_precedence_warning = off
 #quote_all_identifiers = off
-#sql_inheritance = on
 #standard_conforming_strings = on
 #synchronize_seqscans = on
 
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 65510b0..d11f112 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -45,8 +45,7 @@ typedef struct Alias
 typedef enum InhOption
 {
 	INH_NO,						/* Do NOT scan child tables */
-	INH_YES,					/* DO scan child tables */
-	INH_DEFAULT					/* Use current SQL_inheritance option */
+	INH_YES						/* DO scan child tables */
 } InhOption;
 
 /* What to do at commit time for temporary relations */
diff --git a/src/include/parser/parse_clause.h b/src/include/parser/parse_clause.h
index d04ce11..549bde4 100644
--- a/src/include/parser/parse_clause.h
+++ b/src/include/parser/parse_clause.h
@@ -19,7 +19,6 @@
 extern void transformFromClause(ParseState *pstate, List *frmList);
 extern int setTargetTable(ParseState *pstate, RangeVar *relation,
 			   bool inh, bool alsoSource, AclMode requiredPerms);
-extern bool interpretInhOption(InhOption inhOpt);
 extern bool interpretOidsOption(List *defList, bool allowOids);
 
 extern Node *transformWhereClause(ParseState *pstate, Node *clause,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 0bf9f21..66a3915 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -244,7 +244,6 @@ extern bool log_btree_build_stats;
 
 extern PGDLLIMPORT bool check_function_bodies;
 extern bool default_with_oids;
-extern bool SQL_inheritance;
 
 extern int	log_min_error_statement;
 extern int	log_min_messages;
#9Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Robert Haas (#8)

On 2016/12/17 10:40, Robert Haas wrote:

On Fri, Dec 16, 2016 at 7:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 12/16/16 11:05 AM, Robert Haas wrote:

If we were going to do anything about this,
my vote would be to remove sql_inheritance.

Go for it.

Let's also remove the table* syntax then.

Meh --- that might break existing queries, to what purpose?

We certainly shouldn't remove query syntax without a deprecation period.
I'm less concerned about that for GUCs.

I agree. Patch attached, just removing the GUC and a fairly minimal
amount of the supporting infrastructure.

+1 to removing the sql_inheritance GUC. The patch looks good to me.

Thanks,
Amit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Robert Haas (#8)

Robert Haas wrote:

On Fri, Dec 16, 2016 at 7:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 12/16/16 11:05 AM, Robert Haas wrote:

If we were going to do anything about this,
my vote would be to remove sql_inheritance.

Go for it.

Let's also remove the table* syntax then.

Meh --- that might break existing queries, to what purpose?

We certainly shouldn't remove query syntax without a deprecation period.
I'm less concerned about that for GUCs.

I agree. Patch attached, just removing the GUC and a fairly minimal
amount of the supporting infrastructure.

Any particular reason not to change inhOpt to be a simple boolean, and
remove the enum?

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Robert Haas
robertmhaas@gmail.com
In reply to: Alvaro Herrera (#10)

On Mon, Dec 19, 2016 at 11:48 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Any particular reason not to change inhOpt to be a simple boolean, and
remove the enum?

No, no particular reason. I thought about it, but I didn't really see
any advantage in getting rid of the typedef.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Robert Haas
robertmhaas@gmail.com
In reply to: Amit Langote (#9)

On Mon, Dec 19, 2016 at 12:25 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I agree. Patch attached, just removing the GUC and a fairly minimal
amount of the supporting infrastructure.

+1 to removing the sql_inheritance GUC. The patch looks good to me.

Great, committed. I realize just now that I forgot to credit anyone
as a reviewer, but hopefully nobody's going to mind that too much
considering this is a purely mechanical patch I wrote in 20 minutes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#12)

Robert Haas <robertmhaas@gmail.com> writes:

Great, committed. I realize just now that I forgot to credit anyone
as a reviewer, but hopefully nobody's going to mind that too much
considering this is a purely mechanical patch I wrote in 20 minutes.

Do you have any particular objection to taking the next step of removing
enum InhOption in favor of making inhOpt a bool? It seems to me that
stuff like

-       bool        recurse = interpretInhOption(rv->inhOpt);
+       bool        recurse = (rv->inhOpt == INH_YES);

just begs the question of why it's not simply

bool recurse = rv->inh;

Certainly a reader who did not know the history would be confused at
the useless-looking complexity.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#14Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#13)

On Fri, Dec 23, 2016 at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Great, committed. I realize just now that I forgot to credit anyone
as a reviewer, but hopefully nobody's going to mind that too much
considering this is a purely mechanical patch I wrote in 20 minutes.

Do you have any particular objection to taking the next step of removing
enum InhOption in favor of making inhOpt a bool? It seems to me that
stuff like

-       bool        recurse = interpretInhOption(rv->inhOpt);
+       bool        recurse = (rv->inhOpt == INH_YES);

just begs the question of why it's not simply

bool recurse = rv->inh;

Certainly a reader who did not know the history would be confused at
the useless-looking complexity.

No, not really. I don't feel like it's an improvement, but you and
Alvaro obviously do, so have at it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#15Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#14)

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Dec 23, 2016 at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Do you have any particular objection to taking the next step of removing
enum InhOption in favor of making inhOpt a bool?

No, not really. I don't feel like it's an improvement, but you and
Alvaro obviously do, so have at it.

OK, will do.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers