MIME-Version: 1.0
References: 
 <CANxoLDdJsRJqnjMXV3yjsk07Z5iRWxG-c2hZJC7bAKqf8ZXj_A@mail.gmail.com>
In-Reply-To: 
 <CANxoLDdJsRJqnjMXV3yjsk07Z5iRWxG-c2hZJC7bAKqf8ZXj_A@mail.gmail.com>
From: Philip Alger <paalger0@gmail.com>
Date: Wed, 15 Oct 2025 12:30:35 -0500
Message-ID: 
 <CAPXBC8+i=c7FCcGr6OR0y=mcx3EfdXuxyk_XYgSQ7+egGvb8vA@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: pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="000000000000601da6064135dcf3"
Archived-At: 
 <https://www.postgresql.org/message-id/CAPXBC8%2Bi%3Dc7FCcGr6OR0y%3Dmcx3EfdXuxyk_XYgSQ7%2BegGvb8vA%40mail.gmail.com>
Precedence: bulk

--000000000000601da6064135dcf3
Content-Type: text/plain; charset="UTF-8"

Hi Akshay,

When applying the patch, I got a number of errors and the tests failed. I
think it stems from:

+ targetTable = relation_open(tableID, NoLock);
+ relation_close(targetTable, NoLock);


I changed them to use "AccessShareLock" and it worked, and it's probably
relevant to change table_close to "AccessShareLock" as well. You're just
reading from the table.

+ table_close(pgPolicyRel, RowExclusiveLock);


You might move "Datum valueDatum" to the top of the function. The
compiler screamed at me for that, so I went ahead and made that change and
the screaming stopped.

+ /* Check if the policy has a TO list */
+ Datum valueDatum = heap_getattr(tuplePolicy,


I also don't think you need the extra parenthesis around "USING (%s)" and
""WITH CHECK (%s)" in the code; it seems to print it just fine without
them. Other people might have different opinions.

2) SELECT pg_get_policy_ddl('rls_tbl_1', 'rls_p8', true);   -- *pretty
> formatted DDL*
>                pg_get_policy_ddl
> ------------------------------------------------
>  CREATE POLICY rls_p8 ON rls_tbl_1
>          AS PERMISSIVE
>          FOR ALL
>          TO regress_rls_alice, regress_rls_dave
>          USING (true)
>  ;
>
>
As for the "pretty" part. In my opinion, I don't think it's necessary, and
putting the statement terminator (;) seems strange.
However, I think you're going to get a lot of opinions on what
well-formatted SQL looks like.

-- 
Best,
Phil Alger

--000000000000601da6064135dcf3
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi Akshay,<div><br></div><div>When applyi=
ng the patch, I got a number of errors and the tests failed. I think it ste=
ms from:</div><div><br></div></div><blockquote style=3D"margin:0 0 0 40px;b=
order:none;padding:0px"><div><div>+	targetTable =3D relation_open(tableID, =
NoLock);</div></div><div><div>+	relation_close(targetTable, NoLock);</div><=
/div></blockquote><div dir=3D"ltr"><div><br></div><div>I changed them to us=
e &quot;AccessShareLock&quot; and it worked, and it&#39;s probably relevant=
 to change table_close to &quot;AccessShareLock&quot; as well. You&#39;re j=
ust reading from the table.</div><div><br></div></div><blockquote style=3D"=
margin:0 0 0 40px;border:none;padding:0px"><div dir=3D"ltr"><div>+	table_cl=
ose(pgPolicyRel, RowExclusiveLock);</div></div></blockquote><div dir=3D"ltr=
"><div><br></div></div><div dir=3D"ltr"><div>You might move &quot;Datum val=
ueDatum&quot; to the top of the function. The compiler=C2=A0screamed at me =
for that, so I went ahead and made that change and the=C2=A0screaming stopp=
ed.=C2=A0</div><div><br></div><div><blockquote style=3D"margin:0px 0px 0px =
40px;border:none;padding:0px"><div><div>+ /* Check if the policy has a TO l=
ist */</div></div><div><div>+ Datum valueDatum =3D heap_getattr(tuplePolicy=
,</div></div></blockquote><div dir=3D"ltr"><div><br></div></div></div></div=
><div class=3D"gmail_quote gmail_quote_container"><div>I also don&#39;t thi=
nk you need the extra parenthesis around &quot;USING (%s)&quot; and &quot;&=
quot;WITH CHECK (%s)&quot; in the code; it seems to print it just fine with=
out them. Other people might have different opinions.</div><div><br></div><=
blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l=
eft:1px solid rgb(204,204,204);padding-left:1ex"><div dir=3D"ltr">2) SELECT=
 pg_get_policy_ddl(&#39;rls_tbl_1&#39;, &#39;rls_p8&#39;, true); =C2=A0 -- =
<b>pretty formatted DDL</b><br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0pg_get_policy_ddl=C2=A0<br>-----------------------------------=
-------------<br>=C2=A0CREATE POLICY rls_p8 ON rls_tbl_1=C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0AS PER=
MISSIVE<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0FOR ALL<br>=C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0TO regress_rls_alice, regress_rls_dave<br>=C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0USING (true)<br>=C2=A0;<br><br></div>
</blockquote></div><div><br clear=3D"all"></div><div>As for the &quot;prett=
y&quot; part. In my opinion, I don&#39;t think it&#39;s necessary,=C2=A0and=
 putting the statement terminator (;) seems strange.=C2=A0</div><div>Howeve=
r, I think you&#39;re going to get a lot of opinions on what well-formatted=
 SQL looks like.</div><div><br></div><span class=3D"gmail_signature_prefix"=
>-- </span><br><div dir=3D"ltr" class=3D"gmail_signature"><div dir=3D"ltr">=
Best,=C2=A0</div><div dir=3D"ltr">Phil Alger</div></div></div>

--000000000000601da6064135dcf3--