Re: [HACKERS] view reloptions

2014-07-14 Thread Alvaro Herrera
Jaime Casanova wrote:

 Attached is a patch moving the reloptions of views into its own structure.
 i also created a view_reloptions() function in order to not use
 heap_reloptions() for views, but maybe that was too much?

No, that was part of what I was thinking too -- I have pushed this now.
Many thanks.

 i haven't seen the check_option_offset thingy yet, but i hope to take
 a look at that tomorrow.

I'm guessing you didn't get around to doing this.  I gave it a quick
look and my conclusion is that it requires somewhat more work than it's
worth.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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


Re: [HACKERS] view reloptions

2014-06-13 Thread Jaime Casanova
On Wed, Jun 11, 2014 at 2:46 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 I just noticed by chance that view relations are using StdRdOptions to
 allocate their reloptions.  I can't find any reason for this, other than
 someone failed to realize that they should instead have a struct defined
 of its own, just like (say) GIN indexes do.  Views using StdRdOptions is
 quite pointless, given that it's used for stuff like fillfactor and
 autovacuum, neither of which apply to views.

 9.2 added security_barrier to StdRdOptions, and 9.4 is now adding
 check_option_offset, which is a string reloption with some funny rules.

[...]
 2) it would mean that security_barrier would change for external code
 that expects StdRdOptions rather than, say, ViewOptions
 3) I don't have time to do the legwork before CF1 anyway

 If we don't change it now, I'm afraid we'll be stuck with using
 StdRdOptions for views for all eternity.

 (It's a pity I didn't become aware of this earlier in 9.4 cycle when I
 added the multixact freeze reloptions ... I could have promoted a patch
 back then.)


Attached is a patch moving the reloptions of views into its own structure.
i also created a view_reloptions() function in order to not use
heap_reloptions() for views, but maybe that was too much?

i haven't seen the check_option_offset thingy yet, but i hope to take
a look at that tomorrow.

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157
From 87ed78a4f276484a37917c417286a16082030f13 Mon Sep 17 00:00:00 2001
From: Jaime Casanova ja...@2ndquadrant.com
Date: Fri, 13 Jun 2014 01:10:24 -0500
Subject: [PATCH] Move reloptions from views into its own structure.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Per gripe from Álvaro Herrera.
---
 src/backend/access/common/reloptions.c |   42 ++-
 src/backend/commands/tablecmds.c   |9 +-
 src/include/access/reloptions.h|1 +
 src/include/utils/rel.h|   43 
 src/tools/pgindent/typedefs.list   |1 +
 5 files changed, 71 insertions(+), 25 deletions(-)

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 522b671..c7ad6f9 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -834,10 +834,12 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc, Oid amoptions)
 	{
 		case RELKIND_RELATION:
 		case RELKIND_TOASTVALUE:
-		case RELKIND_VIEW:
 		case RELKIND_MATVIEW:
 			options = heap_reloptions(classForm-relkind, datum, false);
 			break;
+		case RELKIND_VIEW:
+			options = view_reloptions(datum, false);
+			break;
 		case RELKIND_INDEX:
 			options = index_reloptions(amoptions, datum, false);
 			break;
@@ -1200,10 +1202,6 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
 		offsetof(StdRdOptions, autovacuum) +offsetof(AutoVacOpts, vacuum_scale_factor)},
 		{autovacuum_analyze_scale_factor, RELOPT_TYPE_REAL,
 		offsetof(StdRdOptions, autovacuum) +offsetof(AutoVacOpts, analyze_scale_factor)},
-		{security_barrier, RELOPT_TYPE_BOOL,
-		offsetof(StdRdOptions, security_barrier)},
-		{check_option, RELOPT_TYPE_STRING,
-		offsetof(StdRdOptions, check_option_offset)},
 		{user_catalog_table, RELOPT_TYPE_BOOL,
 		offsetof(StdRdOptions, user_catalog_table)}
 	};
@@ -1225,6 +1223,38 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
 }
 
 /*
+ * Option parser for views
+ */
+bytea *
+view_reloptions(Datum reloptions, bool validate)
+{
+	relopt_value *options;
+	ViewOptions *vopts;
+	int			numoptions;
+	static const relopt_parse_elt tab[] = {
+		{security_barrier, RELOPT_TYPE_BOOL,
+		offsetof(ViewOptions, security_barrier)},
+		{check_option, RELOPT_TYPE_STRING,
+		offsetof(ViewOptions, check_option_offset)}
+	};
+
+	options = parseRelOptions(reloptions, validate, RELOPT_KIND_VIEW, numoptions);
+
+	/* if none set, we're done */
+	if (numoptions == 0)
+		return NULL;
+
+	vopts = allocateReloptStruct(sizeof(ViewOptions), options, numoptions);
+
+	fillRelOptions((void *) vopts, sizeof(ViewOptions), options, numoptions,
+   validate, tab, lengthof(tab));
+
+	pfree(options);
+
+	return (bytea *) vopts;
+}
+
+/*
  * Parse options for heaps, views and toast tables.
  */
 bytea *
@@ -1248,8 +1278,6 @@ heap_reloptions(char relkind, Datum reloptions, bool validate)
 		case RELKIND_RELATION:
 		case RELKIND_MATVIEW:
 			return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
-		case RELKIND_VIEW:
-			return default_reloptions(reloptions, validate, RELOPT_KIND_VIEW);
 		default:
 			/* other relkinds are not supported */
 			return NULL;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 341262b..fd27cdb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c

Re: [HACKERS] view reloptions

2014-06-11 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Is it too late to redefine the check_option_offset stuff before 9.4
 ships, so that it is in its own struct?

We have a forced initdb already for beta2, so I'd say not.

 3) I don't have time to do the legwork before CF1 anyway

... but if nobody does the work, it's moot.

 If we don't change it now, I'm afraid we'll be stuck with using
 StdRdOptions for views for all eternity.

Why would we not be able to change it in 9.5?  It's a catalog change,
sure, but we'll no doubt have others.

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


Re: [HACKERS] view reloptions

2014-06-11 Thread Fabrízio de Royes Mello
On Wed, Jun 11, 2014 at 4:46 PM, Alvaro Herrera alvhe...@2ndquadrant.com
wrote:

 I just noticed by chance that view relations are using StdRdOptions to
 allocate their reloptions.  I can't find any reason for this, other than
 someone failed to realize that they should instead have a struct defined
 of its own, just like (say) GIN indexes do.  Views using StdRdOptions is
 quite pointless, given that it's used for stuff like fillfactor and
 autovacuum, neither of which apply to views.

 9.2 added security_barrier to StdRdOptions, and 9.4 is now adding
 check_option_offset, which is a string reloption with some funny rules.

 Is it too late to redefine the check_option_offset stuff before 9.4
 ships, so that it is in its own struct?  (I'd hope we can redefine it in
 a simpler way also, hopefully one that doesn't require strcmp()'ing that
 string with local or cascaded every time someone is interested in
 knowing the option's value for a particular view.)


Are there a big problem with this implementation?

I asked because we already do a strcmmp()'ing in 'buffering' option for
GiST indexes since this commit
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=5edb24a8.

Regards,

-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog sobre TI: http://fabriziomello.blogspot.com
 Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello