Re: [PATCH] psql: Add tab-complete for optional view parameters
On Fri, Aug 11, 2023 at 12:48:17PM -0700, David Zhang wrote:
>
> Applied v3 patch to master and verified it with below commands,
Thanks for testing!
> [..]
>
> For below changes,
>
> else if (TailMatches("CREATE", "VIEW", MatchAny, "AS") ||
> - TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny,
> "AS"))
> + TailMatches("CREATE", "VIEW", MatchAny, "WITH", "(*)", "AS")
> ||
> + TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "AS")
> ||
> + TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny,
> "WITH", "(*)", "AS"))
>
> it would be great to switch the order of the 3rd and the 4th line to make a
> better match for "CREATE" and "CREATE OR REPLACE" .
I don't see how it would effect matching in any way - or am I
overlooking something here?
postgres=# CREATE VIEW v
AS WITH (
postgres=# CREATE VIEW v AS
.. autocompletes with "SELECT"
postgres=# CREATE VIEW v WITH ( security_invoker = true )
.. autocompletes with "AS" and so on
And the same for CREATE OR REPLACE VIEW.
Can you provide an example case that would benefit from that?
>
> Since it supports in the middle for below case,
> postgres=# alter view v set ( security_
> security_barrier security_invoker
>
> and during view reset it can also provide all the options list,
> postgres=# alter view v reset (
> CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER
>
> but not sure if it is a good idea or possible to autocomplete the reset
> options after seeing one of the options showing up with "," for example,
> postgres=# alter view v reset ( CHECK_OPTION,
> SECURITY_BARRIER SECURITY_INVOKER
I'd rather not add this and leave it as-is. It doesn't add any real
value - how often does this case really come up, especially with ALTER
VIEW only having three options?
Thanks,
Christoph
Re: [PATCH] psql: Add tab-complete for optional view parameters
On Tue, Aug 08, 2023 at 09:17:51AM +0100, Dean Rasheed wrote: > > On Mon, 7 Aug 2023 at 19:49, Christoph Heiss wrote: > > > > On Fri, Jan 06, 2023 at 12:18:44PM +, Dean Rasheed wrote: > > > Hmm, I don't think we should be offering "check_option" as a tab > > > completion for CREATE VIEW at all, since that would encourage users to > > > use non-SQL-standard syntax, rather than CREATE VIEW ... WITH > > > [CASCADED|LOCAL] CHECK OPTION. > > > > Left that part in for now. I would argue that it is a well-documented > > combination and as such users would expect it to turn up in the > > tab-complete as well. OTOH not against removing it either, if there are > > others voicing the same opinion .. > > > > On reflection, I think that's probably OK. I mean, I still don't like > the fact that it's offering to complete with non-SQL-standard syntax, > but that seems less bad than using an incomplete list of options, and > I don't really have any other better ideas. My thought pretty much as well. While obviously far from ideal as you say, it's the less bad case of these both. I would also guess that it is not the only instance of non-SQL-standard syntax completion in psql .. Thanks for weighing in once again. Cheers, Christoph
Re: [PATCH] psql: Add tab-complete for optional view parameters
Hi all,
sorry for the long delay.
On Mon, Jan 09, 2023 at 04:32:09PM +0100, Jim Jones wrote:
> However, an "ALTER TABLE S" does not complete the open
> parenthesis "(" from "SET (", as suggested in "ALTER VIEW ".
>
> postgres=# ALTER VIEW w SET
> Display all 187 possibilities? (y or n)
>
> Is it intended to behave like this? If so, an "ALTER VIEW
> RES" does complete the open parenthesis -> "RESET (".
On Sun, Jan 29, 2023 at 10:19:12AM +, Mikhail Gribkov wrote:
> The patch have a potential, although I have to agree with Jim Jones,
> it obviously have a problem with "alter view set"
> handling.
> [..]
> I think it may worth looking at "alter materialized view" completion
> tree and making "alter view" the same way.
Thank you both for reviewing/testing and the suggestions. Yeah,
definitively, sounds very sensible.
I've attached a new revision, rebased and addressing the above by
aligning it with how "ALTER MATERIALIZED VIEW" works, such that "SET ("
and "SET SCHEMA" won't compete anymore. So that should now work more
like expected.
postgres=# ALTER MATERIALIZED VIEW m
ALTER COLUMN CLUSTER ON DEPENDS ON EXTENSION
NO DEPENDS ON EXTENSION OWNER TO RENAME
RESET ( SET
postgres=# ALTER MATERIALIZED VIEW m SET
(ACCESS METHODSCHEMA TABLESPACE
WITHOUT CLUSTER
postgres=# ALTER VIEW v
ALTER COLUMN OWNER TO RENAMERESET ( SET
postgres=# ALTER VIEW v SET
( SCHEMA
postgres=# ALTER VIEW v SET (
CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER
On Fri, Jan 06, 2023 at 12:18:44PM +, Dean Rasheed wrote:
> Hmm, I don't think we should be offering "check_option" as a tab
> completion for CREATE VIEW at all, since that would encourage users to
> use non-SQL-standard syntax, rather than CREATE VIEW ... WITH
> [CASCADED|LOCAL] CHECK OPTION.
Left that part in for now. I would argue that it is a well-documented
combination and as such users would expect it to turn up in the
tab-complete as well. OTOH not against removing it either, if there are
others voicing the same opinion ..
Thanks,
Christoph
>From 3663eb0b5008d632972d4b66a105fc08cfff13fb Mon Sep 17 00:00:00 2001
From: Christoph Heiss
Date: Mon, 7 Aug 2023 20:37:19 +0200
Subject: [PATCH v3] psql: Add tab-complete for optional view parameters
This adds them in the same matter as it works for storage parameters of
tables.
Signed-off-by: Christoph Heiss
---
src/bin/psql/tab-complete.c | 36 +++-
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 779fdc90cb..83ec1508bb 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1329,6 +1329,13 @@ static const char *const table_storage_parameters[] = {
NULL
};
+/* Optional parameters for CREATE VIEW and ALTER VIEW */
+static const char *const view_optional_parameters[] = {
+ "check_option",
+ "security_barrier",
+ "security_invoker",
+ NULL
+};
/* Forward declaration of functions */
static char **psql_completion(const char *text, int start, int end);
@@ -2216,8 +2223,7 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH("TO");
/* ALTER VIEW */
else if (Matches("ALTER", "VIEW", MatchAny))
- COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
- "SET SCHEMA");
+ COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", "RESET (",
"SET");
/* ALTER VIEW xxx RENAME */
else if (Matches("ALTER", "VIEW", MatchAny, "RENAME"))
COMPLETE_WITH_ATTR_PLUS(prev2_wd, "COLUMN", "TO");
@@ -2233,6 +2239,16 @@ psql_completion(const char *text, int start, int end)
/* ALTER VIEW xxx RENAME COLUMN yyy */
else if (Matches("ALTER", "VIEW", MatchAny, "RENAME", "COLUMN",
MatchAnyExcept("TO")))
COMPLETE_WITH("TO");
+ /* ALTER VIEW xxx SET ( yyy [= zzz] ) */
+ else if (Matches("ALTER", "VIEW", MatchAny, "SET"))
+ COMPLETE_WITH("(", "SCHEMA");
+ /* ALTER VIEW xxx SET|RESET ( yyy [= zzz] ) */
+ else if (Matches("ALTER", "VIEW", MatchAny, "SET|RESET", "("))
+ COMPLETE_WITH_LIST(view_optional_parameters);
+ else if (Matches("ALTER", "VIEW", MatchAny, &quo
Re: [PATCH] psql: Add tab-complete for optional view parameters
Thanks for the review!
On 12/8/22 12:19, Melih Mutlu wrote:
Hi Christoph,
I just took a quick look at your patch.
Some suggestions:
+ else if (Matches("ALTER", "VIEW", MatchAny, "SET", "("))
+ COMPLETE_WITH_LIST(view_optional_parameters);
+ /* ALTER VIEW xxx RESET ( yyy , ... ) */
+ else if (Matches("ALTER", "VIEW", MatchAny, "RESET", "("))
+ COMPLETE_WITH_LIST(view_optional_parameters);
What about combining these two cases into one like Matches("ALTER",
"VIEW", MatchAny, "SET|RESET", "(") ?
Good thinking, incorporated that into v2.
While at it, I also added completion for the values of the SET
parameters, since that is useful as well.
/* ALTER VIEW */
else if (Matches("ALTER", "VIEW", MatchAny))
COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
"SET SCHEMA");
Also seems like SET and RESET don't get auto-completed for "ALTER VIEW
".
I think it would be nice to include those missing words.
That is already contained in the patch:
@@ -2139,7 +2146,7 @@ psql_completion(const char *text, int start, int end)
/* ALTER VIEW */
else if (Matches("ALTER", "VIEW", MatchAny))
COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
- "SET SCHEMA");
+ "SET SCHEMA", "SET (", "RESET (");
Thanks,
ChristophFrom a53f73da6c3f6d6ace59ec9d8d9db5efef323f6c Mon Sep 17 00:00:00 2001
From: Christoph Heiss
Date: Fri, 9 Dec 2022 11:22:38 +0100
Subject: [PATCH v2] psql: Add tab-complete for optional view parameters
This adds them in the same matter as it works for storage parameters of
tables.
Signed-off-by: Christoph Heiss
---
src/bin/psql/tab-complete.c | 32
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 89e7317c23..7d30ec6d5a 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1310,6 +1310,13 @@ static const char *const table_storage_parameters[] = {
NULL
};
+/* Optional parameters for CREATE VIEW and ALTER VIEW */
+static const char *const view_optional_parameters[] = {
+ "check_option",
+ "security_barrier",
+ "security_invoker",
+ NULL
+};
/* Forward declaration of functions */
static char **psql_completion(const char *text, int start, int end);
@@ -2139,7 +2146,7 @@ psql_completion(const char *text, int start, int end)
/* ALTER VIEW */
else if (Matches("ALTER", "VIEW", MatchAny))
COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
- "SET SCHEMA");
+ "SET SCHEMA", "SET (", "RESET (");
/* ALTER VIEW xxx RENAME */
else if (Matches("ALTER", "VIEW", MatchAny, "RENAME"))
COMPLETE_WITH_ATTR_PLUS(prev2_wd, "COLUMN", "TO");
@@ -2155,6 +2162,13 @@ psql_completion(const char *text, int start, int end)
/* ALTER VIEW xxx RENAME COLUMN yyy */
else if (Matches("ALTER", "VIEW", MatchAny, "RENAME", "COLUMN", MatchAnyExcept("TO")))
COMPLETE_WITH("TO");
+ /* ALTER VIEW xxx SET|RESET ( yyy [= zzz] ) */
+ else if (Matches("ALTER", "VIEW", MatchAny, "SET|RESET", "("))
+ COMPLETE_WITH_LIST(view_optional_parameters);
+ else if (Matches("ALTER", "VIEW", MatchAny, "SET", "(", "check_option", "="))
+ COMPLETE_WITH("local", "cascaded");
+ else if (Matches("ALTER", "VIEW", MatchAny, "SET", "(", "security_barrier|security_invoker", "="))
+ COMPLETE_WITH("true", "false");
/* ALTER MATERIALIZED VIEW */
else if (Matches("ALTER", "MATERIALIZED", "VIEW", MatchAny))
@@ -3426,13 +3440,23 @@ psql_completion(const char *text, int start, int end)
}
/* CREATE VIEW --- is allowed inside CREATE SCHEMA, so use TailMatches */
- /* Complete CREATE [ OR REPLACE ] VIEW with AS */
+ /* Complete CREATE [ OR REPLACE ] VIEW with AS or WITH ( */
else if (TailMatches("CREATE", "VIEW", MatchAny) ||
TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny))
+ COMPLETE_WITH("AS", "WITH (");
+ /* Complete CREATE [ OR REPLACE ] VIEW WITH ( with supported options */
+ else if (TailMatches("CREATE", "VIEW", MatchAny, "WITH", "(") ||
+ TailMatches("C
[PATCH] psql: Add tab-complete for optional view parameters
Hi all!
This adds tab-complete for optional parameters (as can be specified
using WITH) for views, similarly to how it works for storage parameters
of tables.
Thanks,
Christoph HeissFrom bd12c08a80b5973fdcac59def213216d06f150b0 Mon Sep 17 00:00:00 2001
From: Christoph Heiss
Date: Wed, 7 Dec 2022 19:15:48 +0100
Subject: [PATCH] psql: Add tab-complete for optional view parameters
This adds them in the same matter as it works for storage parameters of
tables.
Signed-off-by: Christoph Heiss
---
src/bin/psql/tab-complete.c | 31 +++
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 89e7317c23..999a0e5aa1 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1310,6 +1310,13 @@ static const char *const table_storage_parameters[] = {
NULL
};
+/* Optional parameters for CREATE VIEW and ALTER VIEW */
+static const char *const view_optional_parameters[] = {
+ "check_option",
+ "security_barrier",
+ "security_invoker",
+ NULL
+};
/* Forward declaration of functions */
static char **psql_completion(const char *text, int start, int end);
@@ -2139,7 +2146,7 @@ psql_completion(const char *text, int start, int end)
/* ALTER VIEW */
else if (Matches("ALTER", "VIEW", MatchAny))
COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
- "SET SCHEMA");
+ "SET SCHEMA", "SET (", "RESET (");
/* ALTER VIEW xxx RENAME */
else if (Matches("ALTER", "VIEW", MatchAny, "RENAME"))
COMPLETE_WITH_ATTR_PLUS(prev2_wd, "COLUMN", "TO");
@@ -2155,6 +2162,12 @@ psql_completion(const char *text, int start, int end)
/* ALTER VIEW xxx RENAME COLUMN yyy */
else if (Matches("ALTER", "VIEW", MatchAny, "RENAME", "COLUMN", MatchAnyExcept("TO")))
COMPLETE_WITH("TO");
+ /* ALTER VIEW xxx SET ( yyy [= zzz] ) */
+ else if (Matches("ALTER", "VIEW", MatchAny, "SET", "("))
+ COMPLETE_WITH_LIST(view_optional_parameters);
+ /* ALTER VIEW xxx RESET ( yyy , ... ) */
+ else if (Matches("ALTER", "VIEW", MatchAny, "RESET", "("))
+ COMPLETE_WITH_LIST(view_optional_parameters);
/* ALTER MATERIALIZED VIEW */
else if (Matches("ALTER", "MATERIALIZED", "VIEW", MatchAny))
@@ -3426,13 +3439,23 @@ psql_completion(const char *text, int start, int end)
}
/* CREATE VIEW --- is allowed inside CREATE SCHEMA, so use TailMatches */
- /* Complete CREATE [ OR REPLACE ] VIEW with AS */
+ /* Complete CREATE [ OR REPLACE ] VIEW with AS or WITH ( */
else if (TailMatches("CREATE", "VIEW", MatchAny) ||
TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny))
+ COMPLETE_WITH("AS", "WITH (");
+ /* Complete CREATE [ OR REPLACE ] VIEW WITH ( with supported options */
+ else if (TailMatches("CREATE", "VIEW", MatchAny, "WITH", "(") ||
+ TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "WITH", "("))
+ COMPLETE_WITH_LIST(view_optional_parameters);
+ /* Complete CREATE [ OR REPLACE ] VIEW WITH ( ... ) with "AS" */
+ else if (TailMatches("CREATE", "VIEW", MatchAny, "WITH", "(*)") ||
+ TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "WITH", "(*)"))
COMPLETE_WITH("AS");
- /* Complete "CREATE [ OR REPLACE ] VIEW AS with "SELECT" */
+ /* Complete "CREATE [ OR REPLACE ] VIEW [ WITH ( ... ) ] AS with "SELECT" */
else if (TailMatches("CREATE", "VIEW", MatchAny, "AS") ||
- TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "AS"))
+ TailMatches("CREATE", "VIEW", MatchAny, "WITH", "(*)", "AS") ||
+ TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "AS") ||
+ TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "WITH", "(*)", "AS"))
COMPLETE_WITH("SELECT");
/* CREATE MATERIALIZED VIEW */
--
2.38.1
Re: [PATCH] Add sortsupport for range types and btree_gist
Hi!
Sorry for the long delay.
This fixes the crashes, now all tests run fine w/ and w/o debug
configuration. That shouldn't even have slipped through as such.
Notable changes from v1:
- gbt_enum_sortsupport() now passes on fcinfo->flinfo
enum_cmp_internal() needs a place to cache the typcache entry.
- inet sortsupport now uses network_cmp() directly
Thanks,
Christoph HeissFrom 667779bc0761c1356141722181c5a54ac46d96b9 Mon Sep 17 00:00:00 2001
From: Christoph Heiss
Date: Wed, 31 Aug 2022 19:20:43 +0200
Subject: [PATCH v2 1/1] Add sortsupport for range types and btree_gist
Incrementally building up a GiST index can result in a sub-optimal index
structure for range types.
By sorting the data before inserting it into the index will result in a
much better index.
This can provide sizeable improvements in query execution time, I/O read
time and shared block hits/reads.
Signed-off-by: Christoph Heiss
---
contrib/btree_gist/Makefile | 3 +-
contrib/btree_gist/btree_bit.c | 19
contrib/btree_gist/btree_bool.c | 22
contrib/btree_gist/btree_cash.c | 22
contrib/btree_gist/btree_enum.c | 26 +
contrib/btree_gist/btree_gist--1.7--1.8.sql | 110
contrib/btree_gist/btree_gist.control | 2 +-
contrib/btree_gist/btree_inet.c | 19
contrib/btree_gist/btree_interval.c | 19
contrib/btree_gist/btree_macaddr8.c | 19
contrib/btree_gist/btree_time.c | 19
src/backend/utils/adt/rangetypes_gist.c | 70 +
src/include/catalog/pg_amproc.dat | 3 +
src/include/catalog/pg_proc.dat | 3 +
14 files changed, 354 insertions(+), 2 deletions(-)
create mode 100644 contrib/btree_gist/btree_gist--1.7--1.8.sql
diff --git a/contrib/btree_gist/Makefile b/contrib/btree_gist/Makefile
index 48997c75f6..4096de73ea 100644
--- a/contrib/btree_gist/Makefile
+++ b/contrib/btree_gist/Makefile
@@ -33,7 +33,8 @@ EXTENSION = btree_gist
DATA = btree_gist--1.0--1.1.sql \
btree_gist--1.1--1.2.sql btree_gist--1.2.sql btree_gist--1.2--1.3.sql \
btree_gist--1.3--1.4.sql btree_gist--1.4--1.5.sql \
- btree_gist--1.5--1.6.sql btree_gist--1.6--1.7.sql
+ btree_gist--1.5--1.6.sql btree_gist--1.6--1.7.sql \
+ btree_gist--1.7--1.8.sql
PGFILEDESC = "btree_gist - B-tree equivalent GiST operator classes"
REGRESS = init int2 int4 int8 float4 float8 cash oid timestamp timestamptz \
diff --git a/contrib/btree_gist/btree_bit.c b/contrib/btree_gist/btree_bit.c
index 5b246bcde4..bf32bfd628 100644
--- a/contrib/btree_gist/btree_bit.c
+++ b/contrib/btree_gist/btree_bit.c
@@ -7,6 +7,7 @@
#include "btree_utils_var.h"
#include "utils/builtins.h"
#include "utils/bytea.h"
+#include "utils/sortsupport.h"
#include "utils/varbit.h"
@@ -19,10 +20,17 @@ PG_FUNCTION_INFO_V1(gbt_bit_picksplit);
PG_FUNCTION_INFO_V1(gbt_bit_consistent);
PG_FUNCTION_INFO_V1(gbt_bit_penalty);
PG_FUNCTION_INFO_V1(gbt_bit_same);
+PG_FUNCTION_INFO_V1(gbt_bit_sortsupport);
/* define for comparison */
+static int
+bit_fast_cmp(Datum x, Datum y, SortSupport ssup)
+{
+ return DatumGetInt32(DirectFunctionCall2(byteacmp, x, y));
+}
+
static bool
gbt_bitgt(const void *a, const void *b, Oid collation, FmgrInfo *flinfo)
{
@@ -208,3 +216,14 @@ gbt_bit_penalty(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(gbt_var_penalty(result, o, n, PG_GET_COLLATION(),
, fcinfo->flinfo));
}
+
+Datum
+gbt_bit_sortsupport(PG_FUNCTION_ARGS)
+{
+ SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
+
+ ssup->comparator = bit_fast_cmp;
+ ssup->ssup_extra = NULL;
+
+ PG_RETURN_VOID();
+}
diff --git a/contrib/btree_gist/btree_bool.c b/contrib/btree_gist/btree_bool.c
index 8b2af129b5..3a9f230e68 100644
--- a/contrib/btree_gist/btree_bool.c
+++ b/contrib/btree_gist/btree_bool.c
@@ -6,6 +6,7 @@
#include "btree_gist.h"
#include "btree_utils_num.h"
#include "common/int.h"
+#include "utils/sortsupport.h"
typedef struct boolkey
{
@@ -23,6 +24,16 @@ PG_FUNCTION_INFO_V1(gbt_bool_picksplit);
PG_FUNCTION_INFO_V1(gbt_bool_consistent);
PG_FUNCTION_INFO_V1(gbt_bool_penalty);
PG_FUNCTION_INFO_V1(gbt_bool_same);
+PG_FUNCTION_INFO_V1(gbt_bool_sortsupport);
+
+static int
+bool_fast_cmp(Datum x, Datum y, SortSupport ssup)
+{
+ bool arg1 = DatumGetBool(x);
+ bool arg2 = DatumGetBool(y);
+
+ return arg1 - arg2;
+}
static bool
gbt_boolgt(const void *a, const void *b, FmgrInfo *flinfo)
@@ -167,3 +178,14 @@ gbt_bool_same(PG_FUNCTION_ARGS)
*result = gbt_num_same((void *) b1, (void *) b2, , fcinfo->flinfo);
PG_RETURN_POINTER(result);
}
+
+Datum
+gbt_bool_sortsupport(PG_FUNCTION_ARGS)
+{
+ SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
+
+ ssup->comparator = bool_fast_cmp;
+ ssup->ssup_extra = NULL;
+
+ PG_RETURN_VOID
[PATCH] Add sortsupport for range types and btree_gist
Hi all! The motivation behind this is that incrementally building up a GiST index for certain input data can create a terrible tree structure. Furthermore, exclusion constraints are commonly implemented using GiST indices and for that use case, data is mostly orderable. By sorting the data before inserting it into the index results in a much better index structure, leading to significant performance improvements. Testing was done using following setup, with about 50 million rows: CREATE EXTENSION btree_gist; CREATE TABLE t (id uuid, block_range int4range); CREATE INDEX ON before USING GIST (id, block_range); COPY t FROM '..' DELIMITER ',' CSV HEADER; using SELECT * FROM t WHERE id = '..' AND block_range && '..' as test query, using a unpatched instance and one with the patch applied. Some stats for fetching 10,000 random rows using the query above, 100 iterations to get good averages. The benchmarking was done on a unpatched instance compiled using the exact same options as with the patch applied. [ Results are noted in a unpatched -> patched fashion. ] First set of results are after the initial CREATE TABLE, CREATE INDEX and a COPY to the table, thereby incrementally building the index. Shared Hit Blocks (average): 110.97 -> 78.58 Shared Read Blocks (average): 58.90 -> 47.42 Execution Time (average): 1.10 -> 0.83 ms I/O Read Time (average): 0.19 -> 0.15 ms After a REINDEX on the table, the results improve even more: Shared Hit Blocks (average): 84.24 -> 8.54 Shared Read Blocks (average): 49.89 -> 0.74 Execution Time (average): 0.84 -> 0.065 ms I/O Read Time (average): 0.16 -> 0.004 ms Additionally, the time a REINDEX takes also improves significantly: 672407.584 ms (11:12.408) -> 130670.232 ms (02:10.670) Most of the sortsupport for btree_gist was implemented by re-using already existing infrastructure. For the few remaining types (bit, bool, cash, enum, interval, macaddress8 and time) I manually implemented them directly in btree_gist. It might make sense to move them into the backend for uniformity, but I wanted to get other opinions on that first. `make check-world` reports no regressions. Attached below, besides the patch, are also two scripts for benchmarking. `bench-gist.py` to benchmark the actual patch, example usage of this would be e.g. `./bench-gist.py -o results.csv public.table`. This expects a local instance with no authentication and default `postgres` user. The port can be set using the `--port` option. `plot.py` prints average values (as used above) and creates boxplots for each statistic from the result files produced with `bench-gist.py`. Depends on matplotlib and pandas. Additionally, if needed, the sample dataset used to benchmark this is available to independently verify the results [1]. Thanks, Christoph Heiss --- [1] https://drive.google.com/file/d/1SKRiUYd78_zl7CeD8pLDoggzCCh0wj39From 22e1b60929c39309e06c2477d8d027e60ad49b9d Mon Sep 17 00:00:00 2001 From: Christoph Heiss Date: Thu, 9 Jun 2022 17:07:46 +0200 Subject: [PATCH 1/1] Add sortsupport for range types and btree_gist Incrementally building up a GiST index can result in a sub-optimal index structure for range types. By sorting the data before inserting it into the index will result in a much better index. This can provide sizeable improvements in query execution time, I/O read time and shared block hits/reads. Signed-off-by: Christoph Heiss --- contrib/btree_gist/Makefile | 3 +- contrib/btree_gist/btree_bit.c | 19 contrib/btree_gist/btree_bool.c | 22 contrib/btree_gist/btree_cash.c | 22 contrib/btree_gist/btree_enum.c | 19 contrib/btree_gist/btree_gist--1.7--1.8.sql | 105 contrib/btree_gist/btree_gist.control | 2 +- contrib/btree_gist/btree_interval.c | 19 contrib/btree_gist/btree_macaddr8.c | 19 contrib/btree_gist/btree_time.c | 19 src/backend/utils/adt/rangetypes_gist.c | 70 + src/include/catalog/pg_amproc.dat | 3 + src/include/catalog/pg_proc.dat | 3 + 13 files changed, 323 insertions(+), 2 deletions(-) create mode 100644 contrib/btree_gist/btree_gist--1.7--1.8.sql diff --git a/contrib/btree_gist/Makefile b/contrib/btree_gist/Makefile index 48997c75f6..4096de73ea 100644 --- a/contrib/btree_gist/Makefile +++ b/contrib/btree_gist/Makefile @@ -33,7 +33,8 @@ EXTENSION = btree_gist DATA = btree_gist--1.0--1.1.sql \ btree_gist--1.1--1.2.sql btree_gist--1.2.sql btree_gist--1.2--1.3.sql \ btree_gist--1.3--1.4.sql btree_gist--1.4--1.5.sql \ - btree_gist--1.5--1.6.sql btree_gist--1.6--1.7.sql + btree_gist--1.5--1.6.sql btree_gist--1.6--1.7.sql \ + btree_gist--1.7--1.8.sql PGFILEDESC = "btree_gist - B-tree equivalent GiST operator classes" REGRESS = init
Re: [PATCH] Add reloption for views to enable RLS
On 3/9/22 16:06, Laurenz Albe wrote: This paragraph contains a couple of grammatical errors. How about Note that the user performing the insert, update or delete on the view must have the corresponding insert, update or delete privilege on the view. Unless security_invoker is set to true, the view's owner must additionally have the relevant privileges on the underlying base relations, but the user performing the update does not need any permissions on the underlying base relations (see ). If security_invoker is set to true, it is the invoking user rather than the view owner that must have the relevant privileges on the underlying base relations. Replaced the two paragraphs with your suggestion, it is indeed easier to read. Also, this: [..] could be written like this (introducing a new variable): if (rule->event == CMD_SELECT && relation->rd_rel->relkind == RELKIND_VIEW && RelationHasSecurityInvoker(relation)) user_for_check = InvalidOid; else user_for_check = relation->rd_rel->relowner; setRuleCheckAsUser((Node *) rule->actions, user_for_check); setRuleCheckAsUser(rule->qual, user_for_check); This might be easier to read. Makes sense, I've changed that. This also seems to be more in line with all the other code. While at it I also split the comment alongside it to match, hopefully that makes sense. Thanks, Christoph HeissFrom 9837e98f31fdbf1bcd1929309a14b1fd99fd6ec1 Mon Sep 17 00:00:00 2001 From: Christoph Heiss Date: Mon, 14 Mar 2022 13:32:28 +0100 Subject: [PATCH v11 1/1] Add new boolean reloption "security_invoker" to views When this reloption is set to "true", all permissions on the underlying relations will be checked against the invoking user rather than the view owner. The latter remains the default behavior. Author: Christoph Heiss Co-Author: Laurenz Albe Reviewed-By: Laurenz Albe, Wolfgang Walther Discussion: https://postgr.es/m/b66dd6d6-ad3e-c6f2-8b90-47be773da240%40cybertec.at Signed-off-by: Christoph Heiss --- doc/src/sgml/ref/alter_view.sgml | 13 ++- doc/src/sgml/ref/create_view.sgml | 77 ++ src/backend/access/common/reloptions.c| 11 +++ src/backend/rewrite/rewriteHandler.c | 18 +++-- src/backend/utils/cache/relcache.c| 80 --- src/include/utils/rel.h | 11 +++ src/test/regress/expected/create_view.out | 73 ++--- src/test/regress/expected/rowsecurity.out | 43 +- src/test/regress/expected/rules.out | 20 + src/test/regress/expected/updatable_views.out | 28 +++ src/test/regress/sql/create_view.sql | 44 +- src/test/regress/sql/rowsecurity.sql | 26 ++ src/test/regress/sql/rules.sql| 23 ++ src/test/regress/sql/updatable_views.sql | 27 +++ 14 files changed, 430 insertions(+), 64 deletions(-) diff --git a/doc/src/sgml/ref/alter_view.sgml b/doc/src/sgml/ref/alter_view.sgml index 98c312c5bf..f6bb084117 100644 --- a/doc/src/sgml/ref/alter_view.sgml +++ b/doc/src/sgml/ref/alter_view.sgml @@ -156,11 +156,22 @@ ALTER VIEW [ IF EXISTS ] name RESET Changes the security-barrier property of the view. The value must - be Boolean value, such as true + be a Boolean value, such as true or false. + +security_invoker (boolean) + + + Changes whether permission checks on the underlying relations are + performed as the view owner or as the invoking user. Default is + false. The value must be a Boolean value, such as + true or false. + + + diff --git a/doc/src/sgml/ref/create_view.sgml b/doc/src/sgml/ref/create_view.sgml index bf03287592..29e2b4a480 100644 --- a/doc/src/sgml/ref/create_view.sgml +++ b/doc/src/sgml/ref/create_view.sgml @@ -137,8 +137,6 @@ CREATE VIEW [ schema . ] view_namelocal or cascaded, and is equivalent to specifying WITH [ CASCADED | LOCAL ] CHECK OPTION (see below). - This option can be changed on existing views using ALTER VIEW. @@ -152,7 +150,23 @@ CREATE VIEW [ schema . ] view_name - + + +security_invoker (boolean) + + + If this option is set to true, it will cause all + access to underlying tables to be checked as referenced by the + invoking user, otherwise as the view owner (default). See below for + implementation details and quirks. + + + + + + All of the above options can be changed on existing views using ALTER VIEW. + @@ -265,13 +279,42 @@ CREATE VIEW vista AS SELECT text 'Hello Worl
Re: [PATCH] Add reloption for views to enable RLS
On 3/2/22 11:10, Dean Rasheed wrote: For my part, I find myself more and more convinced that "security_invoker" is the right name, because it matches the terminology used for functions, and in other database systems. I think the parallels between security invoker functions and security invoker views are quite strong. [..] What are other people's opinions? Since there don't seem to be any more objections to "security_invoker" I attached v10 renaming it again. I've tried to better clarify the whole invoker vs. definer thing in the CREATE VIEW documentation by explicitly mentioning that "security_invoker=false" is _not_ the same as "security definer", based on the earlier discussions. This should hopefully avoid any implicit associations. Thanks, ChristophFrom 89efb198694f7ff6e05068662a72a0bdcb43e13d Mon Sep 17 00:00:00 2001 From: Christoph Heiss Date: Tue, 8 Mar 2022 17:54:46 +0100 Subject: [PATCH v10 1/1] Add new boolean reloption "security_invoker" to views When this reloption is set to "true", all permissions on the underlying relations will be checked against the invoking user rather than the view owner. The latter remains the default behavior. Author: Christoph Heiss Co-Author: Laurenz Albe Reviewed-By: Laurenz Albe, Wolfgang Walther Discussion: https://postgr.es/m/b66dd6d6-ad3e-c6f2-8b90-47be773da240%40cybertec.at Signed-off-by: Christoph Heiss --- doc/src/sgml/ref/alter_view.sgml | 13 ++- doc/src/sgml/ref/create_view.sgml | 76 +++--- src/backend/access/common/reloptions.c| 11 +++ src/backend/rewrite/rewriteHandler.c | 18 +++-- src/backend/utils/cache/relcache.c| 79 --- src/include/utils/rel.h | 11 +++ src/test/regress/expected/create_view.out | 73 ++--- src/test/regress/expected/rowsecurity.out | 43 +- src/test/regress/expected/rules.out | 20 + src/test/regress/expected/updatable_views.out | 28 +++ src/test/regress/sql/create_view.sql | 44 ++- src/test/regress/sql/rowsecurity.sql | 26 ++ src/test/regress/sql/rules.sql| 23 ++ src/test/regress/sql/updatable_views.sql | 27 +++ 14 files changed, 430 insertions(+), 62 deletions(-) diff --git a/doc/src/sgml/ref/alter_view.sgml b/doc/src/sgml/ref/alter_view.sgml index 98c312c5bf..f6bb084117 100644 --- a/doc/src/sgml/ref/alter_view.sgml +++ b/doc/src/sgml/ref/alter_view.sgml @@ -156,11 +156,22 @@ ALTER VIEW [ IF EXISTS ] name RESET Changes the security-barrier property of the view. The value must - be Boolean value, such as true + be a Boolean value, such as true or false. + +security_invoker (boolean) + + + Changes whether permission checks on the underlying relations are + performed as the view owner or as the invoking user. Default is + false. The value must be a Boolean value, such as + true or false. + + + diff --git a/doc/src/sgml/ref/create_view.sgml b/doc/src/sgml/ref/create_view.sgml index bf03287592..1f55379f73 100644 --- a/doc/src/sgml/ref/create_view.sgml +++ b/doc/src/sgml/ref/create_view.sgml @@ -137,8 +137,6 @@ CREATE VIEW [ schema . ] view_namelocal or cascaded, and is equivalent to specifying WITH [ CASCADED | LOCAL ] CHECK OPTION (see below). - This option can be changed on existing views using ALTER VIEW. @@ -152,7 +150,23 @@ CREATE VIEW [ schema . ] view_name - + + +security_invoker (boolean) + + + If this option is set to true, it will cause all + access to underlying tables to be checked as referenced by the + invoking user, otherwise as the view owner (default). See below for + implementation details and quirks. + + + + + + All of the above options can be changed on existing views using ALTER VIEW. + @@ -265,13 +279,42 @@ CREATE VIEW vista AS SELECT text 'Hello World' AS hello; -Access to tables referenced in the view is determined by permissions of -the view owner. In some cases, this can be used to provide secure but -restricted access to the underlying tables. However, not all views are -secure against tampering; see for -details. Functions called in the view are treated the same as if they had -been called directly from the query using the view. Therefore the user of +By default, access to relations referenced in the view is determined +by permissions of the view owner. This can be used to provide secure +but restricted access to the underlying tables. H
Re: [PATCH] Add reloption for views to enable RLS
Thanks for reviewing! On 2/25/22 19:22, Dean Rasheed wrote: Re-reading this thread, I think I preferred the name "security_invoker". The main objection seemed to come from the potential confusion with SECURITY INVOKER/DEFINER functions, but I think that's really a different thing. As long as the documentation for the default behaviour is clear (which I think it was), then it should be easy to explain how a security invoker view behaves differently. Also, there's value in using the same terminology as other databases, because many users will already be familiar with the feature from those databases. That is also the main reason I preferred naming it "security_invoker" - it is consistent with other databases and eases transition from such systems. I kept "check_permissions_owner" for now. Constantly changing it around with each iteration doesn't really bring any value IMHO, I'd rather have a final consensus on how to name the option and *then* change it for good. Some other review comments: 1). This new comment: + +Be aware that USAGE privileges on schemas containing +the underlying base relations are not checked. + is not entirely accurate. It's more accurate to say that a user creating or replacing a view must have CREATE privileges on the schema containing the view and USAGE privileges on any schemas referred to in the view query, whereas a user using the view only needs USAGE privileges on the schema containing the view. (Note that, for the view creator, USAGE is required on any schema referred to in the query -- e.g., schemas containing functions as well as base relations.) Improved in the attached v9. 2). The patch is adding a new field to RangeTblEntry which seems to be unnecessary -- it's set, and copied around, but never read, so it should just be removed. I removed that field in v9 since it is indeed completely unused. I initially added it to be consistent with the "security_barrier" implementation and than somewhat forgot about it. 3). Looking at this change: [..] I think it should call setRuleCheckAsUser() in all cases. It might be true that the rule fetched has checkAsUser set to InvalidOid throughout its action and quals, but it seems unwise to rely on that -- better to code defensively and explicitly set it in all cases. It probably doesn't really matter, but I agree that coding defensively is always a good thing. Changed that in v9 to call setRuleCheckAsUser() either with ->relowner or InvalidOid. 4). In the same code block, I think the new behaviour should be applied to SELECT rules only. The view may have other non-SELECT rules (just as a table may have non-SELECT rules), created using CREATE RULE, but their actions are independent of the view definition. Currently their permissions are checked as the view/table owner, and if anyone wanted to change that, it should be an option on the rule, not the view (just as triggers can be made security definer or invoker, depending on how the trigger function is defined). Good catch, I added a additional check for rule->event and a test for that in v9. [ I also had to add a missing DROP statement to some previous test, just a heads up. ] It makes sense to mimic the behavior of triggers and further, user-created rules otherwise might behave differently for tables and views, depending on the view definition. [ But I'm not _that_ familiar with CREATE RULE, FWIW. ] 5). In the same function, the block of code that fetches rules and triggers has been moved. I think it would be worth adding a comment to explain why it's now important to extract the reloptions *before* fetching the relation's rules and triggers. Added a small comment explaining that in v9. 6). The second set of tests added to rowsecurity.sql seem to have nothing to do with RLS, and probably belong in updatable_views.sql, and I think it would be worth adding a few more tests for things like views on top of views. Seems reasonable to move them into updatable_views.sql, done that for v9. Further I added two (simple) tests for chained views as you mentioned, hope they reflect what you had in mind. Thanks, ChristophFrom a7e84c92761881419bf8d63ebfcc528417dc8d24 Mon Sep 17 00:00:00 2001 From: Christoph Heiss Date: Tue, 1 Mar 2022 17:36:42 +0100 Subject: [PATCH v9 1/1] Add new boolean reloption "check_permissions_owner" to views When this reloption is set to "false", all permissions on the underlying relations will be checked against the invoking user rather than the view owner. The latter remains the default behavior. Author: Christoph Heiss Co-Author: Laurenz Albe Reviewed-By: Laurenz Albe, Wolfgang Walther Discussion: https://postgr.es/m/b66dd6d6-ad3e-c6f2-8b90-47be773da240%40cybertec.at Signed-off-by: Christoph Heiss --- doc/src/sgml/ref/alter_view.sgml | 13 ++- doc/src/sgml/ref/create_view.sgml | 72
Re: [PATCH] Add reloption for views to enable RLS
Hi, On 2/15/22 09:37, walt...@technowledgy.de wrote: Christoph Heiss: xxx_owner=true would be the default and xxx_owner=false could be set explicitly to get the behavior we are looking for in this patch? I'm not sure if an option which is on by default would be best, IMHO. I would rather have an off-by-default option, so that you explicitly have to turn *on* that behavior rather than turning *off* the current. Just out of curiosity I asked myself whether there were any other boolean options that default to true in postgres - and there are plenty. ./configure options, client connection settings, server config options, etc - but also some SQL statements: - CREATE USER defaults to LOGIN - CREATE ROLE defaults to INHERIT - CREATE COLLATION defaults to DETERMINISTIC=true There's even reloptions, that do, e.g. vacuum_truncate. Knowing that I happily drop my objection about that. :^) [..] The more I think about it, the more it becomes clear that really the current default behavior of "running the query as the view owner" is the special thing here, not the behavior you are introducing. If we were to start from scratch, it would be pretty obvious - to me - that run_as_owner=false would be the default, and the run_as_owner=true would need to be turned on explicitly. I'm thinking about "run_as_owner" as the better design and "defaults to true" as a backwards compatibility thing. Right, if we treat that as a kind of "backwards-compatible" feature, having an reloption that is on by default makes sense. I converted the option to run_as_owner=true|false in the attached v7. It now definitely seems like the right way to move forward and getting more feedback. Thanks, Christoph HeissFrom 27b3c425bc000d32253a6c33057c75dfb2deb6ef Mon Sep 17 00:00:00 2001 From: Christoph Heiss Date: Tue, 15 Feb 2022 12:25:46 +0100 Subject: [PATCH v7 1/1] Add new boolean reloption "security_invoker" to views When this reloption is set to true, all permissions on the underlying objects will be checked against the invoking user rather than the view owner, as is currently implemented. Signed-off-by: Christoph Heiss Co-Author: Laurenz Albe Discussion: https://postgr.es/m/b66dd6d6-ad3e-c6f2-8b90-47be773da240%40cybertec.at --- doc/src/sgml/ref/alter_view.sgml | 12 +++- doc/src/sgml/ref/create_view.sgml | 74 ++- src/backend/access/common/reloptions.c| 11 src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/equalfuncs.c| 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/optimizer/plan/subselect.c| 3 + src/backend/optimizer/prep/prepjointree.c | 2 + src/backend/rewrite/rewriteHandler.c | 19 -- src/backend/utils/cache/relcache.c| 63 ++- src/include/nodes/parsenodes.h| 1 + src/include/utils/rel.h | 11 src/test/regress/expected/create_view.out | 46 +++--- src/test/regress/expected/rowsecurity.out | 65 +++- src/test/regress/sql/create_view.sql | 22 ++- src/test/regress/sql/rowsecurity.sql | 44 ++ 17 files changed, 316 insertions(+), 61 deletions(-) diff --git a/doc/src/sgml/ref/alter_view.sgml b/doc/src/sgml/ref/alter_view.sgml index 98c312c5bf..bc6184bfea 100644 --- a/doc/src/sgml/ref/alter_view.sgml +++ b/doc/src/sgml/ref/alter_view.sgml @@ -156,11 +156,21 @@ ALTER VIEW [ IF EXISTS ] name RESET Changes the security-barrier property of the view. The value must - be Boolean value, such as true + be a Boolean value, such as true or false. + +run_as_owner (boolean) + + + Changes the user as which the subquery is run. Default is + true. The value must be a Boolean value, such as + true or false. + + + diff --git a/doc/src/sgml/ref/create_view.sgml b/doc/src/sgml/ref/create_view.sgml index bf03287592..c34df4e746 100644 --- a/doc/src/sgml/ref/create_view.sgml +++ b/doc/src/sgml/ref/create_view.sgml @@ -137,8 +137,6 @@ CREATE VIEW [ schema . ] view_namelocal or cascaded, and is equivalent to specifying WITH [ CASCADED | LOCAL ] CHECK OPTION (see below). - This option can be changed on existing views using ALTER VIEW. @@ -152,7 +150,22 @@ CREATE VIEW [ schema . ] view_name - + + +run_as_owner (boolean) + + + Set by default. If this option is set to true, + it will cause all access to underlying tables to be checked as + referenced by the view owner, otherwise as the invoking user. + + + + + + All of the above options
Re: [PATCH] Add reloption for views to enable RLS
Hi all, again, many thanks for the reviews and testing! On 2/4/22 17:09, Laurenz Albe wrote: I also see no reason to split a small patch like this into three parts. I've split it into the three unrelated parts (code, docs, tests) to ease review, but I happily carry it as one patch too. In the attached, I dealt with the above and went over the comments. How do you like it? That is really nice, I used it to base v6 on. On 2/9/22 17:40, walt...@technowledgy.de wrote: Ah, good find. In that case, I suggest to change the docs slightly to say that the schema will not be checked. In one place it's described as "it will cause all access to the underlying tables to be checked as ..." which is fine, I think. But in another place it's "access to tables, functions and *other objects* referenced in the view, ..." which is misleading I removed the reference to "other objects" for now in v6. I agree that the name "security_invoker" is suggestive of SECURITY INVOKER in CREATE FUNCTION, but the behavior is different. Perhaps the solution is as simple as choosing a different name that does not prompt this association, for example "permissions_invoker". Yes, given that there is not much that can be done about the functionality anymore, a different name would be better. This would also avoid the implicit "if security_invoker=false, the view behaves like SECURITY DEFINER" association, which is also clearly wrong. And this assumption is actually what made me think the chained views example was somehow off. I am not convinced "permissions_invoker" is much better, though. The difference between SECURITY INVOKER and SECURITY DEFINER is invoker vs definer... where, I think, we need something else to describe what we currently have and what the patch provides. Maybe we can look at it from the other perspective: Both ways of operating keep the CURRENT_USER the same, pretty much like what we understand "security invoker" should do. The difference, however, is the current default in which the permissions are checked with the view *owner*. Let's treat this difference as the thing that can be set: security_owner=true|false. Or run_as_owner=true|false. xxx_owner=true would be the default and xxx_owner=false could be set explicitly to get the behavior we are looking for in this patch? I'm not sure if an option which is on by default would be best, IMHO. I would rather have an off-by-default option, so that you explicitly have to turn *on* that behavior rather than turning *off* the current. [ Pretty much bike-shedding here, but if the agreement comes to one of "xxx_owner" I won't mind it either. ] My best suggestions is maybe something like run_as_invoker=t|f, but that would probably raise the same "invoker vs definer" association. I left it for now as-is. I guess more documentation how this works would be a good idea. [...] but since it exposes this in new ways, it might as well clarify how all this works. I tried to clarify this situation in the documentation in a concise matter, I'd appreciate further feedback on that. Thanks, Christoph HeissFrom 7d8f8e1cb27a3d22d049a5d820ddd66ec77a3297 Mon Sep 17 00:00:00 2001 From: Christoph Heiss Date: Mon, 14 Feb 2022 17:54:35 +0100 Subject: [PATCH v6 1/1] Add new boolean reloption "security_invoker" to views When this reloption is set to true, all permissions on the underlying objects will be checked against the invoking user rather than the view owner, as is currently implemented. Signed-off-by: Christoph Heiss Co-Author: Laurenz Albe Discussion: https://postgr.es/m/b66dd6d6-ad3e-c6f2-8b90-47be773da240%40cybertec.at Signed-off-by: Christoph Heiss --- doc/src/sgml/ref/alter_view.sgml | 12 +++- doc/src/sgml/ref/create_view.sgml | 73 ++- src/backend/access/common/reloptions.c| 11 src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/equalfuncs.c| 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/optimizer/plan/subselect.c| 1 + src/backend/optimizer/prep/prepjointree.c | 1 + src/backend/rewrite/rewriteHandler.c | 20 +-- src/backend/utils/cache/relcache.c| 63 ++- src/include/nodes/parsenodes.h| 1 + src/include/utils/rel.h | 11 src/test/regress/expected/create_view.out | 46 +++--- src/test/regress/expected/rowsecurity.out | 65 +++- src/test/regress/sql/create_view.sql | 22 ++- src/test/regress/sql/rowsecurity.sql | 44 ++ 17 files changed, 313 insertions(+), 61 deletions(-) diff --git a/doc/src/sgml/ref/alter_view.sgml b/doc/src/sgml/ref/alter_view.sgml index 98c312c5bf..8bdc90a5a1 100644 --- a/doc/src/sgml/ref/alter_view.sgml +++ b/doc/src/sgml/ref/al
Re: [PATCH] Add reloption for views to enable RLS
Hi Laurenz,
thank you again for the review!
On 1/20/22 15:20, Laurenz Albe wrote:
[..]
I gave the new patch a spin, and got a surprising result:
[..]
INSERT INTO v VALUES (1);
INSERT 0 1
Huh? "duff" has no permission to insert into "tab"!
That really should not happen, thanks for finding that and helping me
investigating on how to fix that!
This is now solved by checking the security_invoker property on the view
in rewriteTargetView().
I've also added a testcase for this in v4 to catch that in future.
[..]
About the documentation:
--- a/doc/src/sgml/ref/create_view.sgml
+++ b/doc/src/sgml/ref/create_view.sgml
+
+security_invoker (boolean)
+
+
+ If this option is set, it will cause all access to the underlying
+ tables to be checked as referenced by the invoking user, rather than
+ the view owner. This will only take effect when row level security
is
+ enabled on the underlying tables (using
+ ALTER TABLE ... ENABLE ROW LEVEL SECURITY).
+
Why should this *only* take effect if (not "when") RLS is enabled?
The above test shows that there is an effect even without RLS.
+ This option can be changed on existing views using ALTER VIEW. See
+ for more details on row level
security.
+
I don't think that it is necessary to mention that this can be changed with
ALTER VIEW - all storage parameters can be. I guess you copied that from
the "check_option" documentation, but I would say it need not be mentioned
there either.
Exactly, I tried to fit it in with the existing parameters.
I moved the link to ALTER VIEW to the end of the paragraph, as it
applies to all options anyways.
+
+If the security_invoker option is set on the view,
+access to tables is determined by permissions of the invoking user, rather
+than the view owner. This can be used to provide stricter permission
+checking to the underlying tables than by default.
Since you are talking about use cases here, RLS might deserve a mention.
Expanded upon a little bit in v4.
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
+ {
+ {
+ "security_invoker",
+ "View subquery in invoked within the current security context.",
+ RELOPT_KIND_VIEW,
+ AccessExclusiveLock
+ },
+ false
+ },
That doesn't seem to be proper English.
Yes, that happened when rewriting this for v1 -> v2.
Fixed.
Thanks,
Christoph HeissFrom 01437a45bfd069080ffe0eb45288bfddd3de6009 Mon Sep 17 00:00:00 2001
From: Christoph Heiss
Date: Wed, 2 Feb 2022 16:44:38 +0100
Subject: [PATCH v4 1/3] Add new boolean reloption security_invoker to views
When this reloption is set to true, all references to the underlying tables will
be checked against the invoking user rather than the view owner, as is currently
implemented.
Signed-off-by: Christoph Heiss
---
src/backend/access/common/reloptions.c| 11
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/equalfuncs.c| 1 +
src/backend/nodes/outfuncs.c | 1 +
src/backend/nodes/readfuncs.c | 1 +
src/backend/optimizer/plan/subselect.c| 1 +
src/backend/optimizer/prep/prepjointree.c | 1 +
src/backend/rewrite/rewriteHandler.c | 17 --
src/backend/utils/cache/relcache.c| 63 +--
src/include/nodes/parsenodes.h| 1 +
src/include/utils/rel.h | 11
11 files changed, 77 insertions(+), 32 deletions(-)
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index d592655258..c7c62a0076 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -140,6 +140,15 @@ static relopt_bool boolRelOpts[] =
},
false
},
+ {
+ {
+ "security_invoker",
+ "Check permissions against underlying tables as the calling user, not as view owner",
+ RELOPT_KIND_VIEW,
+ AccessExclusiveLock
+ },
+ false
+ },
{
{
"vacuum_truncate",
@@ -1996,6 +2005,8 @@ view_reloptions(Datum reloptions, bool validate)
static const relopt_parse_elt tab[] = {
{"security_barrier", RELOPT_TYPE_BOOL,
offsetof(ViewOptions, security_barrier)},
+ {"security_invoker", RELOPT_TYPE_BOOL,
+ offsetof(ViewOptions, security_invoker)},
{"check_option", RELOPT_TYPE_ENUM,
offsetof(ViewOptions, check_option)}
};
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 90b5da51c9..b171992ba8 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2465,6 +2465,7 @@ _copyRangeTblEntry(const RangeTblEntry *from)
COPY_NODE_FIELD(tablesample);
COPY_NODE_FIELD(subquery);
COPY_SCALAR_FIELD(security_barrier);
+
Re: [PATCH] Add reloption for views to enable RLS
Hi,
On 1/19/22 09:30, Julien Rouhaud wrote:
Hi,
On Tue, Jan 18, 2022 at 04:16:53PM +0100, Christoph Heiss wrote:
I've attached a v2 where I addressed the things you mentioned.
This version unfortunately doesn't apply anymore:
http://cfbot.cputube.org/patch_36_3466.log
=== Applying patches on top of PostgreSQL commit ID
e0e567a106726f6709601ee7cffe73eb6da8084e ===
=== applying patch
./0001-PATCH-v2-1-3-Add-new-boolean-reloption-security_invo.patch
=== applying patch
./0002-PATCH-v2-2-3-Add-regression-tests-for-new-security_i.patch
patching file src/test/regress/expected/create_view.out
Hunk #5 FAILED at 2019.
Hunk #6 succeeded at 2056 (offset 16 lines).
1 out of 6 hunks FAILED -- saving rejects to file
src/test/regress/expected/create_view.out.rej
Could you send a rebased version?
My bad - I attached a new version rebased on latest master.
Thanks,
Christoph HeissFrom b1b5a6c6bd63c509b3bcdef6d1e7a548413c2a9d Mon Sep 17 00:00:00 2001
From: Christoph Heiss
Date: Tue, 18 Jan 2022 15:42:58 +0100
Subject: [PATCH 1/3] [PATCH v3 1/3] Add new boolean reloption security_invoker
to views
When this reloption is set to true, all references to the underlying tables will
be checked against the invoking user rather than the view owner, as is currently
implemented.
Row level security must be enabled on the tables for this to take effect.
Signed-off-by: Christoph Heiss
---
src/backend/access/common/reloptions.c| 11
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/equalfuncs.c| 1 +
src/backend/nodes/outfuncs.c | 1 +
src/backend/nodes/readfuncs.c | 1 +
src/backend/optimizer/plan/subselect.c| 1 +
src/backend/optimizer/prep/prepjointree.c | 1 +
src/backend/rewrite/rewriteHandler.c | 1 +
src/backend/utils/cache/relcache.c| 63 +--
src/include/nodes/parsenodes.h| 1 +
src/include/utils/rel.h | 11
11 files changed, 65 insertions(+), 28 deletions(-)
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index d592655258..beb170b5e6 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -140,6 +140,15 @@ static relopt_bool boolRelOpts[] =
},
false
},
+ {
+ {
+ "security_invoker",
+ "View subquery in invoked within the current security context.",
+ RELOPT_KIND_VIEW,
+ AccessExclusiveLock
+ },
+ false
+ },
{
{
"vacuum_truncate",
@@ -1996,6 +2005,8 @@ view_reloptions(Datum reloptions, bool validate)
static const relopt_parse_elt tab[] = {
{"security_barrier", RELOPT_TYPE_BOOL,
offsetof(ViewOptions, security_barrier)},
+ {"security_invoker", RELOPT_TYPE_BOOL,
+ offsetof(ViewOptions, security_invoker)},
{"check_option", RELOPT_TYPE_ENUM,
offsetof(ViewOptions, check_option)}
};
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 90b5da51c9..b171992ba8 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2465,6 +2465,7 @@ _copyRangeTblEntry(const RangeTblEntry *from)
COPY_NODE_FIELD(tablesample);
COPY_NODE_FIELD(subquery);
COPY_SCALAR_FIELD(security_barrier);
+ COPY_SCALAR_FIELD(security_invoker);
COPY_SCALAR_FIELD(jointype);
COPY_SCALAR_FIELD(joinmergedcols);
COPY_NODE_FIELD(joinaliasvars);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 06345da3ba..a832c5fefe 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2766,6 +2766,7 @@ _equalRangeTblEntry(const RangeTblEntry *a, const RangeTblEntry *b)
COMPARE_NODE_FIELD(tablesample);
COMPARE_NODE_FIELD(subquery);
COMPARE_SCALAR_FIELD(security_barrier);
+ COMPARE_SCALAR_FIELD(security_invoker);
COMPARE_SCALAR_FIELD(jointype);
COMPARE_SCALAR_FIELD(joinmergedcols);
COMPARE_NODE_FIELD(joinaliasvars);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 2b0236937a..883284ad0d 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -3261,6 +3261,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
case RTE_SUBQUERY:
WRITE_NODE_FIELD(subquery);
WRITE_BOOL_FIELD(security_barrier);
+ WRITE_BOOL_FIELD(security_invoker);
break;
case RTE_JOIN:
WRITE_ENUM_FIELD(jointype, JoinType);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 3f68f7c18d..ad825bb27f 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1444,6 +1444,7 @@ _readRangeTblEntry(void)
case RTE_SUBQUERY:
READ_NODE_FIELD(subquery);
READ_BOOL_FIELD(security_barrier);
+ READ_BOOL_FIELD(security_invoker);
break;
case RTE_JOIN:
READ_ENUM_FIELD(jointype, JoinType);
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 41bd1ae7d4..30
Re: [PATCH] Add reloption for views to enable RLS
Hi Laurenz,
thanks for the review!
I've attached a v2 where I addressed the things you mentioned.
On 1/11/22 19:59, Laurenz Albe wrote:
[..]
You made that an enum with only a single value.
What other values could you imagine in the future?
I think that this should be a boolean reloption, for example "security_definer".
If unset or set to "off", you would get the current behavior.
A boolean option would have been indeed the better choice, I agree.
I haven't though of any specific other values for this enum, it was
rather a decision following a off-list discussion.
I've changed the option to be boolean and renamed it to
"security_invoker". This puts it in line with how other systems (e.g.
MySQL) name their equivalent feature, so I think this should be an
appropriate choice.
Finally, patch 0003 updates the documentation for this new reloption.
[..]
Please avoid long lines like that.
Fixed.
Also, I don't think that the documentation on
RLS policies is the correct place for this. It should be on a page dedicated
to views
or permissions.
The CREATE VIEW page already has a paragraph about this, starting with
"Access to tables referenced in the view is determined by permissions of the view
owner."
This looks like the best place to me (and it would need to be adapted anyway).
It makes sense to put it there, thanks for the pointer! I wasn't really
that sure where to put the documentation to start with, and this seems
like a more appropriate place.
Please review further.
Thanks,
Christoph HeissFrom 25267e6b8a2ffd81f14acbee95ef08d9edf3d31c Mon Sep 17 00:00:00 2001
From: Christoph Heiss
Date: Tue, 18 Jan 2022 15:42:58 +0100
Subject: [PATCH 1/3] [PATCH v2 1/3] Add new boolean reloption security_invoker
to views
When this reloption is set to true, all references to the underlying tables will
be checked against the invoking user rather than the view owner, as is currently
implemented.
Row level security must be enabled on the tables for this to take effect.
Signed-off-by: Christoph Heiss
---
src/backend/access/common/reloptions.c| 11
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/equalfuncs.c| 1 +
src/backend/nodes/outfuncs.c | 1 +
src/backend/nodes/readfuncs.c | 1 +
src/backend/optimizer/plan/subselect.c| 1 +
src/backend/optimizer/prep/prepjointree.c | 1 +
src/backend/rewrite/rewriteHandler.c | 1 +
src/backend/utils/cache/relcache.c| 63 +--
src/include/nodes/parsenodes.h| 1 +
src/include/utils/rel.h | 11
11 files changed, 65 insertions(+), 28 deletions(-)
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5602f5323..3c84982fda 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -140,6 +140,15 @@ static relopt_bool boolRelOpts[] =
},
false
},
+ {
+ {
+ "security_invoker",
+ "View subquery in invoked within the current security context.",
+ RELOPT_KIND_VIEW,
+ AccessExclusiveLock
+ },
+ false
+ },
{
{
"vacuum_truncate",
@@ -1996,6 +2005,8 @@ view_reloptions(Datum reloptions, bool validate)
static const relopt_parse_elt tab[] = {
{"security_barrier", RELOPT_TYPE_BOOL,
offsetof(ViewOptions, security_barrier)},
+ {"security_invoker", RELOPT_TYPE_BOOL,
+ offsetof(ViewOptions, security_invoker)},
{"check_option", RELOPT_TYPE_ENUM,
offsetof(ViewOptions, check_option)}
};
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index df0b747883..6efaa07523 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2464,6 +2464,7 @@ _copyRangeTblEntry(const RangeTblEntry *from)
COPY_NODE_FIELD(tablesample);
COPY_NODE_FIELD(subquery);
COPY_SCALAR_FIELD(security_barrier);
+ COPY_SCALAR_FIELD(security_invoker);
COPY_SCALAR_FIELD(jointype);
COPY_SCALAR_FIELD(joinmergedcols);
COPY_NODE_FIELD(joinaliasvars);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index cb7ddd463c..7f0401fa84 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2766,6 +2766,7 @@ _equalRangeTblEntry(const RangeTblEntry *a, const RangeTblEntry *b)
COMPARE_NODE_FIELD(tablesample);
COMPARE_NODE_FIELD(subquery);
COMPARE_SCALAR_FIELD(security_barrier);
+ COMPARE_SCALAR_FIELD(security_invoker);
COMPARE_SCALAR_FIELD(jointype);
COMPARE_SCALAR_FIELD(joinmergedcols);
COMPARE_NODE_FIELD(joinaliasvars);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 91a89b6d51..7dee550856 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -3260,6 +3260,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
case RTE_SUBQUERY:
WRITE_NODE_FIELD(subquery)
[PATCH] Add reloption for views to enable RLS
Hi all! As part of a customer project we are looking to implement an reloption for views which when set, runs the subquery as invoked by the user rather than the view owner, as is currently the case. The rewrite rule's table references are then checked as if the user were referencing the table(s) directly. This feature is similar to so-called 'SECURITY INVOKER' views in other DBMS. Although such permission checking could be implemented using views which SELECT from a table function and further using triggers, that approach has obvious performance downsides. Our initial thought on implementing this was to simply add another reloption for views, just like the already existing `security_barrier`. With this in place, we then can conditionally evaluate in RelationBuildRuleLock() if we need to call setRuleCheckAsUser() or not. The new reloption has been named `security`, which is an enum currently only supporting a single value: `relation_permissions`. The code for fetching the rules and triggers in RelationBuildDesc() had to be moved after the parsing of the reloptions, since with this change RelationBuildRuleLock()now depends upon having relation->rd_options available. The current behavior of views without that new reloption set is unaltered. This is implemented as such in patch 0001. Regression tests are included for both the new reloption of CREATE VIEW and the row level security side of this too, contained in patch 0002. All regression tests are passing without errors. Finally, patch 0003 updates the documentation for this new reloption. An simplified example on how this feature can be used could look like this: CREATE TABLE people (id int, name text, company text); ALTER TABLE people ENABLE ROW LEVEL SECURITY; INSERT INTO people VALUES (1, 'alice', 'foo'), (2, 'bob', 'bar'); CREATE VIEW customers_no_security AS SELECT * FROM people; CREATE VIEW customers WITH (security=relation_permissions) AS SELECT * FROM people; -- We want carol to only see people from company 'foo' CREATE ROLE carol; CREATE POLICY company_foo_only ON people FOR ALL TO carol USING (company = 'foo'); GRANT SELECT ON people TO carol; GRANT SELECT ON customers_no_security TO carol; GRANT SELECT ON customers TO carol; Now using these tables as carol: postgres=# SET ROLE carol; SET For the `people` table, the policy is applied as expected: postgres=> SELECT * FROM people; id | name | company +---+- 1 | alice | foo (1 row) If we now use the view with the new relopt set, the policy is applied too: postgres=> SELECT * FROM customers; id | name | company +---+- 1 | alice | foo (1 row) But without the `security=relation_permissions` relopt, carol gets to see data they should not be able to due to the policy not being applied, since the rules are checked against the view owner: postgres=> SELECT * FROM customers_no_security; id | name | company +---+- 1 | alice | foo 2 | bob | bar (2 rows) Excluding regression tests and documentation, the changes boil down to this: src/backend/access/common/reloptions.c| 20 src/backend/nodes/copyfuncs.c | 1 src/backend/nodes/equalfuncs.c| 1 src/backend/nodes/outfuncs.c | 1 src/backend/nodes/readfuncs.c | 1 src/backend/optimizer/plan/subselect.c| 1 src/backend/optimizer/prep/prepjointree.c | 1 src/backend/rewrite/rewriteHandler.c | 1 src/backend/utils/cache/relcache.c| 62 src/include/nodes/parsenodes.h| 3 src/include/utils/rel.h | 21 11 files changed, 84 insertions(+), 29 deletions(-) All patches are against current master. Thanks, Christoph HeissFrom 2ed6b63adcebfff14965b8c9913ae0fafbe904a2 Mon Sep 17 00:00:00 2001 From: Christoph Heiss Date: Fri, 17 Dec 2021 17:17:54 +0100 Subject: [PATCH 3/3] Add documentation for new 'security' reloption on views --- doc/src/sgml/ddl.sgml | 4 doc/src/sgml/ref/alter_view.sgml | 9 + doc/src/sgml/ref/create_view.sgml | 18 ++ 3 files changed, 31 insertions(+) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 64d9030652..760ea2f794 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -2292,6 +2292,10 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw; are not subject to row security. + + For views, the policies are applied as being referenced through the view owner by default, rather than the user referencing the view. To apply row security policies as defined for the invoking user, the security option can be set on views (see CREATE VIEW) to get the same behavior. + + Row security policies can be specific to commands, or to roles, or to both. A policy can be specified to apply to ALL diff --git a/doc/
