| From: | Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com> |
|---|---|
| To: | Ilmar Y <tanswis42(at)gmail(dot)com> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: [PATCH] Add pg_get_policy_ddl() function to reconstruct CREATE POLICY statement |
| Date: | 2026-05-29 06:50:54 |
| Message-ID: | CANxoLDe7xiCWY-UEmzoK_uQdKh3PPNcGXJ3qdzFy3c0o_F+P_Q@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Thanks for the reviews.
My original patch (v9) was actually correct. After considering Japin's
review comment, I initially thought the extra parentheses weren't
necessary, but they are indeed required for handling boolean values
properly in non-pretty mode too, so I kept them in USING (%s) / WITH CHECK
(%s) for both modes.
`pg_get_expr()` only adds outer parentheses for composite expressions (via
the deparsers for `OpExpr`, `BoolExpr`, etc.). For atomic top-level nodes
like `Const`, `Var`, `current_user`, `NULL`, etc.
For example:
CREATE POLICY p ON t USING (true);
SELECT pg_get_policy_ddl('t', 'p'); -- previously: ... USING true;
(syntax error)
This is exactly why `pg_dump` always wraps the expression unconditionally;
see `src/bin/pg_dump/pg_dump.c`:4473-4477:
if (polinfo->polqual != NULL)
appendPQExpBuffer(query, " USING (%s)", polinfo->polqual);
if (polinfo->polwithcheck != NULL)
appendPQExpBuffer(query, " WITH CHECK (%s)", polinfo->polwithcheck);
I've also added a round-trip regression test with `USING (true)` / `WITH
CHECK (false)` that captures the generated DDL, drops the policies,
re-executes the DDL, and verifies the policies are recreated.
*v11 Patch attached for review.*
On Thu, May 28, 2026 at 7:12 PM Ilmar Y <tanswis42(at)gmail(dot)com> wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: not tested
> Implements feature: tested, failed
> Spec compliant: not tested
> Documentation: not tested
>
> Hi,
>
> I looked at v10, focused on whether the generated CREATE POLICY statement
> can be executed again.
>
> The patch applies cleanly on current master at
> 8a86aa313a714adc56c74e4b08793e4e6102b5ca.
>
> git diff --check reports no issues.
>
> I built with:
>
> ./configure --prefix="$PWD/pg-install" --without-readline --without-zlib
> --without-icu
> make -s -j8
> make -s install
>
> make -C src/test/regress check TESTS=rowsecurity
>
> ended up running the full parallel_schedule in this makefile; all 245 tests
> passed, including rowsecurity.
>
> I found one correctness issue in the generated non-pretty DDL. The code
> assumes that pg_get_expr_ext(..., false) already returns the parentheses
> required by CREATE POLICY syntax, but that is not true for simple boolean
> constants.
>
> For example:
>
> CREATE TABLE t(a int);
> CREATE POLICY p_true ON t USING (true);
> SELECT ddl FROM pg_get_policy_ddl('t', 'p_true', 'pretty', 'false') AS ddl;
>
> returns:
>
> CREATE POLICY p_true ON public.t USING true;
>
> If I drop the policy and execute that generated statement, it fails:
>
> ERROR: syntax error at or near "true"
> LINE 1: CREATE POLICY p_true ON public.t USING true;
> ^
>
> The same issue reproduces for WITH CHECK:
>
> CREATE POLICY p_check ON t FOR INSERT WITH CHECK (false);
>
> is reconstructed as:
>
> CREATE POLICY p_check ON public.t FOR INSERT WITH CHECK false;
>
> and executing it fails at "false".
>
> So I think USING and WITH CHECK need to be parenthesized in non-pretty mode
> too, or the tests should include a round-trip execution check for generated
> DDL with simple boolean expressions.
>
> I used two small SQL reproducers for the manual checks; the complete repro
> is
> included above.
>
> I have not reviewed the broader pg_get_*_ddl API design or every possible
> policy expression form.
>
> Regards,
> Ilmar Yunusov
>
> The new status of this patch is: Waiting on Author
>
| Attachment | Content-Type | Size |
|---|---|---|
| v11-0001-Add-pg_get_policy_ddl-function-to-reconstruct-CREATE.patch | application/x-patch | 25.5 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Jakub Wartak | 2026-05-29 07:25:58 | Re: PostgreSQL 19 Beta 1 release announcement draft |
| Previous Message | Michael Paquier | 2026-05-29 06:30:57 | Re: [PATCH] Prevent repeated deadlock-check signals in standby buffer pin waits |