Fix GRANT/REVOKE ALL IN SCHEMA handling #3712

nikkhils · 2021-10-18T13:34:05Z

Fix the "GRANT/REVOKE ALL IN SCHEMA" handling uniformly across
single-node and multi-node.

Even thought this is a SCHEMA specific activity, we decided to
include the chunks even if they are part of another SCHEMA. So
they will also end up getting/resetting the same privileges.

Includes test case changes for both single-node and multi-node use
cases.

fabriziomello

LGTM

pmwkaa · 2021-10-19T09:36:23Z

tsl/src/remote/dist_ddl.c

+					 * SCHEMA. This means that if the chunks are in another SCHEMA
+					 * then they won't get these privileges.
+					 */
+					foreach (cell, stmt->objects)


We usually don't support mixing distributed tables and non-distributed tables in other DDL queries, maybe we should apply same logic here?

We could do that. Also, it depends on what semantics we want to provide for CREATE SCHEMA implementation for multi-node in general. If we decide to support shipping of the CREATE SCHEMA then the schema will always be present on all nodes.

The problem with local tables, which not exists on data nodes. If you include them in the query, it will work on the AN but fail on DN

pmwkaa · 2021-10-19T09:45:13Z

tsl/test/sql/dist_grant.sql

+FROM generate_series('2018-12-01 00:00'::timestamp, '2018-12-10 00:00'::timestamp, '1h') AS time;
+
+-- GRANT ALL and check privileges
+GRANT ALL ON ALL TABLES  IN SCHEMA public TO :ROLE_DEFAULT_PERM_USER;


I suggest to extend tests including more schema objects combining distributed and non-distributed schemas, if we really want to support it

src/hypertable.c

codecov · 2021-10-21T06:53:43Z

Codecov Report

Merging #3712 (98d696c) into master (b0886c1) will decrease coverage by 0.04%.
The diff coverage is 79.04%.

@@            Coverage Diff             @@
##           master    #3712      +/-   ##
==========================================
- Coverage   90.31%   90.27%   -0.05%     
==========================================
  Files         212      212              
  Lines       37095    37178      +83     
==========================================
+ Hits        33502    33561      +59     
- Misses       3593     3617      +24

Impacted Files	Coverage Δ
tsl/src/remote/dist_ddl.c	`91.14% <54.76%> (-5.87%)`	⬇️
src/process_utility.c	`94.33% <94.00%> (-0.10%)`	⬇️
src/hypertable.c	`88.05% <100.00%> (+0.03%)`	⬆️
tsl/src/nodes/data_node_dispatch.c	`97.10% <0.00%> (-0.25%)`	⬇️
tsl/src/bgw_policy/job.c	`87.50% <0.00%> (-0.06%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0886c1...98d696c. Read the comment docs.

src/hypertable.c

pmwkaa · 2021-10-21T10:36:51Z

src/process_utility.c

@@ -1282,10 +1282,120 @@ process_grant_add_by_rel(GrantStmt *stmt, RangeVar *relation)
 	stmt->objects = lappend(stmt->objects, relation);
 }

+static bool
+check_rv_list(List *rvlist, Name schema_name, Name table_name)


I suggest to rename the function to represent what check here means. eg: find a table in a list, check table existence in the list, etc

rv stands for rangevar which typically contains schema qualified table names. Can change it to check_table_in_rangevar_list

pmwkaa · 2021-10-21T10:42:11Z

tsl/src/remote/dist_ddl.c

+			{
+				GrantStmt *stmt = castNode(GrantStmt, args->parsetree);
+
+				if (stmt->objtype == OBJECT_TABLE)


Should we check the list to make sure, that all tables here are distributed before allowing it?

The command that is shipped to the datanodes is STILL the original GRANT ... SCHEMA. This list is only processed locally.

I am curious where this table list comes in such case. For example GRANT .. ON [ TABLE ] table_name [, ...] assumes that the table list is provided by the user. In case of SCHEMA is it auto-generated by PostgreSQL based on schema?

That's happening via the process_relations_in_namespace function that I added in this PR.

Ok. So because we decided to GRANT on a nested schemas, we can't just pass GRANT ON SCHEMA for each schema to data nodes, we also need to generate GRANT ON TABLE in some cases. I am curious how this works in such case, because we ignore recursive DDL commands in the ddl_command_start hook in case if command is already been scheduled.

pmwkaa · 2021-10-21T10:47:24Z

tsl/src/remote/dist_ddl.c

+	{
+		switch (tag)
+		{
+			case T_GrantStmt:


I guess this path execution path is for regular GRANT command, not the ALL IN SCHEMA?

We will only come to this code path if the command is ALL IN SCHEMA type because only in that case will we have more than one hypertable. The dist_ddl_inspect_hypertable_list function below is the one which actually checks if the involved hypertable is also distributed or not.

pmwkaa

Looks good overall, some points:

Since we generate a list of tables for the GRANT command, it would be interesting to understand, what is the limit there.
Also would be helpful to have tests, when a schema has both distributed and non-distributed hypertables.

Fix the "GRANT/REVOKE ALL IN SCHEMA" handling uniformly across single-node and multi-node. Even thought this is a SCHEMA specific activity, we decided to include the chunks even if they are part of another SCHEMA. So they will also end up getting/resetting the same privileges. Includes test case changes for both single-node and multi-node use cases.

@binakot

This release adds major new features since the 2.4.2 release. We deem it moderate priority for upgrading. This release includes these noteworthy features: * Continuous Aggregates for Distributed Hypertables * Support for PostgreSQL 14 * Experimental: Support for timezones in `time_bucket_ng()`, including the `origin` argument This release also includes several bug fixes. **Features** * timescale#3034 Add support for PostgreSQL 14 * timescale#3435 Add continuous aggregates for distributed hypertables * timescale#3505 Add support for timezones in `time_bucket_ng()` * timescale#3598 Improve evaluation of stable functions such as now() on access node * timescale#3717 Support transparent decompression on individual chunks **Bugfixes** * timescale#3580 Fix memory context bug executing TRUNCATE * timescale#3592 Allow alter column type on distributed hypertable * timescale#3618 Fix execution of refresh_caggs from user actions * timescale#3625 Add shared dependencies when creating chunk * timescale#3626 Fix memory context bug executing TRUNCATE * timescale#3627 Schema qualify UDTs in multi-node * timescale#3638 Allow owner change of a data node * timescale#3654 Fix index attnum mapping in reorder_chunk * timescale#3661 Fix SkipScan path generation with constant DISTINCT column * timescale#3667 Fix compress_policy for multi txn handling * timescale#3673 Fix distributed hypertable DROP within a procedure * timescale#3701 Allow anyone to use size utilities on distributed hypertables * timescale#3708 Fix crash in get_aggsplit * timescale#3709 Fix ordered append pathkey check * timescale#3712 Fix GRANT/REVOKE ALL IN SCHEMA handling * timescale#3724 Fix inserts into compressed chunks on hypertables with caggs * timescale#3727 Fix DirectFunctionCall crash in distributed_exec * timescale#3728 Fix SkipScan with varchar column * timescale#3733 Fix ANALYZE crash with custom statistics for custom types * timescale#3747 Always reset expr context in DecompressChunk **Thanks** * @binakot and @sebvett for reporting an issue with DISTINCT queries * @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE * @mjf for reporting an issue with ordered append and JOINs * @phemmer for reporting the issues on multinode with aggregate queries and evaluation of now() * @abolognino for reporting an issue with INSERTs into compressed hypertables that have cagg * @tanglebones for reporting the ANALYZE crash with custom types on multinode

@binakot

This release adds major new features since the 2.4.2 release. We deem it moderate priority for upgrading. This release includes these noteworthy features: * Continuous Aggregates for Distributed Hypertables * Support for PostgreSQL 14 * Experimental: Support for timezones in `time_bucket_ng()`, including the `origin` argument This release also includes several bug fixes. **Features** * timescale#3034 Add support for PostgreSQL 14 * timescale#3435 Add continuous aggregates for distributed hypertables * timescale#3505 Add support for timezones in `time_bucket_ng()` **Bugfixes** * timescale#3580 Fix memory context bug executing TRUNCATE * timescale#3592 Allow alter column type on distributed hypertable * timescale#3598 Improve evaluation of stable functions such as now() on access node * timescale#3618 Fix execution of refresh_caggs from user actions * timescale#3625 Add shared dependencies when creating chunk * timescale#3626 Fix memory context bug executing TRUNCATE * timescale#3627 Schema qualify UDTs in multi-node * timescale#3638 Allow owner change of a data node * timescale#3654 Fix index attnum mapping in reorder_chunk * timescale#3661 Fix SkipScan path generation with constant DISTINCT column * timescale#3667 Fix compress_policy for multi txn handling * timescale#3673 Fix distributed hypertable DROP within a procedure * timescale#3701 Allow anyone to use size utilities on distributed hypertables * timescale#3708 Fix crash in get_aggsplit * timescale#3709 Fix ordered append pathkey check * timescale#3712 Fix GRANT/REVOKE ALL IN SCHEMA handling * timescale#3717 Support transparent decompression on individual chunks * timescale#3724 Fix inserts into compressed chunks on hypertables with caggs * timescale#3727 Fix DirectFunctionCall crash in distributed_exec * timescale#3728 Fix SkipScan with varchar column * timescale#3733 Fix ANALYZE crash with custom statistics for custom types * timescale#3747 Always reset expr context in DecompressChunk **Thanks** * @binakot and @sebvett for reporting an issue with DISTINCT queries * @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE * @mjf for reporting an issue with ordered append and JOINs * @phemmer for reporting the issues on multinode with aggregate queries and evaluation of now() * @abolognino for reporting an issue with INSERTs into compressed hypertables that have cagg * @tanglebones for reporting the ANALYZE crash with custom types on multinode

@binakot

This release adds major new features since the 2.4.2 release. We deem it moderate priority for upgrading. This release includes these noteworthy features: * Continuous Aggregates for Distributed Hypertables * Support for PostgreSQL 14 * Experimental: Support for timezones in `time_bucket_ng()`, including the `origin` argument This release also includes several bug fixes. **Features** * #3034 Add support for PostgreSQL 14 * #3435 Add continuous aggregates for distributed hypertables * #3505 Add support for timezones in `time_bucket_ng()` **Bugfixes** * #3580 Fix memory context bug executing TRUNCATE * #3592 Allow alter column type on distributed hypertable * #3598 Improve evaluation of stable functions such as now() on access node * #3618 Fix execution of refresh_caggs from user actions * #3625 Add shared dependencies when creating chunk * #3626 Fix memory context bug executing TRUNCATE * #3627 Schema qualify UDTs in multi-node * #3638 Allow owner change of a data node * #3654 Fix index attnum mapping in reorder_chunk * #3661 Fix SkipScan path generation with constant DISTINCT column * #3667 Fix compress_policy for multi txn handling * #3673 Fix distributed hypertable DROP within a procedure * #3701 Allow anyone to use size utilities on distributed hypertables * #3708 Fix crash in get_aggsplit * #3709 Fix ordered append pathkey check * #3712 Fix GRANT/REVOKE ALL IN SCHEMA handling * #3717 Support transparent decompression on individual chunks * #3724 Fix inserts into compressed chunks on hypertables with caggs * #3727 Fix DirectFunctionCall crash in distributed_exec * #3728 Fix SkipScan with varchar column * #3733 Fix ANALYZE crash with custom statistics for custom types * #3747 Always reset expr context in DecompressChunk **Thanks** * @binakot and @sebvett for reporting an issue with DISTINCT queries * @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE * @mjf for reporting an issue with ordered append and JOINs * @phemmer for reporting the issues on multinode with aggregate queries and evaluation of now() * @abolognino for reporting an issue with INSERTs into compressed hypertables that have cagg * @tanglebones for reporting the ANALYZE crash with custom types on multinode

@binakot

This release adds major new features since the 2.4.2 release. We deem it moderate priority for upgrading. This release includes these noteworthy features: * Continuous Aggregates for Distributed Hypertables * Support for PostgreSQL 14 * Experimental: Support for timezones in `time_bucket_ng()`, including the `origin` argument This release also includes several bug fixes. **Features** * #3034 Add support for PostgreSQL 14 * #3435 Add continuous aggregates for distributed hypertables * #3505 Add support for timezones in `time_bucket_ng()` **Bugfixes** * #3580 Fix memory context bug executing TRUNCATE * #3592 Allow alter column type on distributed hypertable * #3598 Improve evaluation of stable functions such as now() on access node * #3618 Fix execution of refresh_caggs from user actions * #3625 Add shared dependencies when creating chunk * #3626 Fix memory context bug executing TRUNCATE * #3627 Schema qualify UDTs in multi-node * #3638 Allow owner change of a data node * #3654 Fix index attnum mapping in reorder_chunk * #3661 Fix SkipScan path generation with constant DISTINCT column * #3667 Fix compress_policy for multi txn handling * #3673 Fix distributed hypertable DROP within a procedure * #3701 Allow anyone to use size utilities on distributed hypertables * #3708 Fix crash in get_aggsplit * #3709 Fix ordered append pathkey check * #3712 Fix GRANT/REVOKE ALL IN SCHEMA handling * #3717 Support transparent decompression on individual chunks * #3724 Fix inserts into compressed chunks on hypertables with caggs * #3727 Fix DirectFunctionCall crash in distributed_exec * #3728 Fix SkipScan with varchar column * #3733 Fix ANALYZE crash with custom statistics for custom types * #3747 Always reset expr context in DecompressChunk **Thanks** * @binakot and @sebvett for reporting an issue with DISTINCT queries * @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE * @mjf for reporting an issue with ordered append and JOINs * @phemmer for reporting the issues on multinode with aggregate queries and evaluation of now() * @abolognino for reporting an issue with INSERTs into compressed hypertables that have cagg * @tanglebones for reporting the ANALYZE crash with custom types on multinode

@binakot

This release adds major new features since the 2.4.2 release. We deem it moderate priority for upgrading. This release includes these noteworthy features: * Continuous Aggregates for Distributed Hypertables * Support for PostgreSQL 14 * Experimental: Support for timezones in `time_bucket_ng()`, including the `origin` argument This release also includes several bug fixes. **Features** * #3034 Add support for PostgreSQL 14 * #3435 Add continuous aggregates for distributed hypertables * #3505 Add support for timezones in `time_bucket_ng()` **Bugfixes** * #3580 Fix memory context bug executing TRUNCATE * #3592 Allow alter column type on distributed hypertable * #3598 Improve evaluation of stable functions such as now() on access node * #3618 Fix execution of refresh_caggs from user actions * #3625 Add shared dependencies when creating chunk * #3626 Fix memory context bug executing TRUNCATE * #3627 Schema qualify UDTs in multi-node * #3638 Allow owner change of a data node * #3654 Fix index attnum mapping in reorder_chunk * #3661 Fix SkipScan path generation with constant DISTINCT column * #3667 Fix compress_policy for multi txn handling * #3673 Fix distributed hypertable DROP within a procedure * #3701 Allow anyone to use size utilities on distributed hypertables * #3708 Fix crash in get_aggsplit * #3709 Fix ordered append pathkey check * #3712 Fix GRANT/REVOKE ALL IN SCHEMA handling * #3717 Support transparent decompression on individual chunks * #3724 Fix inserts into compressed chunks on hypertables with caggs * #3727 Fix DirectFunctionCall crash in distributed_exec * #3728 Fix SkipScan with varchar column * #3733 Fix ANALYZE crash with custom statistics for custom types * #3747 Always reset expr context in DecompressChunk **Thanks** * @binakot and @sebvett for reporting an issue with DISTINCT queries * @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE * @mjf for reporting an issue with ordered append and JOINs * @phemmer for reporting the issues on multinode with aggregate queries and evaluation of now() * @abolognino for reporting an issue with INSERTs into compressed hypertables that have cagg * @tanglebones for reporting the ANALYZE crash with custom types on multinode

@binakot

This release adds major new features since the 2.4.2 release. We deem it moderate priority for upgrading. This release includes these noteworthy features: * Continuous Aggregates for Distributed Hypertables * Support for PostgreSQL 14 * Experimental: Support for timezones in `time_bucket_ng()`, including the `origin` argument This release also includes several bug fixes. **Features** * #3034 Add support for PostgreSQL 14 * #3435 Add continuous aggregates for distributed hypertables * #3505 Add support for timezones in `time_bucket_ng()` **Bugfixes** * #3580 Fix memory context bug executing TRUNCATE * #3592 Allow alter column type on distributed hypertable * #3598 Improve evaluation of stable functions such as now() on access node * #3618 Fix execution of refresh_caggs from user actions * #3625 Add shared dependencies when creating chunk * #3626 Fix memory context bug executing TRUNCATE * #3627 Schema qualify UDTs in multi-node * #3638 Allow owner change of a data node * #3654 Fix index attnum mapping in reorder_chunk * #3661 Fix SkipScan path generation with constant DISTINCT column * #3667 Fix compress_policy for multi txn handling * #3673 Fix distributed hypertable DROP within a procedure * #3701 Allow anyone to use size utilities on distributed hypertables * #3708 Fix crash in get_aggsplit * #3709 Fix ordered append pathkey check * #3712 Fix GRANT/REVOKE ALL IN SCHEMA handling * #3717 Support transparent decompression on individual chunks * #3724 Fix inserts into compressed chunks on hypertables with caggs * #3727 Fix DirectFunctionCall crash in distributed_exec * #3728 Fix SkipScan with varchar column * #3733 Fix ANALYZE crash with custom statistics for custom types * #3747 Always reset expr context in DecompressChunk **Thanks** * @binakot and @sebvett for reporting an issue with DISTINCT queries * @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE * @mjf for reporting an issue with ordered append and JOINs * @phemmer for reporting the issues on multinode with aggregate queries and evaluation of now() * @abolognino for reporting an issue with INSERTs into compressed hypertables that have cagg * @tanglebones for reporting the ANALYZE crash with custom types on multinode

nikkhils self-assigned this Oct 18, 2021

nikkhils requested a review from a team as a code owner October 18, 2021 13:34

nikkhils requested review from berkley, svenklemm and duncan-tsdb and removed request for a team October 18, 2021 13:34

nikkhils force-pushed the grant_revoke branch from 3bdda2c to ba07034 Compare October 18, 2021 13:35

nikkhils linked an issue Oct 18, 2021 that may be closed by this pull request

Handle GRANT/REVOKE ALL ON ALL TABLES IN SCHEMA #3595

Closed

nikkhils force-pushed the grant_revoke branch from ba07034 to 0f2c1ea Compare October 18, 2021 13:47

fabriziomello approved these changes Oct 18, 2021

View reviewed changes

pmwkaa reviewed Oct 19, 2021

View reviewed changes

fabriziomello reviewed Oct 19, 2021

View reviewed changes

src/hypertable.c Show resolved Hide resolved

nikkhils force-pushed the grant_revoke branch 3 times, most recently from a1462a4 to f081a5b Compare October 21, 2021 06:39

nikkhils force-pushed the grant_revoke branch from f081a5b to 97cbb3a Compare October 21, 2021 07:04

pmwkaa reviewed Oct 21, 2021

View reviewed changes

pmwkaa approved these changes Oct 22, 2021

View reviewed changes

nikkhils force-pushed the grant_revoke branch from 97cbb3a to fab2bdc Compare October 22, 2021 10:39

nikkhils force-pushed the grant_revoke branch from fab2bdc to 98d696c Compare October 22, 2021 10:41

nikkhils merged commit 6869785 into timescale:master Oct 22, 2021

nikkhils deleted the grant_revoke branch October 22, 2021 11:18

fabriziomello mentioned this pull request Oct 27, 2021

Release 2.5.0 #3744

Merged

mfundul mentioned this pull request Oct 19, 2022

Fix tests which fail on PG15 #4844

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GRANT/REVOKE ALL IN SCHEMA handling #3712

Fix GRANT/REVOKE ALL IN SCHEMA handling #3712

nikkhils commented Oct 18, 2021 •

edited

Loading

fabriziomello left a comment

pmwkaa Oct 19, 2021

nikkhils Oct 21, 2021

pmwkaa Oct 21, 2021

pmwkaa Oct 19, 2021

codecov bot commented Oct 21, 2021 •

edited

Loading

pmwkaa Oct 21, 2021

nikkhils Oct 21, 2021

pmwkaa Oct 21, 2021

nikkhils Oct 21, 2021

pmwkaa Oct 21, 2021

nikkhils Oct 21, 2021

pmwkaa Oct 22, 2021

pmwkaa Oct 21, 2021

nikkhils Oct 21, 2021

pmwkaa left a comment

Fix GRANT/REVOKE ALL IN SCHEMA handling #3712

Fix GRANT/REVOKE ALL IN SCHEMA handling #3712

Conversation

nikkhils commented Oct 18, 2021 • edited Loading

fabriziomello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 21, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmwkaa left a comment

Choose a reason for hiding this comment

nikkhils commented Oct 18, 2021 •

edited

Loading

codecov bot commented Oct 21, 2021 •

edited

Loading