Re: Logical Replication of sequences

On Thu, 4 Jul 2024 at 12:44, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my review comments for the patch v20240703-0002
>
> ======
> doc/src/sgml/ref/create_publication.sgml
>
>
> Question: Was there a reason you chose wording "synchronizes changes"
> instead of having same "replicates changes" wording of FOR ALL TABLES?

Since at this point we are only supporting sync of sequences, there
are no incremental changes being replicated to subscribers. I thought
synchronization is better suited here.

> ======
> src/backend/catalog/system_views.sql
>
> 1.
> Should there be some new test for the view? Otherwise, AFAICT this
> patch has no tests that will exercise the new function
> pg_get_publication_sequences.

pg_publication_sequences view uses pg_get_publication_sequences which
will be tested with 3rd patch while creating subscription/refreshing
publication sequences. I felt it is ok not to have a test here.

> 5.
> - bool for_all_tables; /* Special publication for all tables in db */
> + List    *for_all_objects; /* Special publication for all objects in
> + * db */
>
> Is this OK? Saying "for all objects" seemed misleading.

This change is not required, reverting it.

> 6.
> I asked this before in a previous review [1-#17] -- I didn't
> understand the point of the sequence 'testpub_seq0' since nobody seems
> to be doing anything with it. Should it just be removed? Or is there a
> missing test case to use it?

Since we are having all sequences published I wanted to have a
sequence in another schema also. Adding describe for it too.

> ~~~
>
> 7.
> Other things to consider:
>
> (I didn't include these in my attached diff)
>
> * could use a single CREATE SEQUENCE stmt instead of multiple

CREATE SEQUENCE does not support specifying multiple sequences in one
statement, skipping this.

The rest of the comments are fixed, the attached v20240705 version
patch has the changes for the same.

Regards,
Vignesh