On Thu, 26 Jun 2025 at 15:27, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Jun 24, 2025 at 9:48 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > I have included the changes for
> > it in v14-0003 patch.
> >
> Thanks for the patches. I have reviewed patch001 alone, please find
> few comments:
>
> 1)
> + <para>
> + The <literal>RESET</literal> clause will reset the publication to the
> + default state which includes resetting the publication parameters, setting
> + <literal>ALL TABLES</literal> flag to <literal>false</literal> and
> + dropping all relations and schemas that are associated with the
> + publication.
> </para>
>
> It is misleading, as far as I have understood, we do not drop the
> tables or schemas associated with the pub; we just remove those from
> the publication's object list. See previous doc:
> "The ADD and DROP clauses will add and remove one or more
> tables/schemas from the publication"
>
> Perhaps we want to say the same thing when we speak about the 'drop'
> aspect of RESET.
I have updated the document.
> 2)
> AlterPublicationReset():
>
> + if (!OidIsValid(prid))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_OBJECT),
> + errmsg("relation \"%s\" is not part of the publication",
> + get_rel_name(relid))));
>
> Can you please help me understand which scenario will give this error?
>
> Another question is do we really need this error? IIUC, we generally
> give errors if a user has explicitly called out a name of an object
> and that object is not found. Example:
>
> postgres=# alter publication pubnew drop table t1,tab2;
> ERROR: relation "t1" is not part of the publication
>
> While in a few other cases, we pass missing_okay as true and do not
> give errors. Please see other callers of performDeletion in
> publicationcmds.c itself. There we have usage of missing_okay=true. I
> have not researched myself, but please analyze the cases where
> missing_okay is passed as true to figure out if those match our RESET
> case. Try to reproduce if possible and then take a call.
I thought about the above point and I also think this check is not
required. Also, the function was calling PublicationDropSchemas with
missing_ok as false. I have changed it to be true.
> 3)
> +ALTER PUBLICATION testpub_reset ADD ALL TABLES IN SCHEMA public;
> +ERROR: syntax error at or near "ALL"
> +LINE 1: ALTER PUBLICATION testpub_reset ADD ALL TABLES IN SCHEMA pub...
>
> There is a problem in syntax, I think the intention of testcase was to
> run this query successfully.
I have fixed it.
Thanks Shveta for reviewing the patch. I have addressed the comments
and posted an updated version v15 in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEU%2BaPu6iAH2cTA0cDtn3pd6c_njBONCt3FubYZoEEnm8Q%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
