MIME-Version: 1.0
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>
 <SY7PR01MB1092118DD149062E230159D89B6162@SY7PR01MB10921.ausprd01.prod.outlook.com>
In-Reply-To: 
 <SY7PR01MB1092118DD149062E230159D89B6162@SY7PR01MB10921.ausprd01.prod.outlook.com>
From: solai v <solai.cdac@gmail.com>
Date: Mon, 8 Jun 2026 18:11:12 +0530
Message-ID: 
 <CAF0whuefRgV3_xgMhPoKaid1LN34CTLhTU-r5dgfFyEGVAFrPg@mail.gmail.com>
Subject: Re: [PATCH] Add pg_get_policy_ddl() function to reconstruct CREATE
 POLICY statement
To: Akshay Joshi <akshay.joshi@enterprisedb.com>
Cc: Ilmar Y <tanswis42@gmail.com>, pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAF0whuefRgV3_xgMhPoKaid1LN34CTLhTU-r5dgfFyEGVAFrPg%40mail.gmail.com>
Precedence: bulk

Hi all,


On Mon, Jun 8, 2026 at 5:32=E2=80=AFPM Japin Li <japinli@hotmail.com> wrote=
:
>
> On Fri, 29 May 2026 at 12:20, Akshay Joshi <akshay.joshi@enterprisedb.com=
> 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 handlin=
g 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 unconditional=
ly; see `src/bin/pg_dump/pg_dump.c`:4473-4477:
> >
> >     if (polinfo->polqual !=3D NULL)
> >         appendPQExpBuffer(query, " USING (%s)", polinfo->polqual);
> >     if (polinfo->polwithcheck !=3D NULL)
> >         appendPQExpBuffer(query, " WITH CHECK (%s)", polinfo->polwithch=
eck);
> >
> > I've also added a round-trip regression test with `USING (true)` / `WIT=
H 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=E2=80=AFPM Ilmar Y <tanswis42@gmail.com> w=
rote:
> >
> >  The following review has been posted through the commitfest applicatio=
n:
> >  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 statem=
ent
> >  can be executed again.
> >
> >  The patch applies cleanly on current master at
> >  8a86aa313a714adc56c74e4b08793e4e6102b5ca.
> >
> >  git diff --check reports no issues.
> >
> >  I built with:
> >
> >  ./configure --prefix=3D"$PWD/pg-install" --without-readline --without-=
zlib --without-icu
> >  make -s -j8
> >  make -s install
> >
> >  make -C src/test/regress check TESTS=3Drowsecurity
> >
> >  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 co=
de
> >  assumes that pg_get_expr_ext(..., false) already returns the parenthes=
es
> >  required by CREATE POLICY syntax, but that is not true for simple bool=
ean
> >  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 gene=
rated
> >  DDL with simple boolean expressions.
> >
> >  I used two small SQL reproducers for the manual checks; the complete r=
epro is
> >  included above.
> >
> >  I have not reviewed the broader pg_get_*_ddl API design or every possi=
ble
> >  policy expression form.
> >
> >  Regards,
> >  Ilmar Yunusov
> >
> >  The new status of this patch is: Waiting on Author
>


I reviewed and tested the V11 pg_get_policy_ddl() patch. I verified
the basic functionality by creating various row-level security
policies and checking the reconstructed DDL returned by
pg_get_policy_ddl(). The function correctly reconstructed policies for
different command types (SELECT, INSERT, UPDATE, DELETE), PERMISSIVE
and RESTRICTIVE policies, multiple role lists, quoted identifiers,
USING clauses, and WITH CHECK clauses.
I also tested more complex cases involving subqueries. Policies
containing EXISTS subqueries in both USING and WITH CHECK clauses were
reconstructed successfully. Nested subqueries were also handled
correctly, and I did not encounter any issues related to expression
reconstruction or variable handling. I verified NULL input handling as
well. Passing NULL values for the table name or policy name returned
no rows as expected. Invalid relation names resulted in the expected
regclass resolution error. One observation from testing is that the
generated DDL omits default clauses such as FOR ALL and TO PUBLIC. For
example, a policy created with FOR ALL TO PUBLIC is reconstructed
without those clauses, while still producing semantically equivalent
DDL. I'm not sure whether this is intentional or if the function is
expected to reproduce all clauses explicitly, but it may be worth
discussing.
Overall, the patch worked as expected in all scenarios I tested, and I
did not find any functional issues.


Regards,
Solai