On Wed, Nov 19, 2025 at 3:34 PM Shlok Kyal <[email protected]> wrote:
>
>
> I have also addressed the comments in [1], [2].
>
> [1]:
> https://www.postgresql.org/message-id/CAHut%2BPtRzCD4-0894cutkU_h8cPNtosN0_oSHn2iAKEfg2ENOQ%40mail.gmail.com
> [2]:
> https://www.postgresql.org/message-id/CAHut+PuHn-hohA4OdEJz+Zfukfr41TvMTeTH7NwJ=wg1+94...@mail.gmail.com
>
Thanks for the patch. Please find a few comments on 001:
1)
+ bool nulls[Natts_pg_publication];
+ bool replaces[Natts_pg_publication];
+ Datum values[Natts_pg_publication];
+ memset(values, 0, sizeof(values));
+ memset(nulls, false, sizeof(nulls));
+ memset(replaces, false, sizeof(replaces));
Can we initialize all 3 like below? Then we do not need memset.
bool nulls[Natts_pg_publication] = {0};
2)
AlterPublicationReset():
Can we reset the columns in same sequence as they are originally
defined (see pg_publication_d.h), it makes it easy to map when
comparing/reviewing that we do not have missed any.
3)
+/* default values for flags and publication parameters */
...
+#define PUB_DEFAULT_VIA_ROOT false
+#define PUB_DEFAULT_ALL_TABLES false
+#define PUB_DEFAULT_ALL_SEQUENCES false
+#define PUB_DEFAULT_GENCOLS PUBLISH_GENCOLS_NONE
These too we can rearrange as per order in pg_publication_d.h
4)
+ COMPLETE_WITH("ADD", "DROP", "OWNER TO", "RENAME TO", "RESET", "SET");
Is it supposed to be in alphabetical order? Looking at others, I do
not think so. If not, then I feel 'SET' first followed by 'RESET'
seems a more obvious choice to me. See similar (ENABLE followed by
DISABLE):
COMPLETE_WITH("CONNECTION", "ENABLE", "DISABLE", "OWNER TO",
5)
+DROP TABLE pub_sch1.tbl1;
+DROP SCHEMA pub_sch1;
We can use 'DROP SCHEMA pub_sch1 CASCADE'. Then we need not to worry
about dropping the associated table(s) (even if we create more in
future in this schema).
thanks
Shveta
