Thread: Re: pg_dump, pg_dumpall, pg_restore: Add --no-policies option

On Wed, 15 Jan 2025 at 13:21, jian he <jian.universality@gmail.com> wrote:
>
> hi.
>
> around src/bin/pg_dump/pg_dump.c line 1117
> right after "ropt->no_comments = dopt.no_comments;"
> we also need add
>     ropt->no_policies = dopt.no_policies;
> ?
>
> overall looks good to me.
> The tests seem wrong per
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5499
> I have no idea how to improve the test.

Here is a rebased version along with the test failure fixes, please
accept the change if you are ok with it.

Regards,
Vignesh

Hi

On 27.02.25 15:37, vignesh C wrote:
> Here is a rebased version along with the test failure fixes, please
> accept the change if you are ok with it.


Patch LGTM. +1

It applies cleanly and works as described:


== pg_dump ==

$ /usr/local/postgres-dev/bin/pg_dump db > dump.out
$ grep "POLICY" dump.out | tee /dev/tty | wc -l
-- Name: t p; Type: POLICY; Schema: public; Owner: jim
CREATE POLICY p ON public.t FOR DELETE;
2

$ /usr/local/postgres-dev/bin/pg_dump db --no-policies > dump.out
$ grep "POLICY" dump.out | tee /dev/tty | wc -l
0

== pg_dumpall ==

$ /usr/local/postgres-dev/bin/pg_dumpall > dumpall.out
$ grep "POLICY" dumpall.out | tee /dev/tty | wc -l
-- Name: t p; Type: POLICY; Schema: public; Owner: jim
CREATE POLICY p ON public.t FOR DELETE;
2

$ /usr/local/postgres-dev/bin/pg_dumpall --no-policies > dumpall.out
$ grep "POLICY" dumpall.out | tee /dev/tty | wc -l
0

== pg_restore ==

$ /usr/local/postgres-dev/bin/pg_dump -Fc db > dump.out
$ /usr/local/postgres-dev/bin/pg_restore -l dump.out | grep POLICY | tee
/dev/tty | wc -l
3375; 3256 16388 POLICY public t p jim
1
$ /usr/local/postgres-dev/bin/pg_restore --no-policies -l dump.out |
grep POLICY | tee /dev/tty | wc -l
0


check-world passes and the documentation is consistent with the existing
"--no*" options of pg_dump, pg_dumpall, and pg_restore.

The new status of this patch is: Ready for Committer

Best regards, Jim

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Hi,
Tested this patch with `--no-policies` option works as expected by ensuring that policy definitions are not included in
databasebackups. Successfully tested using `pg_dump`, `pg_dumpall`, and `pg_restore`, confirming that policies are
excludedupon restoration. The `admin_full_access` policy was correctly applied, granting full access to the `admin`
rolefor the `users` table. Additionally, the `read_only_access` policy was verified to restrict the `readonly` role to
onlyperforming `SELECT` operations.
 

Regards,
Newt Global PostgreSQL Contributors

Greg Sabino Mullane <htamfids@gmail.com> writes:
> Re-reviewed this patch: still compiles, tests pass, and does what it says
> on the tin. +1

Pushed with minor corrections:

* The patch still hadn't absorbed jian he's point that pg_dump main()
needs to fill ropt->no_policies from dopt.no_policies.  It's possible
that that had no externally-visible effect, but just as a matter of
style we should fill the RestoreOptions fully.

* It seems better to me to implement the no_policies filter in
getPolicies() not dumpPolicy().  That way we don't waste effort
on retrieving catalog data we aren't going to use.  It might be
problematic if we had to deal with dependency chains involving
policies, but AFAIK there is nothing that depends on a policy.

* I fixed a couple bits of sloppy alphabetization and updated
the Perl style in the test script.

            regards, tom lane