public inbox for [email protected]  

help / color / mirror / Atom feed

From: Japin Li <[email protected]>
To: Akshay Joshi <[email protected]>
Cc: Ilmar Y <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] Add pg_get_policy_ddl() function to reconstruct CREATE POLICY statement
Date: Fri, 29 May 2026 15:38:32 +0800
Message-ID: <SY7PR01MB1092118DD149062E230159D89B6162@SY7PR01MB10921.ausprd01.prod.outlook.com> (raw)
In-Reply-To: <CANxoLDe7xiCWY-UEmzoK_uQdKh3PPNcGXJ3qdzFy3c0o_F+P_Q@mail.gmail.com>
References: <CANxoLDdJsRJqnjMXV3yjsk07Z5iRWxG-c2hZJC7bAKqf8ZXj_A@mail.gmail.com>
	<CANxoLDffrZGRTGpW_sPQ-hPEYs0hgjaFgJQh3PJFpPu5Zsbgvg@mail.gmail.com>
	<177997571870.313758.10720313850275742354.pgcf@coridan.postgresql.org>
	<CANxoLDe7xiCWY-UEmzoK_uQdKh3PPNcGXJ3qdzFy3c0o_F+P_Q@mail.gmail.com>

On Fri, 29 May 2026 at 12:20, Akshay Joshi <[email protected]> wrote:
> 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.
>

My bad!  I had not considered this situation.

> `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 <[email protected]> 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

-- 
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Add pg_get_policy_ddl() function to reconstruct CREATE POLICY statement
  In-Reply-To: <SY7PR01MB1092118DD149062E230159D89B6162@SY7PR01MB10921.ausprd01.prod.outlook.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox