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: shveta malik <shveta.malik@gmail.com>, 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-08-13T11:59:49Z
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 Tue, 13 Aug 2024 at 09:19, Peter Smith <smithpb2250@gmail.com> wrote:
>
> 3.1. GENERAL
>
> Hmm. I am guessing this was provided as a separate patch to aid review
> by showing that existing functions are moved? OTOH you can't really
> judge this patch properly without already knowing details of what will
> come next in the sequencesync. i.e. As a *standalone* patch without
> the sequencesync.c the refactoring doesn't make much sense.
>
> Maybe it is OK later to combine patches 0003 and 0004. Alternatively,
> keep this patch separated but give greater emphasis in the comment
> header to say this patch only exists separately in order to help the
> review.

I have kept this patch only to show that this patch as such has no
code changes. If we move this to the next patch it will be difficult
for reviewers to know which is new code and which is old code. During
commit we can merge this with the next one. I felt it is better to add
it in the commit message instead of comment header so updated the
commit message.

> ======
> src/backend/replication/logical/syncutils.c
>
> 3.3. "common code" ??
>
> FYI - There are multiple code comments mentioning "common code..."
> which, in the absence of the sequencesync worker (which comes in the
> next patch), have nothing "common" about them at all. Fixing them and
> then fixing them again in the next patch might cause unnecessary code
> churn, but OTOH they aren't correct as-is either. I have left them
> alone for now.

We can ignore this as this will get merged to the next one. If you
have any comments you can give it on top of the next(0004) patch.

> ~
>
> 3.4. function names
>
> With the re-shuffling that this patch does, and changing several from
> static to not-static, should the function names remain as they are?
> They look random to me.
> - finish_sync_worker(void)
> - invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
> - FetchTableStates(bool *started_tx)
> - process_syncing_tables(XLogRecPtr current_lsn)
>
> I think using a consistent naming convention would be better. e.g.
> SyncFinishWorker
> SyncInvalidateTableStates
> SyncFetchTableStates
> SyncProcessTables

One advantage with keeping the existing names the same wherever
possible will help while merging the changes to back-branches. So I'm
not making this change.

> ~~~
>
> nit - file header comment
>
> ======
> src/backend/replication/logical/tablesync.c
>
> 3.5.
> -static void
> +void
>  process_syncing_tables_for_sync(XLogRecPtr current_lsn)
>
> -static void
> +void
>  process_syncing_tables_for_apply(XLogRecPtr current_lsn)
>
> Since these functions are no longer static should those function names
> be changed to use the CamelCase convention for non-static API?

One advantage with keeping the existing names the same wherever
possible will help while merging the changes to back-branches. So I'm
not making this change.

The rest of the comments were fixed, the attached v20240813 has the
changes for the same.

Regards,
Vignesh