Re: Skipping schema changes in publication

On Wed, Dec 3, 2025 at 8:56 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Wed, Nov 19, 2025 at 3:34 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> 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+94uNA@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).
>

6)
Just before AlterPublicationReset-->LockSchemaList does
SearchSysCacheExists1(), I dropped the schema concurrently, which
resulted in below error:

postgres=# alter publication pub1 reset;
ERROR: schema with OID 16393 does not exist

I do not think the user should be seeing this error. For
CreatePublication and AlterPublicationSchemas, it makes sense to give
an error when it calls LockSchemaList as user has given the schema
names himself. But here since schemas are internally fetched, I think
it will be logical to skip the
error if it gets dropped concurrently. Thoughts?

If we plan to skip error, we can do so by passing the flag
missing_okay=true. See RangeVarGetRelid and other such functions using
such a flag.

thanks
Shveta