Обсуждение: [PATCH] Add pg_get_policy_ddl() function to reconstruct CREATE POLICY statement

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)
 ;

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

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
https://www.postgresql.org/message-id/flat/20211219205422.GT17618%40telsasoft.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

 

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.

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. 

> 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], this should be NULL instead of ERROR.
[1] https://postgr.es/m/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",

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.

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.

--

Best, 

Phil Alger