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 @@ -533,7 +533,10 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId) reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, validnsps, true, false); - (void) heap_reloptions(relkind, reloptions, true); + if (relkind == RELKIND_VIEW) + (void) view_reloptions(reloptions, true); + else + (void) heap_reloptions(relkind, reloptions, true); if (stmt->ofTypename) { @@ -8890,10 +8893,12 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation, { case RELKIND_RELATION: case RELKIND_TOASTVALUE: - case RELKIND_VIEW: case RELKIND_MATVIEW: (void) heap_reloptions(rel->rd_rel->relkind, newOptions, true); break; + case RELKIND_VIEW: + (void) view_reloptions(newOptions, true); + break; case RELKIND_INDEX: (void) index_reloptions(rel->rd_am->amoptions, newOptions, true); break; diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h index 81ff328..c226448 100644 --- a/src/include/access/reloptions.h +++ b/src/include/access/reloptions.h @@ -268,6 +268,7 @@ extern void fillRelOptions(void *rdopts, Size basesize, extern bytea *default_reloptions(Datum reloptions, bool validate, relopt_kind kind); extern bytea *heap_reloptions(char relkind, Datum reloptions, bool validate); +extern bytea *view_reloptions(Datum reloptions, bool validate); extern bytea *index_reloptions(RegProcedure amoptions, Datum reloptions, bool validate); extern bytea *attribute_reloptions(Datum reloptions, bool validate); diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index af4f53f..f619011 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -216,8 +216,6 @@ typedef struct StdRdOptions int32 vl_len_; /* varlena header (do not touch directly!) */ int fillfactor; /* page fill factor in percent (0..100) */ AutoVacOpts autovacuum; /* autovacuum-related options */ - bool security_barrier; /* for views */ - int check_option_offset; /* for views */ bool user_catalog_table; /* use as an additional catalog * relation */ } StdRdOptions; @@ -248,12 +246,33 @@ typedef struct StdRdOptions (BLCKSZ * (100 - RelationGetFillFactor(relation, defaultff)) / 100) /* + * RelationIsUsedAsCatalogTable + * Returns whether the relation should be treated as a catalog table + * from the pov of logical decoding. + */ +#define RelationIsUsedAsCatalogTable(relation) \ + ((relation)->rd_options ? \ + ((StdRdOptions *) (relation)->rd_options)->user_catalog_table : false) + + +/* + * ViewOptions + * Contents of rd_options for views + */ +typedef struct ViewOptions +{ + int32 vl_len_; /* varlena header (do not touch directly!) */ + bool security_barrier; + int check_option_offset; +} ViewOptions; + +/* * RelationIsSecurityView * Returns whether the relation is security view, or not */ #define RelationIsSecurityView(relation) \ ((relation)->rd_options ? \ - ((StdRdOptions *) (relation)->rd_options)->security_barrier : false) + ((ViewOptions *) (relation)->rd_options)->security_barrier : false) /* * RelationHasCheckOption @@ -262,7 +281,7 @@ typedef struct StdRdOptions */ #define RelationHasCheckOption(relation) \ ((relation)->rd_options && \ - ((StdRdOptions *) (relation)->rd_options)->check_option_offset != 0) + ((ViewOptions *) (relation)->rd_options)->check_option_offset != 0) /* * RelationHasLocalCheckOption @@ -271,9 +290,9 @@ typedef struct StdRdOptions */ #define RelationHasLocalCheckOption(relation) \ ((relation)->rd_options && \ - ((StdRdOptions *) (relation)->rd_options)->check_option_offset != 0 ? \ + ((ViewOptions *) (relation)->rd_options)->check_option_offset != 0 ? \ strcmp((char *) (relation)->rd_options + \ - ((StdRdOptions *) (relation)->rd_options)->check_option_offset, \ + ((ViewOptions *) (relation)->rd_options)->check_option_offset, \ "local") == 0 : false) /* @@ -283,19 +302,11 @@ typedef struct StdRdOptions */ #define RelationHasCascadedCheckOption(relation) \ ((relation)->rd_options && \ - ((StdRdOptions *) (relation)->rd_options)->check_option_offset != 0 ? \ + ((ViewOptions *) (relation)->rd_options)->check_option_offset != 0 ? \ strcmp((char *) (relation)->rd_options + \ - ((StdRdOptions *) (relation)->rd_options)->check_option_offset, \ + ((ViewOptions *) (relation)->rd_options)->check_option_offset, \ "cascaded") == 0 : false) -/* - * RelationIsUsedAsCatalogTable - * Returns whether the relation should be treated as a catalog table - * from the pov of logical decoding. - */ -#define RelationIsUsedAsCatalogTable(relation) \ - ((relation)->rd_options ? \ - ((StdRdOptions *) (relation)->rd_options)->user_catalog_table : false) /* * RelationIsValid diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index f75fc69..913d6ef 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -1955,6 +1955,7 @@ VariableSpace VariableStatData Vfd ViewCheckOption +ViewOptions ViewStmt VirtualTransactionId Vsrt -- 1.7.2.5
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers