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