Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wbeKV-002jiF-31 for pgsql-hackers@arkaria.postgresql.org; Mon, 22 Jun 2026 13:04:48 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wbeKU-006oCm-2R for pgsql-hackers@arkaria.postgresql.org; Mon, 22 Jun 2026 13:04:46 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wbeKU-006oCe-1J for pgsql-hackers@lists.postgresql.org; Mon, 22 Jun 2026 13:04:46 +0000 Received: from mail-ej1-x62e.google.com ([2a00:1450:4864:20::62e]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wbeKR-00000001l81-3F6d for pgsql-hackers@lists.postgresql.org; Mon, 22 Jun 2026 13:04:46 +0000 Received: by mail-ej1-x62e.google.com with SMTP id a640c23a62f3a-c07ea058c0cso710135266b.2 for ; Mon, 22 Jun 2026 06:04:42 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1782133482; cv=none; d=google.com; s=arc-20240605; b=lXgPEY6qSeX7zu9sPgX1TO2jtKO2DfRkjgnWO9GAdf2D5py/l8S6YMnRxjWsBpbcVB PCgwoDM3Ou+ElHTWp/ktvKdvyeMWjzGu7Olg3C99RVfFvaqDfK8TU4oGSSFGEpM9JH9u 22Hl8ey8QLfUtW33wBUMjSZQyEMpuq7pw3AdJo9NfmM2vos6L1Cd36qf9nuZri6l9Zmk GoG00sim0RgxT6IP+TDr0pMasMdXy0Rq9lBur97FD3xXlJy9CessTFy5vthehVE0Zl6b HiT3nKbggnPB+aunZLgG2xspXkuaAvon/FX3jRlvkXUD3DEME+VlHJ9MASLUXBtyKHgy Gdxg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=f4O1zovWdnkL4k/PC3uAEgCsxUEaFHAGDiu90Huelug=; fh=43BBsOV/H4zr+RvM2Fvx157Hg9iqEs/peSgeLZFMw5o=; b=KzmAFnRMdh2WvRItZT/zxm+B8gzFSJyDBs8W5K0KGO0ybkjc+WUVReMqvakeo/B9fP XkmkwHapuauDXzeRNXKBrTvuBi3msvw/1tui4Q4VzssY68F/kl+oMya0VdojZmFDgzd1 MCnL+C/S7LvUtmaPFyrXZ4YzEDCx6v46Lg51HQj6NlpQIFw5Fo/62jNxF7D762sJpn8v HmmSXfWCZUY2lawvefEcMOkwP0YaojStM/bquxQFJvuLiwbaJGyfKK35ZWd49+wu2hZR oKo3PoCsRlZoHPDeMStZEZpbILxRY3LbjV6OBEq9xxQEUltmSRWrUE7FNjdHaCfHia7G fUYA==; darn=lists.postgresql.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb.com; s=google; t=1782133482; x=1782738282; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=f4O1zovWdnkL4k/PC3uAEgCsxUEaFHAGDiu90Huelug=; b=i6NQoJOd+B8frvCcw7AVf21eoadI0CW/oDNAn8140fdnZF9/nopqIR45eGWsfWn/xq 2CF+LViQn4nuOvb/BMmbnVJDsoXdCsxQkRIeoZdkDZs2zC72ZcljrBpqGo3PKmmO55eY 1YbM0HsMX3/Y1gnfbxYY2sRqv7QKRLnpq+036BX0HBfTwCYpSl1NEG+BrtDeB5Jc00hB JE7dC+4bL+btWRjaPs0/oxVcVLXQVSt7rB2spRudF82NE3sJlyb+1pBV1m0WoDo+QHez spb7pfT4CUbwT4Va+60OXmK/p0ZnUa0pS2enFdLg1y8J9DzupzTSQ4Kdi9GSn2IanTAR VKAQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782133482; x=1782738282; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=f4O1zovWdnkL4k/PC3uAEgCsxUEaFHAGDiu90Huelug=; b=rgjN5lLefNGNs1b/YuiryllFnMClO1b6AtpcBjzLzPskXRcjuwIkRsNRRe8BWfNAbT lG/X/nQaNZkdq/pXVurpN8P0Gs3VKm2nyQPPytE+CFdPz1OHuXROWYXclSZ1+6tXMoc6 ioaNEWJVfoi3sxl2dQxstB6BDAw6YrUbipWMflWjDgMVwRuQ2U7X6IRFir7Bg0vtMuvG c8JoyndkIBiazvCFMkK6thIxucYQnXr7NyvULrfieci0kfOXzJIa74SDWrhrFxyO2VWO qhrSDKlY6+wyIqI7UgciyaSH5m3biovEKuSpvC29gU2mM3PY1OeSdShjDIafgPcHUOsZ D+9g== X-Forwarded-Encrypted: i=1; AFNElJ+77ON+05po4/nJZULjfm8/40CAe0dxGNnHoilALkWR49N7JVd+CQwMSMzRzX0IuwCoh5Icv1c15ykP3Bkz@lists.postgresql.org X-Gm-Message-State: AOJu0Yw8elr7LJx6UEpIdmPxaO9niBzAw9evDwV5wGsT962RZCtKrC3r 9F/7EMLx75fYji17lQfg+IPmKlDcmKeCJ0hx+P7hpNutTTl9f8POBeBM8Xoz73Zi444vPIA3SXU /8LBjdK6pjpmtMzmVjKKOQuelZk9SA4gN77MxNJJo X-Gm-Gg: AfdE7cnCzB6bJCj4Woa982EEBSgXI365e0R3KsQTW0Nq9gi8ZdowvWBSIxGvkVCj9ZL 9O1yoO9QPiPFwVtLBpdPNuEDgHXPEPd+EusrW5EXqjAL8/Ey6hq8NgQ5L+538NYgJ67GrHWRN9K sUkRk2rEmXBaPPZyakvecNBjglxabq3rLUz30L5QoHYYqri1UbTeauzjMQBowwkmdcfw/EaSvNi Ee+7VAzzA7Vw62lVR5FUcCPp+06+9hxEylvahRsUPFJbkSCzpsQqL9FybjPqJGRYYj+KyWeqw== X-Received: by 2002:a17:907:97c8:b0:bf9:cc72:6e05 with SMTP id a640c23a62f3a-c0b62137a6cmr605290066b.25.1782133481745; Mon, 22 Jun 2026 06:04:41 -0700 (PDT) MIME-Version: 1.0 References: <177997571870.313758.10720313850275742354.pgcf@coridan.postgresql.org> In-Reply-To: From: Akshay Joshi Date: Mon, 22 Jun 2026 18:34:29 +0530 X-Gm-Features: AVVi8CdkQrODAwxEXUarCa-CgBlSgJAd7mCMgG7w3Kqhvlgfi-uiFWnIIJNz1eI Message-ID: Subject: Re: [PATCH] Add pg_get_policy_ddl() function to reconstruct CREATE POLICY statement To: solai v Cc: Ilmar Y , pgsql-hackers@lists.postgresql.org Content-Type: multipart/alternative; boundary="0000000000001f5bf70654d749bd" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000001f5bf70654d749bd Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Jun 8, 2026 at 6:10=E2=80=AFPM solai v wrote= : > Hi all, > > > On Mon, Jun 8, 2026 at 5:32=E2=80=AFPM Japin Li wro= te: > > > > 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 > 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 !=3D NULL) > > > appendPQExpBuffer(query, " USING (%s)", polinfo->polqual); > > > if (polinfo->polwithcheck !=3D 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 ar= e > recreated. > > > > > > v11 Patch attached for review. > > > > > > On Thu, May 28, 2026 at 7:12=E2=80=AFPM Ilmar Y = 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=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 24= 5 > 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 > --0000000000001f5bf70654d749bd Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Mon, Jun 8, = 2026 at 6:10=E2=80=AFPM solai v <solai.cdac@gmail.com> wrote:
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 Ja= pin's review comment, I initially thought the extra
> > parentheses weren't necessary, but they are indeed required f= or handling boolean values properly in non-pretty mode too,
> > so I kept them in USING (%s) / WITH CHECK (%s) for both modes. > >
>
> My bad!=C2=A0 I had not considered this situation.
>
> > `pg_get_expr()` only adds outer parentheses for composite express= ions (via the deparsers for `OpExpr`, `BoolExpr`, etc.).
> > For atomic top-level nodes like `Const`, `Var`, `current_user`, `= NULL`, etc.
> > For example:
> >
> >=C2=A0 =C2=A0 =C2=A0CREATE POLICY p ON t USING (true);
> >=C2=A0 =C2=A0 =C2=A0SELECT pg_get_policy_ddl('t', 'p&#= 39;);=C2=A0 -- previously: ... USING true;=C2=A0 (syntax error)
> >
> > This is exactly why `pg_dump` always wraps the expression uncondi= tionally; see `src/bin/pg_dump/pg_dump.c`:4473-4477:
> >
> >=C2=A0 =C2=A0 =C2=A0if (polinfo->polqual !=3D NULL)
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0appendPQExpBuffer(query, " = USING (%s)", polinfo->polqual);
> >=C2=A0 =C2=A0 =C2=A0if (polinfo->polwithcheck !=3D NULL)
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0appendPQExpBuffer(query, " = WITH CHECK (%s)", polinfo->polwithcheck);
> >
> > I've also added a round-trip regression test with `USING (tru= e)` / `WITH CHECK (false)` that captures the generated DDL,
> > drops the policies, re-executes the DDL, and verifies the policie= s are recreated.
> >
> > v11 Patch attached for review.
> >
> > On Thu, May 28, 2026 at 7:12=E2=80=AFPM Ilmar Y <tanswis42@gmail.com> wro= te:
> >
> >=C2=A0 The following review has been posted through the commitfest= application:
> >=C2=A0 make installcheck-world:=C2=A0 not tested
> >=C2=A0 Implements feature:=C2=A0 =C2=A0 =C2=A0 =C2=A0tested, faile= d
> >=C2=A0 Spec compliant:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0not= tested
> >=C2=A0 Documentation:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 not= tested
> >
> >=C2=A0 Hi,
> >
> >=C2=A0 I looked at v10, focused on whether the generated CREATE PO= LICY statement
> >=C2=A0 can be executed again.
> >
> >=C2=A0 The patch applies cleanly on current master at
> >=C2=A0 8a86aa313a714adc56c74e4b08793e4e6102b5ca.
> >
> >=C2=A0 git diff --check reports no issues.
> >
> >=C2=A0 I built with:
> >
> >=C2=A0 ./configure --prefix=3D"$PWD/pg-install" --withou= t-readline --without-zlib --without-icu
> >=C2=A0 make -s -j8
> >=C2=A0 make -s install
> >
> >=C2=A0 make -C src/test/regress check TESTS=3Drowsecurity
> >
> >=C2=A0 ended up running the full parallel_schedule in this makefil= e; all 245 tests
> >=C2=A0 passed, including rowsecurity.
> >
> >=C2=A0 I found one correctness issue in the generated non-pretty D= DL.=C2=A0 The code
> >=C2=A0 assumes that pg_get_expr_ext(..., false) already returns th= e parentheses
> >=C2=A0 required by CREATE POLICY syntax, but that is not true for = simple boolean
> >=C2=A0 constants.
> >
> >=C2=A0 For example:
> >
> >=C2=A0 CREATE TABLE t(a int);
> >=C2=A0 CREATE POLICY p_true ON t USING (true);
> >=C2=A0 SELECT ddl FROM pg_get_policy_ddl('t', 'p_true&= #39;, 'pretty', 'false') AS ddl;
> >
> >=C2=A0 returns:
> >
> >=C2=A0 CREATE POLICY p_true ON public.t USING true;
> >
> >=C2=A0 If I drop the policy and execute that generated statement, = it fails:
> >
> >=C2=A0 ERROR:=C2=A0 syntax error at or near "true"
> >=C2=A0 LINE 1: CREATE POLICY p_true ON public.t USING true;
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^
> >
> >=C2=A0 The same issue reproduces for WITH CHECK:
> >
> >=C2=A0 CREATE POLICY p_check ON t FOR INSERT WITH CHECK (false); > >
> >=C2=A0 is reconstructed as:
> >
> >=C2=A0 CREATE POLICY p_check ON public.t FOR INSERT WITH CHECK fal= se;
> >
> >=C2=A0 and executing it fails at "false".
> >
> >=C2=A0 So I think USING and WITH CHECK need to be parenthesized in= non-pretty mode
> >=C2=A0 too, or the tests should include a round-trip execution che= ck for generated
> >=C2=A0 DDL with simple boolean expressions.
> >
> >=C2=A0 I used two small SQL reproducers for the manual checks; the= complete repro is
> >=C2=A0 included above.
> >
> >=C2=A0 I have not reviewed the broader pg_get_*_ddl API design or = every possible
> >=C2=A0 policy expression form.
> >
> >=C2=A0 Regards,
> >=C2=A0 Ilmar Yunusov
> >
> >=C2=A0 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 omissi= on of FOR ALL and TO PUBLIC is intentional and matches the long-standing co= nvention 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 re= sult is semantically equivalent to the CREATE POLICY DDL.=C2=A0


Regards,
Solai
--0000000000001f5bf70654d749bd--