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

Looks good to me. Would ideally like to see some tests: should be easy
enough to add to t/002_pg_dump.pl, but probably not worth it just for a
simple flag like this? We don't test a lot of other flags, but on the other
hand, that's what a test suite is supposed to do.

Cheers,
Greg

Thank you Greg.

002_pg_dump.pl already deals with CREATE POLICY and ALTER TABLE .. ENABLE
ROW LEVEL SECURITY, so I just added "--no-policies" there, to have basic
coverage.

Nik

On Fri, Jan 10, 2025 at 9:44 AM Greg Sabino Mullane <htamfids@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.

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

Re-reviewed this patch: still compiles, tests pass, and does what it says
on the tin. +1

Cheers,
Greg

--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support

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 database backups. Successfully tested using `pg_dump`, `pg_dumpall`, and `pg_restore`, confirming that policies are excluded upon restoration. The `admin_full_access` policy was correctly applied, granting full access to the `admin` role for the `users` table. Additionally, the `read_only_access` policy was verified to restrict the `readonly` role to only performing `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