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: Masahiko Sawada <sawada.mshk@gmail.com>, shveta malik <shveta.malik@gmail.com>, Shlok Kyal <shlok.kyal.oss@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, 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-12-23T07:20:14Z
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 Wed, 18 Dec 2024 at 11:40, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh,
>
> Here are some review comments for the patch v20241211-0002.
> ~~~
>
> publish option effect fro SEQUENCE replication?
>
> 2.
> It's not obvious to me if the SEQUENCE replication stuff is affected
> but the setting of pubactions (ie the 'publish' option). I'm thinking
> that probably anything to do with SEQUENCEs may no be considered a DML
> operation, but if that is true it might be better to explicitly say
> so.
>
> Also, we might need to include a test to show even if publish='' that
> the SEQUENCE synchronization is not affected.

Since we just synchronize the data and we don't replicate incremental
changes and insert/update/delete are only for incremental data sync, I
felt we need not mention this explicitly in this case as we have
mentioned that "Incremental sequence changes are not replicated" in
0005 patch. I also felt that there was no need to add a testcase for
this as currently it has nothing to do with this option.

>
> ======
> src/backend/commands/publicationcmds.c
>
> CreatePublication:
>
> 3.
> - /* FOR ALL TABLES requires superuser */
> - if (stmt->for_all_tables && !superuser())
> + /* FOR ALL TABLES or FOR ALL SEQUENCES requires superuser */
> + if ((stmt->for_all_tables || stmt->for_all_sequences) && !superuser())
>   ereport(ERROR,
>   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> - errmsg("must be superuser to create FOR ALL TABLES publication")));
> + errmsg("must be superuser to create a %s publication",
> + stmt->for_all_tables ? "FOR ALL TABLES" :
> + "FOR ALL SEQUENCES")));
>
> It seems a quirk that if FOR ALL TABLES and FOR ALL SEQUENCES are
> specified at the same time then you would only get the "FOR ALL
> TABLES" error, but maybe that is OK?

I have modified the error message slightly

> ~~~
>
> AlterPublicationOwner_internal:
>
> 4.
> Ditto quirk as commented above, but maybe it is OK.

I have modified the error message slightly

The rest of the comments are fixed. Also the comments from [1] are fixed.
The attached v202412123 version patch has the changes for the same.

[1] - https://www.postgresql.org/message-id/CAHut%2BPsk1V2eUL_nreWp1trO1iSDqhDtBnfu65PrsoorpuNzKA%40mail.gmail.com

Regards,
Vignesh