pg.ddx.io pgsql-hackers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
view reloptions 5+ messages / 4 participants [nested] [flat]
Commits from this thread: 2014-07-14 346d7be184 28a935149f Move view reloptions into their own varlena struct
* view reloptions @ 2014-06-11 19:46 Alvaro Herrera <alvherre@2ndquadrant.com> 2014-06-11 22:00 ` Re: view reloptions Tom Lane <tgl@sss.pgh.pa.us> 2014-06-11 22:27 ` Re: view reloptions Fabrízio de Royes Mello <fabriziomello@gmail.com> 2014-06-13 06:21 ` Re: view reloptions Jaime Casanova <jaime@2ndquadrant.com> 0 siblings, 3 replies; 5+ messages in thread From: Alvaro Herrera @ 2014-06-11 19:46 UTC (permalink / raw) To: pgsql-hackers 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.) There are some problems with this idea though, namely: 1) it's damn late in the release cycle already 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.) Thoughts? -- Álvaro Herrera http://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 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: view reloptions 2014-06-11 19:46 view reloptions Alvaro Herrera <alvherre@2ndquadrant.com> @ 2014-06-11 22:00 ` Tom Lane <tgl@sss.pgh.pa.us> 2 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2014-06-11 22:00 UTC (permalink / raw) To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers Alvaro Herrera <alvherre@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 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: view reloptions 2014-06-11 19:46 view reloptions Alvaro Herrera <alvherre@2ndquadrant.com> @ 2014-06-11 22:27 ` Fabrízio de Royes Mello <fabriziomello@gmail.com> 2 siblings, 0 replies; 5+ messages in thread From: Fabrízio de Royes Mello @ 2014-06-11 22:27 UTC (permalink / raw) To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers On Wed, Jun 11, 2014 at 4:46 PM, Alvaro Herrera <alvherre@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 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: view reloptions 2014-06-11 19:46 view reloptions Alvaro Herrera <alvherre@2ndquadrant.com> @ 2014-06-13 06:21 ` Jaime Casanova <jaime@2ndquadrant.com> 2014-07-14 21:40 ` Re: view reloptions Alvaro Herrera <alvherre@2ndquadrant.com> 2 siblings, 1 reply; 5+ messages in thread From: Jaime Casanova @ 2014-06-13 06:21 UTC (permalink / raw) To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers On Wed, Jun 11, 2014 at 2:46 PM, Alvaro Herrera <alvherre@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Attachments:^ permalink raw reply [nested|flat] 5+ messages in thread0001-Move-reloptions-from-views-into-its-own-structure.patch (text/x-patch, 8.5K) download
From 87ed78a4f276484a37917c417286a16082030f13 Mon Sep 17 00:00:00 2001 From: Jaime Casanova <jaime@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
* Re: view reloptions 2014-06-11 19:46 view reloptions Alvaro Herrera <alvherre@2ndquadrant.com> 2014-06-13 06:21 ` Re: view reloptions Jaime Casanova <jaime@2ndquadrant.com> @ 2014-07-14 21:40 ` Alvaro Herrera <alvherre@2ndquadrant.com> 0 siblings, 0 replies; 5+ messages in thread From: Alvaro Herrera @ 2014-07-14 21:40 UTC (permalink / raw) To: Jaime Casanova <jaime@2ndquadrant.com>; +Cc: pgsql-hackers 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 Herrera http://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 ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2014-07-14 21:40 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2014-06-11 19:46 view reloptions Alvaro Herrera <alvherre@2ndquadrant.com> 2014-06-11 22:00 ` Tom Lane <tgl@sss.pgh.pa.us> 2014-06-11 22:27 ` Fabrízio de Royes Mello <fabriziomello@gmail.com> 2014-06-13 06:21 ` Jaime Casanova <jaime@2ndquadrant.com> 2014-07-14 21:40 ` Alvaro Herrera <alvherre@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox