On Mon, Jun 8, 2026 at 6:10 PM solai v <[email protected]> wrote:
> Hi all, > > > On Mon, Jun 8, 2026 at 5:32 PM Japin Li <[email protected]> wrote: > > > > 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 > > > > > 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. > Thanks for the review. The omission of FOR ALL and TO PUBLIC is intentional and matches the long-standing convention used by pg_get_indexdef, pg_get_constraintdef, pg_get_viewdef, etc.: emit a clause only when it differs from the parser's default. The result is semantically equivalent to the CREATE POLICY DDL. > > > Regards, > Solai >
