Horton

Re: Logical Replication of sequences

vignesh C <vignesh21@gmail.com>

View thread
From: vignesh C <vignesh21@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Shlok Kyal <shlok.kyal.oss@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Tomas Vondra <tomas.vondra@enterprisedb.com>, Euler Taveira <euler@eulerto.com>, Michael Paquier <michael@paquier.xyz>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com>, "Jonathan S. Katz" <jkatz@postgresql.org>
Date: 2024-07-05T11:58:26Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Doc: Add documentation for sequence synchronization.

  2. Remove unused assignment in CREATE PUBLICATION grammar.

  3. Add seq_sync_error_count to subscription statistics.

  4. Fix few issues in commit 5509055d69.

  5. Add sequence synchronization for logical replication.

  6. Add worker type argument to logical replication worker functions.

  7. Introduce "REFRESH SEQUENCES" for subscriptions.

  8. Refactor logical worker synchronization code into a separate file.

  9. Standardize use of REFRESH PUBLICATION in code and messages.

  10. Add "ALL SEQUENCES" support to publications.

  11. Expose sequence page LSN via pg_get_sequence_data.

  12. Resume conflict-relevant data retention automatically.

  13. Post-commit review fixes for 228c370868.

  14. Generate GUC tables from .dat file

Attachments

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