| From: | Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com> |
|---|---|
| To: | Rui Zhao <zhaorui126(at)gmail(dot)com> |
| Cc: | solai v <solai(dot)cdac(at)gmail(dot)com>, Ilmar Y <tanswis42(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: [PATCH] Add pg_get_policy_ddl() function to reconstruct CREATE POLICY statement |
| Date: | 2026-07-09 10:57:41 |
| Message-ID: | CANxoLDd4uL=EU+4cGVWp1F9oqVaye4uybUkJhhS7QfeHaqeDXQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Thanks Rui for the review. I have tried to fix the issues you raised.
The v16 patch is ready for review/commit.
On Wed, Jul 8, 2026 at 10:28 PM Rui Zhao <zhaorui126(at)gmail(dot)com> wrote:
> Hi Akshay,
>
> I tested v15 on current master (57f93af36f): builds, make check passes, and
> basic reconstruction, pretty mode, NULL handling, and default-clause
> omission
> all look good. Two things.
>
> 1) Back in the v3/v4 discussion you decided the ON <table> name should
> always
> be schema-qualified for safety (the pg_get_triggerdef_worker thread with
> jian he), and that pretty here only controls formatting, not schema (Phil's
> point). That reasoning didn't reach the USING / WITH CHECK expressions,
> though: object references inside them are still qualified only by the
> caller's
> search_path, so they can lose their schema and rebind to a different object
> when the DDL is replayed elsewhere:
>
> CREATE FUNCTION s1.f(int) RETURNS bool LANGUAGE sql AS 'SELECT $1 > 0';
> CREATE FUNCTION s2.f(int) RETURNS bool LANGUAGE sql AS 'SELECT $1 < 0';
> CREATE POLICY pf ON t2 USING (s1.f(a));
>
> SET search_path = public, s1;
> SELECT ddl FROM pg_get_policy_ddl('t2', 'pf') AS ddl;
> -- CREATE POLICY pf ON public.t2 USING (f(a)); -- s1. dropped
>
> SET search_path = public, s2;
> CREATE POLICY pf ON t2 USING (f(a)); -- now s2.f,
> opposite meaning
>
> So within one statement the ON clause is always qualified but the
> expression
> body isn't -- and as Phil noted, the pretty flag can't fix this, since
> pg_get_expr qualifies by search_path visibility regardless of pretty.
>
> Worth settling the contract, given where this function sits. The old
> pg_get_viewdef / ruledef / indexdef functions are search_path-aware and
> leave
> qualification to the caller (pg_dump sets search_path empty around them so
> the
> output is portable). The new pg_get_*_ddl functions committed so far (role,
> database, tablespace) are on global, schemaless objects, so the question
> never
> came up -- pg_get_policy_ddl is the first of the family whose output embeds
> schema-qualifiable references. Your own pg_get_table_ddl patch already hit
> this and deparses under a controlled search_path (narrowed to pg_catalog),
> so
> the most consistent fix is to do the same here (NewGUCNestLevel +
> set_config);
> an empty search_path already yields the fully-qualified s1.f(a). If instead
> the intent is to follow the caller's search_path, that's fine too, but it
> should be documented (and SET search_path = '' noted as the way to get
> portable DDL).
>
> 2) The doc calls the second parameter policy_name, but the actual argument
> is
> policyname, so the documented named-argument call fails:
>
> SELECT * FROM pg_get_policy_ddl("table" => 't'::regclass,
> policy_name => 'p_all');
> ERROR: function pg_get_policy_ddl(table => regclass, policy_name
> => unknown) does not exist
>
> Regards,
> Rui
>
| Attachment | Content-Type | Size |
|---|---|---|
| v16-0001-Add-pg_get_policy_ddl-to-reconstruct-CREATE.patch | application/octet-stream | 34.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Alexander Korotkov | 2026-07-09 11:26:44 | Re: Tepid: selective index updates for heap relations |
| Previous Message | Tatsuro Yamada | 2026-07-09 10:55:50 | RE: Add enable_groupagg GUC parameter to control GroupAggregate usage |