reorg / pg_repack

Reorganize tables in PostgreSQL databases with minimal locks
BSD 3-Clause "New" or "Revised" License
1.91k stars 177 forks source link

--no-superuser-check does not work #223

Closed bradnicholson closed 3 weeks ago

bradnicholson commented 4 years ago

The --no-superuser-check does not work. When passed in as an option, it fails with:

pg_repack -k  -U admin -d test  -t foo
INFO: repacking table "public.foo"
ERROR: query failed: ERROR:  must be superuser to use repack_apply function
DETAIL: query was: SELECT repack.repack_apply($1, $2, $3, $4, $5, $6)
ERROR: query failed: ERROR:  must be superuser to use repack_drop function
DETAIL: query was: SELECT repack.repack_drop($1, $2)
ERROR: query failed: ERROR:  must be superuser to use repack_drop function
DETAIL: query was: SELECT repack.repack_drop($1, $2)
ERROR: query failed: ERROR:  must be superuser to use repack_drop function
DETAIL: query was: SELECT repack.repack_drop($1, $2)
ERROR: query failed: ERROR:  must be superuser to use repack_drop function
DETAIL: query was: SELECT repack.repack_drop($1, $2)

Looking at the code - its failing here https://github.com/reorg/pg_repack/blob/master/lib/repack.c#L111

This always evaluates to the actual superuser status of the user.

I don't see superuser() defined anywhere in pg_repack - so I presume that is coming in from Postgres includes.

Initial discussion in #201

dvarrazzo commented 4 years ago

Thank you for looking into this. Can you provide a patch please?

schmiddy commented 4 years ago

IIRC the --no-superuser-check was added so that pg_repack could work on AWS Postgres RDS. The permissions in RDS are a little different from vanilla Postgres - users don't have available a full-fledged superuser role, rather they can get a role that belongs to the rds_superuser group. It's possible Postgres RDS has a slightly different implementation of that superuser() function underneath the hood.

https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/Appendix.PostgreSQL.CommonDBATasks.html

I'm not opposed to a change/fix here, but it would be nice if we could avoid breaking functionality for users on RDS.

dvarrazzo commented 4 years ago

We might event want to keep it simple and wontfix this, documenting that --no-superuser-check is for aws only

bradnicholson commented 4 years ago

The binary correctly checks and blocks non-super user access, and correctly respects the -k flag.

Is the secondary check in the backend code actually needed? Sure, it prevents non-superusers from calling the functions directly in the DB, but the binary (intends) to add that functionality anyway.

chobostar commented 4 years ago

We use own package with that patch:

diff --git a/lib/pg_repack.sql.in b/lib/pg_repack.sql.in
index 99003b6..7a6be23 100644
--- a/lib/pg_repack.sql.in
+++ b/lib/pg_repack.sql.in
@@ -328,3 +328,23 @@ LANGUAGE C STABLE STRICT;
 CREATE FUNCTION repack.get_table_and_inheritors(regclass) RETURNS regclass[] AS
 'MODULE_PATHNAME', 'repack_get_table_and_inheritors'
 LANGUAGE C STABLE STRICT;
+
+DO $$
+DECLARE
+  v_database_owner text;
+BEGIN
+    SELECT rolname
+    INTO v_database_owner
+    FROM pg_catalog.pg_roles u
+    JOIN pg_catalog.pg_database d ON d.datdba = u.oid
+    WHERE datname = current_database();
+
+    IF FOUND THEN
+        EXECUTE 'ALTER SCHEMA repack OWNER TO ' || quote_ident(v_database_owner);
+        EXECUTE 'GRANT ALL ON ALL TABLES IN SCHEMA repack TO ' || quote_ident(v_database_owner);
+        EXECUTE 'GRANT EXECUTE ON ALL FUNCTIONS IN SCHEMA repack TO ' || quote_ident(v_database_owner);
+
+        EXECUTE 'ALTER DEFAULT PRIVILEGES IN SCHEMA repack GRANT INSERT ON TABLES TO ' || quote_ident(v_database_owner);
+        EXECUTE 'ALTER DEFAULT PRIVILEGES IN SCHEMA repack GRANT USAGE, SELECT ON SEQUENCES TO ' || quote_ident(v_database_owner);
+    END IF;
+END;$$;
diff --git a/lib/repack.c b/lib/repack.c
index d75f256..9ef98ff 100644
--- a/lib/repack.c
+++ b/lib/repack.c
@@ -108,8 +108,10 @@ static void swap_heap_or_index_files(Oid r1, Oid r2);
 static void
 must_be_superuser(const char *func)
 {
+    /* disabled checks
    if (!superuser())
        elog(ERROR, "must be superuser to use %s function", func);
+    */
 }
evenfrost commented 2 years ago

This option also works for DigitalOcean's Managed Databases. Leaving this for future finders.

ahmedwonolo commented 1 year ago

In essence, --no-superuser-check makes no difference. If it only suppresses the message on client binary side, but the plugin still has the check. Wanting to run this tool with non superuser doesn't work on AWS!

MichaelDBA commented 1 year ago

Personally, I don't see why a non-superuser should be allowed to use pg_repack since I consider this a purely DBA/administrative activity. But perhaps there are exceptions to this rule that I'm not considering.

vadim2404 commented 1 year ago

Personally, I don't see why a non-superuser should be allowed to use pg_repack since I consider this a purely DBA/administrative activity. But perhaps there are exceptions to this rule that I'm not considering.

Some hosted providers do not allow privileged access. For instance, AWS RDS.

MichaelDBA commented 1 year ago

@vadim2404 , AWS RDS allows privileged access. rds_superuser is privileged access.

vadim2404 commented 1 year ago

But still, it requires -k option.

https://www.magistratehq.com/blog/using-pgrepack-in-aws-rds/

MichaelDBA commented 1 year ago

Ha, ok, you got me on that one! Although I can't get used to the thought that rds_superuser is not a superuser. True, it cannot do everything that a OnPrem superuser can do, but...

vadim2404 commented 1 year ago

Yes, not everything is possible under the rds_superuser. Therefore this flag is helpful

airhorns commented 5 months ago

It'd be great to have formal support for running pg_repack as a non-superuser. We have a user in production with escalated permissions that manages tables with dynamic schemas (which we use to improve performance), and we need to manage bloat on those dynamic tables, but don't really want to have superuser credentials floating in memory of that user. It should be able to repack tables it owns and has all permissions on, just not everything else. Are there any technical blockers to making that work?

za-arthur commented 3 weeks ago

pg_repack can be run by an owner by the PR https://github.com/reorg/pg_repack/pull/431 now.