Re: Logical Replication of sequences

On Tue, Oct 7, 2025 at 4:56 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 7 Oct 2025 at 10:53, shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Mon, Oct 6, 2025 at 4:33 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > >
> > > Here is the rebased remaining patches.
> > >
> >
> > Thank You for the patches, please find a few comment on 001:
> >
> > 1)
> > Shall we have 'pg_publication_sequences' created in the first patch
> > itself to help verify which all sequences are added to ALL SEQ
> > publications? Currently it is in 4th patch.
>
> Moved it to 0001 patch
>
> > 2)
> > postgres=# create publication pub1 for all sequences WITH(publish='insert');
> > ERROR:  publication parameters are not supported for publications
> > defined as FOR ALL SEQUENCES
> >
> > postgres=# alter publication pub1 add table tab1;
> > ERROR:  Tables or sequences cannot be added to or dropped from
> > publication defined FOR ALL TABLES, ALL SEQUENCES, or both
> >
> > a) First msg has 'as', while second does not. Shall we make both the
> > same? I think we can get rid of 'as'.
>
> The second error message is changed now based on another comment, so
> both of them will have as now.
>
> > b) Shouldn't the error msg start with lower case (second one)?
>
> Updated
>
> > 3)
> > + * Process all_objects_list to set all_tables/all_sequences.
> >
> > can we please replace 'all_tables/all_sequences' with 'all_tables
> > and/or all_sequences'
>
> Updated
>
> > 4)
> > +/*
> > + * Publication types supported by FOR ALL ...
> > + */
> > +typedef enum PublicationAllObjType
> >
> > Should it be:
> > 'Types of objects supported by FOR ALL publications'
>
> Modified
>
> > 5)
> > +-- Specifying both ALL TABLES and ALL SEQUENCES along with WITH
> > clause should throw a warning
> > +SET client_min_messages = 'NOTICE';
> > +CREATE PUBLICATION regress_pub_for_allsequences_alltables_withcaluse
> > FOR ALL SEQUENCES, ALL TABLES WITH (publish = 'insert');
> > +NOTICE:  publication parameters are not applicable to sequence
> > synchronization and will be ignored
> >
> > Comment can be changed to say it will emit/raise a NOTICE (instead of warning).
>
> Modified
>
> > 6)
> > commit msg:
> > --
> > Note: This patch currently supports only the "ALL SEQUENCES" clause.
> > Handling of clauses such as "FOR SEQUENCE" and "FOR SEQUENCES IN SCHEMA"
> > will be addressed in a subsequent patch.
> > --
> >
> > This seems misleading, as we are not planning the "FOR SEQUENCE" in
> > the current set of patches, maybe we can rephrase it a bit.
>
> Modified
>
> Thanks for the comments, the v20251007 patch attached at [1] has the
> changes for the same.
>

Thanks for making the changes.  Just one trivial comment:

We can have 'applicable to' instead of 'applicable for' in all these places:

a)
+ /* Publication actions are not applicable for sequence-only publications */

b)
+      publishing tables. This clause is not applicable for sequences. The

c)
+ errmsg("publication parameters are not applicable for publications
defined as FOR ALL SEQUENCES"));


thanks
Shveta