On Tue, Oct 21, 2025 at 2:39 PM Chao Li <[email protected]> wrote:
> > > > On Oct 16, 2025, at 20:50, Akshay Joshi <[email protected]> > 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 <[email protected]> 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). > > Best regards, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > > > > >
