Horton

Re: Logical Replication of sequences

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: vignesh C <vignesh21@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-14T00:34:01Z
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

On Tue, Aug 13, 2024 at 10:00 PM vignesh C <vignesh21@gmail.com> wrote:
>
> 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.
>

Yes, I wrote "comment header" but it was a typo; I meant "commit
header". What you did looks good now. Thanks.

> > ~
> >
> > 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.
>

According to my understanding, the logical replication code tries to
maintain name conventions for static functions (snake_case) and for
non-static functions (CamelCase) as an aid for code readability. I
think we should either do our best to abide by those conventions, or
we might as well just forget them and have a naming free-for-all.
Since the new syncutils.c module is being introduced by this patch, my
guess is that any future merging to back-branches will be affected
regardless. IMO this is an ideal opportunity to try to nudge the
function names in the right direction. YMMV.

======
Kind Regards,
Peter Smith.
Fujitsu Australia