[PATCH] Add pg_get_policy_ddl() function to reconstruct CREATE POLICY statement

Hello,

I have reviewed this patch before and provided a number of comments that
have been addressed by Akshay (so I encourage you to list my name and
this address in a Reviewed-by trailer line in the commit message). One
thing I had not noticed is that while this function has a "pretty" flag,
it doesn't use it to pass anything to pg_get_expr_worker()'s prettyFlags
argument, and I think it should -- probably just

prettyFlags = GET_PRETTY_FLAGS(pretty);

same as pg_get_querydef() does.

Thanks

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

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

On Wed, Oct 15, 2025 at 10:55 PM Álvaro Herrera <alvherre@kurilemu.de>
wrote:

Hello,

I have reviewed this patch before and provided a number of comments that
have been addressed by Akshay (so I encourage you to list my name and
this address in a Reviewed-by trailer line in the commit message). One
thing I had not noticed is that while this function has a "pretty" flag,
it doesn't use it to pass anything to pg_get_expr_worker()'s prettyFlags
argument, and I think it should -- probably just

prettyFlags = GET_PRETTY_FLAGS(pretty);

same as pg_get_querydef() does.

Fixed and added 'Reviewed-by:'

On Wed, Oct 15, 2025 at 11:00 PM Philip Alger <paalger0@gmail.com> wrote:

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,

Fixed all the above review comments in the v2 patch.

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.

We need to add extra parentheses for the USING and CHECK clauses. Without
them, expressions like USING true or CHECK true will throw a syntax error
at or near "true".

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.

I think the pretty format option is a nice-to-have parameter. Users can
simply set it to false if they don’t want the DDL to be formatted.
As for the statement terminator, it’s useful to include it, while running
multiple queries together could result in a syntax error. In my opinion,
there’s no harm in providing the statement terminator.
However, I’ve modified the logic to add the statement terminator at the end
instead of appending to a new line.

hi. I still can not compile your v2.

../../Desktop/pg_src/src1/postgres/src/backend/utils/adt/ruleutils.c:
In function ‘get_formatted_string’:
../../Desktop/pg_src/src1/postgres/src/backend/utils/adt/ruleutils.c:13770:9:
error: function ‘get_formatted_string’ might be a candidate for
‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
13770 | appendStringInfoVA(buf, fmt, args);
| ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Maybe you can register your patch on https://commitfest.postgresql.org/
then it will run all CI tests on all kinds of OS.

row security policy qual and with_check_qual can contain sublink/subquery.
but pg_get_expr can not cope with sublink/subquery.

see pg_get_expr comments below:
* Currently, the expression can only refer to a single relation, namely
* the one specified by the second parameter. This is sufficient for
* partial indexes, column default expressions, etc. We also support
* Var-free expressions, for which the OID can be InvalidOid.

see commit 6867f96 and
/messages/by-id/20211219205422.GT17618@telsasoft.com

I guess (because I can not compile, mentioned above):
"ERROR: expression contains variables"
can be triggered by the following setup:

create table t(a int);
CREATE POLICY p1 ON t AS RESTRICTIVE FOR ALL
USING (a IS NOT NULL AND (SELECT 1 = 1 FROM pg_rewrite WHERE
pg_get_function_arg_default(ev_class, 1) !~~ pg_get_expr(ev_qual, 0,
false)));
SELECT pg_get_policy_ddl('t', 'p1', true);

You can also check my patch at https://commitfest.postgresql.org/patch/6054/
which similarly needs to build the POLICY definition for reconstruction.

see RelationBuildRowSecurity, checkExprHasSubLink also.

On Thu, Oct 16, 2025 at 2:45 PM jian he <jian.universality@gmail.com> wrote:

hi. I still can not compile your v2.

../../Desktop/pg_src/src1/postgres/src/backend/utils/adt/ruleutils.c:
In function ‘get_formatted_string’:

../../Desktop/pg_src/src1/postgres/src/backend/utils/adt/ruleutils.c:13770:9:
error: function ‘get_formatted_string’ might be a candidate for
‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
13770 | appendStringInfoVA(buf, fmt, args);
| ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

I’m relatively new to PostgreSQL development. I’m working on setting up the
CI pipeline and will try to fix all warnings.

Maybe you can register your patch on https://commitfest.postgresql.org/
then it will run all CI tests on all kinds of OS.

row security policy qual and with_check_qual can contain sublink/subquery.
but pg_get_expr can not cope with sublink/subquery.

see pg_get_expr comments below:
* Currently, the expression can only refer to a single relation, namely
* the one specified by the second parameter. This is sufficient for
* partial indexes, column default expressions, etc. We also support
* Var-free expressions, for which the OID can be InvalidOid.

see commit 6867f96 and

/messages/by-id/20211219205422.GT17618@telsasoft.com

I guess (because I can not compile, mentioned above):
"ERROR: expression contains variables"
can be triggered by the following setup:

create table t(a int);
CREATE POLICY p1 ON t AS RESTRICTIVE FOR ALL
USING (a IS NOT NULL AND (SELECT 1 = 1 FROM pg_rewrite WHERE
pg_get_function_arg_default(ev_class, 1) !~~ pg_get_expr(ev_qual, 0,
false)));
SELECT pg_get_policy_ddl('t', 'p1', true);

The above example works fine with my patch
[image: Screenshot 2025-10-16 at 5.08.10 PM.png]

Hi Akshay,

As for the statement terminator, it’s useful to include it, while

running multiple queries together could result in a syntax error. In my
opinion, there’s no harm in providing the statement terminator.

However, I’ve modified the logic to add the statement terminator at the
end instead of appending to a new line.

Regarding putting the terminator at the end, I think my original comment
got cut off by my poor editing. Yes, that's what I was referring to; no
need to append it to a new line.

--
Best, Phil Alger

Please find attached the v3 patch, which resolves all compilation errors
and warnings.

On Thu, Oct 16, 2025 at 6:06 PM Philip Alger <paalger0@gmail.com> wrote:

On Oct 16, 2025, at 20:50, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Please find attached the v3 patch, which resolves all compilation errors and warnings.

On Thu, Oct 16, 2025 at 6:06 PM Philip Alger <paalger0@gmail.com> wrote:
Hi Akshay,

As for the statement terminator, it’s useful to include it, while running multiple queries together could result in a syntax error. In my opinion, there’s no harm in providing the statement terminator.
However, I’ve modified the logic to add the statement terminator at the end instead of appending to a new line.

Regarding putting the terminator at the end, I think my original comment got cut off by my poor editing. Yes, that's what I was referring to; no need to append it to a new line.

--
Best, Phil Alger
<v3-0001-Add-pg_get_policy_ddl-function-to-reconstruct-CREATE.patch>

1 - ruleutils.c
```
+	if (pretty)
+	{
+		/* Indent with tabs */
+		for (int i = 0; i < noOfTabChars; i++)
+		{
+			appendStringInfoString(buf, "\t");
+		}
```

As you are adding a single char of ‘\t’, better to use appendStringInfoChar() that is cheaper.

2 - ruleutils.c
```
+	initStringInfo(&buf);
+
+	targetTable = relation_open(tableID, AccessShareLock);
```

Looks like only usage of opening the table is to get the table name. In that case, why don’t just call get_rel_name(tableID) and store the table name in a local variable?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Thu, Oct 16, 2025 at 8:51 PM Akshay Joshi
<akshay.joshi@enterprisedb.com> wrote:

Please find attached the v3 patch, which resolves all compilation errors and warnings.

drop table if exists t, ts, ts1;
create table t(a int);
CREATE POLICY p0 ON t FOR ALL TO PUBLIC USING (a % 2 = 1);
SELECT pg_get_policy_ddl('t', 'p0', false);

pg_get_policy_ddl
---------------------------------------------------------------------
CREATE POLICY p0 ON t AS PERMISSIVE FOR ALL USING (((a % 2) = 1));
(1 row)

"TO PUBLIC" part is missing, maybe it's ok.

SELECT pg_get_policy_ddl(-1, 'p0', false);
ERROR: could not open relation with OID 4294967295
as I mentioned in a nearby thread [1]/messages/by-id/CACJufxGbE4uJWu1YuqdmOx+7PMBpHvX_fbRMmHu=r4SrsuW9tg@mail.gmail.com, this should be NULL instead of ERROR.
[1]: /messages/by-id/CACJufxGbE4uJWu1YuqdmOx+7PMBpHvX_fbRMmHu=r4SrsuW9tg@mail.gmail.com

IMHO, get_formatted_string is not needed, most of the time, if pretty is true,
we append "\t" and "\n", for that we can simply do
```
appendStringInfo(&buf, "CREATE POLICY %s ON %s ",
quote_identifier(NameStr(*policyName)),
generate_qualified_relation_name(policy_form->polrelid));
if (pretty)
appendStringInfoString(buf, "\t\n");
```

in pg_get_triggerdef_worker, I found the below code pattern:
/*
* In non-pretty mode, always schema-qualify the target table name for
* safety. In pretty mode, schema-qualify only if not visible.
*/
appendStringInfo(&buf, " ON %s ",
pretty ?
generate_relation_name(trigrec->tgrelid, NIL) :
generate_qualified_relation_name(trigrec->tgrelid));

maybe we can apply it too while construct query string:
"CREATE POLICY %s ON %s",

On Tue, Oct 21, 2025 at 2:39 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Oct 16, 2025, at 20:50, Akshay Joshi <akshay.joshi@enterprisedb.com>

wrote:

Please find attached the v3 patch, which resolves all compilation errors

and warnings.

On Thu, Oct 16, 2025 at 6:06 PM Philip Alger <paalger0@gmail.com> wrote:
Hi Akshay,

As for the statement terminator, it’s useful to include it, while

running multiple queries together could result in a syntax error. In my
opinion, there’s no harm in providing the statement terminator.

However, I’ve modified the logic to add the statement terminator at the

end instead of appending to a new line.

Regarding putting the terminator at the end, I think my original comment

got cut off by my poor editing. Yes, that's what I was referring to; no
need to append it to a new line.

--
Best, Phil Alger
<v3-0001-Add-pg_get_policy_ddl-function-to-reconstruct-CREATE.patch>

1 - ruleutils.c
```
+       if (pretty)
+       {
+               /* Indent with tabs */
+               for (int i = 0; i < noOfTabChars; i++)
+               {
+                       appendStringInfoString(buf, "\t");
+               }
```

As you are adding a single char of ‘\t’, better to use
appendStringInfoChar() that is cheaper.

2 - ruleutils.c
```
+       initStringInfo(&buf);
+
+       targetTable = relation_open(tableID, AccessShareLock);
```

Looks like only usage of opening the table is to get the table name. In
that case, why don’t just call get_rel_name(tableID) and store the table
name in a local variable?

Fixed the above review comments in the v4 patch. I have used
generate_qualified_relation_name(tableID) instead of get_rel_name(tableID).

On Wed, Oct 22, 2025 at 12:51 PM jian he <jian.universality@gmail.com>
wrote:

On Thu, Oct 16, 2025 at 8:51 PM Akshay Joshi
<akshay.joshi@enterprisedb.com> wrote:

Please find attached the v3 patch, which resolves all compilation errors

and warnings.

drop table if exists t, ts, ts1;
create table t(a int);
CREATE POLICY p0 ON t FOR ALL TO PUBLIC USING (a % 2 = 1);
SELECT pg_get_policy_ddl('t', 'p0', false);

pg_get_policy_ddl
---------------------------------------------------------------------
CREATE POLICY p0 ON t AS PERMISSIVE FOR ALL USING (((a % 2) = 1));
(1 row)

"TO PUBLIC" part is missing, maybe it's ok.

I used the logic below, which did not return PUBLIC as a role. I have added
logic to default the TO clause to PUBLIC when no specific role name is
provided
valueDatum = heap_getattr(tuplePolicy,
Anum_pg_policy_polroles,
RelationGetDescr(pgPolicyRel),
&attrIsNull);
if (!attrIsNull)
{
ArrayType *policy_roles = DatumGetArrayTypePCopy(valueDatum);
int nitems = ARR_DIMS(policy_roles)[0];
Oid *roles = (Oid *) ARR_DATA_PTR(policy_roles);

SELECT pg_get_policy_ddl(-1, 'p0', false);
ERROR: could not open relation with OID 4294967295
as I mentioned in a nearby thread [1], this should be NULL instead of
ERROR.
[1]
/messages/by-id/CACJufxGbE4uJWu1YuqdmOx+7PMBpHvX_fbRMmHu=r4SrsuW9tg@mail.gmail.com

Fixed in v4 patch.

IMHO, get_formatted_string is not needed, most of the time, if pretty is
true,
we append "\t" and "\n", for that we can simply do
```
appendStringInfo(&buf, "CREATE POLICY %s ON %s ",
quote_identifier(NameStr(*policyName)),
generate_qualified_relation_name(policy_form->polrelid));
if (pretty)
appendStringInfoString(buf, "\t\n");
```

The get_formatted_string function is needed. Instead of using multiple if
statements for the pretty flag, it’s better to have a generic function.
This will be useful if the pretty-format DDL implementation is accepted by
the community, as it can be reused by other pg_get_<object>_ddl() DDL
functions added in the future.

in pg_get_triggerdef_worker, I found the below code pattern:
/*
* In non-pretty mode, always schema-qualify the target table name for
* safety. In pretty mode, schema-qualify only if not visible.
*/
appendStringInfo(&buf, " ON %s ",
pretty ?
generate_relation_name(trigrec->tgrelid, NIL) :
generate_qualified_relation_name(trigrec->tgrelid));

maybe we can apply it too while construct query string:
"CREATE POLICY %s ON %s",

In my opinion, the table name should always be schema-qualified, which I
have addressed in the v4 patch. The implementation of the pretty flag here
differs from pg_get_triggerdef_worker, which is used only for the target
table name. In my patch, the pretty flag adds \t and \n to each different
clause (example AS, FOR, USING ...)

The get_formatted_string function is needed. Instead of using multiple if

statements for the pretty flag, it’s better to have a generic function.
This will be useful if the pretty-format DDL implementation is accepted by
the community, as it can be reused by other pg_get_<object>_ddl() DDL
functions added in the future.

in pg_get_triggerdef_worker, I found the below code pattern:
/*
* In non-pretty mode, always schema-qualify the target table name for
* safety. In pretty mode, schema-qualify only if not visible.
*/
appendStringInfo(&buf, " ON %s ",
pretty ?
generate_relation_name(trigrec->tgrelid, NIL) :
generate_qualified_relation_name(trigrec->tgrelid));

maybe we can apply it too while construct query string:
"CREATE POLICY %s ON %s",

In my opinion, the table name should always be schema-qualified, which I
have addressed in the v4 patch. The implementation of the pretty flag
here differs from pg_get_triggerdef_worker, which is used only for the
target table name. In my patch, the pretty flag adds \t and \n to each
different clause (example AS, FOR, USING ...)

I think that's where the confusion lies with adding `pretty` to the DDL
functions. The `pretty` flag set to `true` on other functions is used to
"not" show the schema name. But in the case of this function, it is used to
format the statement. I wonder if the word `format` or `pretty_format` is a
better name? `pretty` itself in the codebase isn't a very clear name
regardless.

--
Best,
Phil Alger

On Thu, Oct 23, 2025 at 12:19 AM Philip Alger <paalger0@gmail.com> wrote:

The get_formatted_string function is needed. Instead of using multiple

if statements for the pretty flag, it’s better to have a generic
function. This will be useful if the pretty-format DDL implementation is
accepted by the community, as it can be reused by other
pg_get_<object>_ddl() DDL functions added in the future.

in pg_get_triggerdef_worker, I found the below code pattern:
/*
* In non-pretty mode, always schema-qualify the target table name
for
* safety. In pretty mode, schema-qualify only if not visible.
*/
appendStringInfo(&buf, " ON %s ",
pretty ?
generate_relation_name(trigrec->tgrelid, NIL) :
generate_qualified_relation_name(trigrec->tgrelid));

maybe we can apply it too while construct query string:
"CREATE POLICY %s ON %s",

In my opinion, the table name should always be schema-qualified, which I
have addressed in the v4 patch. The implementation of the pretty flag
here differs from pg_get_triggerdef_worker, which is used only for the
target table name. In my patch, the pretty flag adds \t and \n to each
different clause (example AS, FOR, USING ...)

I think that's where the confusion lies with adding `pretty` to the DDL
functions. The `pretty` flag set to `true` on other functions is used to
"not" show the schema name. But in the case of this function, it is used to
format the statement. I wonder if the word `format` or `pretty_format` is a
better name? `pretty` itself in the codebase isn't a very clear name
regardless.

In my opinion, we should first decide whether we want the DDL statement to
be in a 'pretty' format or not. Personally, I believe it should be, since
parsing a one-line DDL statement can be quite complex and difficult for
users of this function. From a user’s perspective, having the entire DDL in
a single line makes it harder to read and understand. The flag allows users
to disable the pretty format by passing false.
If the community agrees on this approach, we can then think about choosing
a more appropriate word for it.

Hi everyone,

On Thu, Oct 16, 2025 at 01:47:53PM +0530, Akshay Joshi wrote:

On Wed, Oct 15, 2025 at 10:55 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

Hello,

I have reviewed this patch before and provided a number of comments that
have been addressed by Akshay (so I encourage you to list my name and
this address in a Reviewed-by trailer line in the commit message).  One
thing I had not noticed is that while this function has a "pretty" flag,
it doesn't use it to pass anything to pg_get_expr_worker()'s prettyFlags
argument, and I think it should -- probably just

  prettyFlags = GET_PRETTY_FLAGS(pretty);

same as pg_get_querydef() does.

Kinda sorta similar thought, I've noticed some existing functions like
pg_get_constraintdef make the "pretty" flag optional, so I'm wondering
if that scheme is also preferred here.

I've attached a small diff to the original
0001-Add-pg_get_policy_ddl-function-to-reconstruct-CREATE.patch to
illustrate the additional work to follow suit, if so desired.

Regards,
Mark
--
Mark Wong <markwkm@gmail.com>
EDB https://enterprisedb.com

Thanks, Mark, for your review comments and the updated patch.

I’ve incorporated your changes and prepared a combined v5 patch. The v5
patch is attached for further review.

On Mon, Oct 27, 2025 at 10:15 PM Mark Wong <markwkm@gmail.com> wrote:

Hi Hackers,

Added a new #define GET_DDL_PRETTY_FLAGS because the existing #define
GET_PRETTY_FLAGS is not suitable for formatting reconstructed DDLs. The
existing #define GET_PRETTY_FLAGS always indents the code, regardless of
whether the flag is set to true or false, which is not the desired behavior
for pg_get_<object>_ddl functions.

Updated the logic of the get_formatted_string function based on Tim
Waizenegger’s suggestion.

I am attaching the new v6 patch, which is ready for review.

On Tue, Oct 28, 2025 at 3:08 PM Akshay Joshi <akshay.joshi@enterprisedb.com>
wrote:

Hi Hackers,

Make the pretty flag a default parameter by adding CREATE OR REPLACE
FUNCTION in system_functions.sql.
Attached is the v7 patch, which is ready for review.

On Mon, Nov 3, 2025 at 5:17 PM Akshay Joshi <akshay.joshi@enterprisedb.com>
wrote:

Em sex., 7 de nov. de 2025 às 09:21, Akshay Joshi <
akshay.joshi@enterprisedb.com> escreveu:

Attached is the v7 patch, which is ready for review.

<https://enterprisedb.com&gt;

For this functionality to be complete, the

ALTER TABLE ... ENABLE ROW LEVEL SECURITY
statement needs to be added to the command.

Did you think about that ?
Maybe one more parameter ?

regards
Marcos