*Chao Li* found an issue in my other patch for *pg_get_table_ddl.* Specifically, the existing pattern in pg_proc.dat for variadic-text functions (e.g., jsonb_delete, json_extract_path) uses *_text* instead of *text* at the variadic position in both proargtypes and proallargtypes, with provariadic => 'text'. This is the convention documented by the sanity check in src/test/regress/sql/opr_sanity.sql.
I realized the same issue was present in this patch as well, so I have fixed it and added corresponding test cases. The v12 patch is now ready for review and commit. On Mon, Jun 22, 2026 at 6:34 PM Akshay Joshi <[email protected]> wrote: > > > 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 >> >
v12-0001-Add-pg_get_policy_ddl-to-reconstruct-CREATE.patch
Description: Binary data
