Logical Replication of sequences
In the past, we have discussed various approaches to replicate sequences by decoding the sequence changes from WAL. However, we faced several challenges to achieve the same, some of which are due to the non-transactional nature of sequences. The major ones were: (a) correctness of the decoding part, some of the problems were discussed at [1][2][3] (b) handling of sequences especially adding certain sequences automatically (e.g. sequences backing SERIAL/BIGSERIAL columns) for built-in logical replication is not considered in the proposed work [1] (c) there were some performance concerns in not so frequent scenarios [4] (see performance issues), we can probably deal with this by making sequences optional for builtin logical replication It could be possible that we can deal with these and any other issues with more work but as the use case for this feature is primarily major version upgrades it is not clear that we want to make such a big change to the code or are there better alternatives to achieve the same. This time at pgconf.dev (https://2024.pgconf.dev/), we discussed alternative approaches for this work which I would like to summarize. The various methods we discussed are as follows: 1. Provide a tool to copy all the sequences from publisher to subscriber. The major drawback is that users need to perform this as an additional step during the upgrade which would be inconvenient and probably not as useful as some built-in mechanism. 2. Provide a command say Alter Subscription ... Replicate Sequences (or something like that) which users can perform before shutdown of the publisher node during upgrade. This will allow copying all the sequences from the publisher node to the subscriber node directly. Similar to previous approach, this could also be inconvenient for users. 3. Replicate published sequences via walsender at the time of shutdown or incrementally while decoding checkpoint record. The two ways to achieve this are: (a) WAL log a special NOOP record just before shutting down checkpointer. Then allow the WALsender to read the sequence data and send it to the subscriber while decoding the new NOOP record. (b) Similar to the previous idea but instead of WAL logging a new record directly invokes a decoding callback after walsender receives a request to shutdown which will allow pgoutput to read and send required sequences. This approach has a drawback that we are adding more work at the time of shutdown but note that we already waits for all the WAL records to be decoded and sent before shutting down the walsender during shutdown of the node. Any other ideas? I have added the members I remember that were part of the discussion in the email. Please feel free to correct me if I have misunderstood or missed any point we talked about. Thoughts? [1] - https://www.postgresql.org/message-id/e4145f77-6f37-40e0-a770-aba359c50b93%40enterprisedb.com [2] - https://www.postgresql.org/message-id/CAA4eK1Lxt%2B5a9fA-B7FRzfd1vns%3DEwZTF5z9_xO9Ms4wsqD88Q%40mail.gmail.com [3] - https://www.postgresql.org/message-id/CAA4eK1KR4%3DyALKP0pOdVkqUwoUqD_v7oU3HzY-w0R_EBvgHL2w%40mail.gmail.com [4] - https://www.postgresql.org/message-id/12822961-b7de-9d59-dd27-2e3dc3980c7e%40enterprisedb.com -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Fri, Jun 7, 2024 at 7:30 PM Amit Kapila wrote: > > On Fri, Jun 7, 2024 at 7:55 AM Masahiko Sawada wrote: > > > > On Thu, Jun 6, 2024 at 6:40 PM Amit Kapila wrote: > > > > > > On Thu, Jun 6, 2024 at 11:10 AM Masahiko Sawada > > > wrote: > > > > > > > > On Wed, Jun 5, 2024 at 9:30 PM Amit Kapila > > > > wrote: > > > > > > > > > > > > > > To achieve this, we can allow sequences to be copied during > > > > > the initial CREATE SUBSCRIPTION command similar to what we do for > > > > > tables. And then later by new/existing command, we re-copy the already > > > > > existing sequences on the subscriber. > > > > > > > > > > The options for the new command could be: > > > > > Alter Subscription ... Refresh Sequences > > > > > Alter Subscription ... Replicate Sequences > > > > > > > > > > In the second option, we need to introduce a new keyword Replicate. > > > > > Can you think of any better option? > > > > > > > > Another idea is doing that using options. For example, > > > > > > > > For initial sequences synchronization: > > > > > > > > CREATE SUBSCRIPTION ... WITH (copy_sequence = true); > > > > > > > > > > How will it interact with the existing copy_data option? So copy_data > > > will become equivalent to copy_table_data, right? > > > > Right. > > > > > > > > > For re-copy (or update) sequences: > > > > > > > > ALTER SUBSCRIPTION ... REFRESH PUBLICATION WITH (copy_sequence = true); > > > > > > > > > > Similar to the previous point it can be slightly confusing w.r.t > > > copy_data. And would copy_sequence here mean that it would copy > > > sequence values of both pre-existing and newly added sequences, if so, > > > that would make it behave differently than copy_data? The other > > > possibility in this direction would be to introduce an option like > > > replicate_all_sequences/copy_all_sequences which indicates a copy of > > > both pre-existing and new sequences, if any. > > > > Copying sequence data works differently than replicating table data > > (initial data copy and logical replication). So I thought the > > copy_sequence option (or whatever better name) always does both > > updating pre-existing sequences and adding new sequences. REFRESH > > PUBLICATION updates the tables to be subscribed, so we also update or > > add sequences associated to these tables. > > > > Are you imagining the behavior for sequences associated with tables > differently than the ones defined by the CREATE SEQUENCE .. command? I > was thinking that users would associate sequences with publications > similar to what we do for tables for both cases. For example, they > need to explicitly mention the sequences they want to replicate by > commands like CREATE PUBLICATION ... FOR SEQUENCE s1, s2, ...; CREATE > PUBLICATION ... FOR ALL SEQUENCES, or CREATE PUBLICATION ... FOR > SEQUENCES IN SCHEMA sch1; > > In this, variants FOR ALL SEQUENCES and SEQUENCES IN SCHEMA sch1 > should copy both the explicitly defined sequences and sequences > defined with the tables. Do you think a different variant for just > copying sequences implicitly associated with tables (say for identity > columns)? Oh, I was thinking that your proposal was to copy literally all sequences by REPLICA/REFRESH SEQUENCE command. But it seems to make sense to explicitly specify the sequences they want to replicate. It also means that they can create a publication that has only sequences. In this case, even if they create a subscription for that publication, we don't launch any apply workers for that subscription. Right? Also, given that the main use case (at least as the first step) is version upgrade, do we really need to support SEQUENCES IN SCHEMA and even FOR SEQUENCE? The WIP patch Vignesh recently submitted is more than 6k lines. I think we can cut the scope for the first implementation so as to make the review easy. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Logical Replication of sequences
On Mon, Jun 10, 2024 at 3:14 PM Masahiko Sawada wrote: > > On Fri, Jun 7, 2024 at 7:30 PM Amit Kapila wrote: > > > > On Fri, Jun 7, 2024 at 7:55 AM Masahiko Sawada > > wrote: > > > > > > On Thu, Jun 6, 2024 at 6:40 PM Amit Kapila > > > wrote: > > > > > > > > On Thu, Jun 6, 2024 at 11:10 AM Masahiko Sawada > > > > wrote: > > > > > > > > > > On Wed, Jun 5, 2024 at 9:30 PM Amit Kapila > > > > > wrote: > > > > > > > > > > > > > > > > > To achieve this, we can allow sequences to be copied during > > > > > > the initial CREATE SUBSCRIPTION command similar to what we do for > > > > > > tables. And then later by new/existing command, we re-copy the > > > > > > already > > > > > > existing sequences on the subscriber. > > > > > > > > > > > > The options for the new command could be: > > > > > > Alter Subscription ... Refresh Sequences > > > > > > Alter Subscription ... Replicate Sequences > > > > > > > > > > > > In the second option, we need to introduce a new keyword Replicate. > > > > > > Can you think of any better option? > > > > > > > > > > Another idea is doing that using options. For example, > > > > > > > > > > For initial sequences synchronization: > > > > > > > > > > CREATE SUBSCRIPTION ... WITH (copy_sequence = true); > > > > > > > > > > > > > How will it interact with the existing copy_data option? So copy_data > > > > will become equivalent to copy_table_data, right? > > > > > > Right. > > > > > > > > > > > > For re-copy (or update) sequences: > > > > > > > > > > ALTER SUBSCRIPTION ... REFRESH PUBLICATION WITH (copy_sequence = > > > > > true); > > > > > > > > > > > > > Similar to the previous point it can be slightly confusing w.r.t > > > > copy_data. And would copy_sequence here mean that it would copy > > > > sequence values of both pre-existing and newly added sequences, if so, > > > > that would make it behave differently than copy_data? The other > > > > possibility in this direction would be to introduce an option like > > > > replicate_all_sequences/copy_all_sequences which indicates a copy of > > > > both pre-existing and new sequences, if any. > > > > > > Copying sequence data works differently than replicating table data > > > (initial data copy and logical replication). So I thought the > > > copy_sequence option (or whatever better name) always does both > > > updating pre-existing sequences and adding new sequences. REFRESH > > > PUBLICATION updates the tables to be subscribed, so we also update or > > > add sequences associated to these tables. > > > > > > > Are you imagining the behavior for sequences associated with tables > > differently than the ones defined by the CREATE SEQUENCE .. command? I > > was thinking that users would associate sequences with publications > > similar to what we do for tables for both cases. For example, they > > need to explicitly mention the sequences they want to replicate by > > commands like CREATE PUBLICATION ... FOR SEQUENCE s1, s2, ...; CREATE > > PUBLICATION ... FOR ALL SEQUENCES, or CREATE PUBLICATION ... FOR > > SEQUENCES IN SCHEMA sch1; > > > > In this, variants FOR ALL SEQUENCES and SEQUENCES IN SCHEMA sch1 > > should copy both the explicitly defined sequences and sequences > > defined with the tables. Do you think a different variant for just > > copying sequences implicitly associated with tables (say for identity > > columns)? > > Oh, I was thinking that your proposal was to copy literally all > sequences by REPLICA/REFRESH SEQUENCE command. But it seems to make > sense to explicitly specify the sequences they want to replicate. It > also means that they can create a publication that has only sequences. > In this case, even if they create a subscription for that publication, > we don't launch any apply workers for that subscription. Right? > > Also, given that the main use case (at least as the first step) is > version upgrade, do we really need to support SEQUENCES IN SCHEMA and > even FOR SEQUENCE? Also, I guess that specifying individual sequences might not be easy to use for users in some cases. For sequences owned by a column of a table, users might want to specify them altogether, rather than separately. For example, CREATE PUBLICATION ... FOR TABLE tab1 WITH SEQUENCES means to add the table tab1 and its sequences to the publication. For other sequences (i.e., not owned by any tables), users might want to specify them individually. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Logical Replication of sequences
On Sat, Jun 8, 2024 at 6:43 PM vignesh C wrote: > On Wed, 5 Jun 2024 at 14:11, Amit Kapila wrote: > [...] > A new catalog table, pg_subscription_seq, has been introduced for > mapping subscriptions to sequences. Additionally, the sequence LSN > (Log Sequence Number) is stored, facilitating determination of > sequence changes occurring before or after the returned sequence > state. > Can't it be done using pg_depend? It seems a bit excessive unless I'm missing something. How do you track sequence mapping with the publication? Regards, Amul
Re: Logical Replication of sequences
On Mon, Jun 10, 2024 at 12:43 PM Masahiko Sawada wrote: > > On Mon, Jun 10, 2024 at 3:14 PM Masahiko Sawada wrote: > > > > On Fri, Jun 7, 2024 at 7:30 PM Amit Kapila wrote: > > > > > > > > > Are you imagining the behavior for sequences associated with tables > > > differently than the ones defined by the CREATE SEQUENCE .. command? I > > > was thinking that users would associate sequences with publications > > > similar to what we do for tables for both cases. For example, they > > > need to explicitly mention the sequences they want to replicate by > > > commands like CREATE PUBLICATION ... FOR SEQUENCE s1, s2, ...; CREATE > > > PUBLICATION ... FOR ALL SEQUENCES, or CREATE PUBLICATION ... FOR > > > SEQUENCES IN SCHEMA sch1; > > > > > > In this, variants FOR ALL SEQUENCES and SEQUENCES IN SCHEMA sch1 > > > should copy both the explicitly defined sequences and sequences > > > defined with the tables. Do you think a different variant for just > > > copying sequences implicitly associated with tables (say for identity > > > columns)? > > > > Oh, I was thinking that your proposal was to copy literally all > > sequences by REPLICA/REFRESH SEQUENCE command. > > I am trying to keep the behavior as close to tables as possible. > > But it seems to make > > sense to explicitly specify the sequences they want to replicate. It > > also means that they can create a publication that has only sequences. > > In this case, even if they create a subscription for that publication, > > we don't launch any apply workers for that subscription. Right? > > Right, good point. I had not thought about this. > > Also, given that the main use case (at least as the first step) is > > version upgrade, do we really need to support SEQUENCES IN SCHEMA and > > even FOR SEQUENCE? > At the very least, we can split the patch to move these variants to a separate patch. Once the main patch is finalized, we can try to evaluate the remaining separately. > Also, I guess that specifying individual sequences might not be easy > to use for users in some cases. For sequences owned by a column of a > table, users might want to specify them altogether, rather than > separately. For example, CREATE PUBLICATION ... FOR TABLE tab1 WITH > SEQUENCES means to add the table tab1 and its sequences to the > publication. For other sequences (i.e., not owned by any tables), > users might want to specify them individually. > Yeah, or we can have a syntax like CREATE PUBLICATION ... FOR TABLE tab1 INCLUDE SEQUENCES. Normally, we use the WITH clause for options (For example, CREATE SUBSCRIPTION ... WITH (streaming=...)). -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote: > > > > On Sat, Jun 8, 2024 at 6:43 PM vignesh C wrote: >> >> On Wed, 5 Jun 2024 at 14:11, Amit Kapila wrote: >> [...] >> A new catalog table, pg_subscription_seq, has been introduced for >> mapping subscriptions to sequences. Additionally, the sequence LSN >> (Log Sequence Number) is stored, facilitating determination of >> sequence changes occurring before or after the returned sequence >> state. > > > Can't it be done using pg_depend? It seems a bit excessive unless I'm missing > something. We'll require the lsn because the sequence LSN informs the user that it has been synchronized up to the LSN in pg_subscription_seq. Since we are not supporting incremental sync, the user will be able to identify if he should run refresh sequences or not by checking the lsn of the pg_subscription_seq and the lsn of the sequence(using pg_sequence_state added) in the publisher. Also, this parallels our implementation for pg_subscription_seq and will aid in expanding for a) incremental synchronization and b) utilizing workers for synchronization using sequence states if necessary. How do you track sequence mapping with the publication? In the publisher we use pg_publication_rel and pg_publication_namespace for mapping the sequences with the publication. Regards, Vignesh Vignesh
Re: Logical Replication of sequences
On Mon, 10 Jun 2024 at 14:48, Amit Kapila wrote: > > On Mon, Jun 10, 2024 at 12:43 PM Masahiko Sawada > wrote: > > > > On Mon, Jun 10, 2024 at 3:14 PM Masahiko Sawada > > wrote: > > > > > > On Fri, Jun 7, 2024 at 7:30 PM Amit Kapila > > > wrote: > > > > > > > > > > > > Are you imagining the behavior for sequences associated with tables > > > > differently than the ones defined by the CREATE SEQUENCE .. command? I > > > > was thinking that users would associate sequences with publications > > > > similar to what we do for tables for both cases. For example, they > > > > need to explicitly mention the sequences they want to replicate by > > > > commands like CREATE PUBLICATION ... FOR SEQUENCE s1, s2, ...; CREATE > > > > PUBLICATION ... FOR ALL SEQUENCES, or CREATE PUBLICATION ... FOR > > > > SEQUENCES IN SCHEMA sch1; > > > > > > > > In this, variants FOR ALL SEQUENCES and SEQUENCES IN SCHEMA sch1 > > > > should copy both the explicitly defined sequences and sequences > > > > defined with the tables. Do you think a different variant for just > > > > copying sequences implicitly associated with tables (say for identity > > > > columns)? > > > > > > Oh, I was thinking that your proposal was to copy literally all > > > sequences by REPLICA/REFRESH SEQUENCE command. > > > > > I am trying to keep the behavior as close to tables as possible. > > > > But it seems to make > > > sense to explicitly specify the sequences they want to replicate. It > > > also means that they can create a publication that has only sequences. > > > In this case, even if they create a subscription for that publication, > > > we don't launch any apply workers for that subscription. Right? > > > > > Right, good point. I had not thought about this. > > > > Also, given that the main use case (at least as the first step) is > > > version upgrade, do we really need to support SEQUENCES IN SCHEMA and > > > even FOR SEQUENCE? > > > > At the very least, we can split the patch to move these variants to a > separate patch. Once the main patch is finalized, we can try to > evaluate the remaining separately. I engaged in an offline discussion with Amit about strategizing the division of patches to facilitate the review process. We agreed on the following split: The first patch will encompass the setting and getting of sequence values (core sequence changes). The second patch will cover all changes on the publisher side related to "FOR ALL SEQUENCES." The third patch will address subscriber side changes aimed at synchronizing "FOR ALL SEQUENCES" publications. The fourth patch will focus on supporting "FOR SEQUENCE" publication. Lastly, the fifth patch will introduce support for "FOR ALL SEQUENCES IN SCHEMA" publication. I will work on this and share an updated patch for the same soon. Regards, Vignesh
Re: Logical Replication of sequences
On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote: > On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote: > > > > > > > > On Sat, Jun 8, 2024 at 6:43 PM vignesh C wrote: > >> > >> On Wed, 5 Jun 2024 at 14:11, Amit Kapila > wrote: > >> [...] > >> A new catalog table, pg_subscription_seq, has been introduced for > >> mapping subscriptions to sequences. Additionally, the sequence LSN > >> (Log Sequence Number) is stored, facilitating determination of > >> sequence changes occurring before or after the returned sequence > >> state. > > > > > > Can't it be done using pg_depend? It seems a bit excessive unless I'm > missing > > something. > > We'll require the lsn because the sequence LSN informs the user that > it has been synchronized up to the LSN in pg_subscription_seq. Since > we are not supporting incremental sync, the user will be able to > identify if he should run refresh sequences or not by checking the lsn > of the pg_subscription_seq and the lsn of the sequence(using > pg_sequence_state added) in the publisher. Also, this parallels our > implementation for pg_subscription_seq and will aid in expanding for > a) incremental synchronization and b) utilizing workers for > synchronization using sequence states if necessary. > > How do you track sequence mapping with the publication? > > In the publisher we use pg_publication_rel and > pg_publication_namespace for mapping the sequences with the > publication. > Thanks for the explanation. I'm wondering what the complexity would be, if we wanted to do something similar on the subscriber side, i.e., tracking via pg_subscription_rel. Regards, Amul
Re: Logical Replication of sequences
On Tue, 11 Jun 2024 at 09:41, Amul Sul wrote: > > On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote: >> >> On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote: >> > >> > >> > >> > On Sat, Jun 8, 2024 at 6:43 PM vignesh C wrote: >> >> >> >> On Wed, 5 Jun 2024 at 14:11, Amit Kapila wrote: >> >> [...] >> >> A new catalog table, pg_subscription_seq, has been introduced for >> >> mapping subscriptions to sequences. Additionally, the sequence LSN >> >> (Log Sequence Number) is stored, facilitating determination of >> >> sequence changes occurring before or after the returned sequence >> >> state. >> > >> > >> > Can't it be done using pg_depend? It seems a bit excessive unless I'm >> > missing >> > something. >> >> We'll require the lsn because the sequence LSN informs the user that >> it has been synchronized up to the LSN in pg_subscription_seq. Since >> we are not supporting incremental sync, the user will be able to >> identify if he should run refresh sequences or not by checking the lsn >> of the pg_subscription_seq and the lsn of the sequence(using >> pg_sequence_state added) in the publisher. Also, this parallels our >> implementation for pg_subscription_seq and will aid in expanding for >> a) incremental synchronization and b) utilizing workers for >> synchronization using sequence states if necessary. >> >> How do you track sequence mapping with the publication? >> >> In the publisher we use pg_publication_rel and >> pg_publication_namespace for mapping the sequences with the >> publication. > > > Thanks for the explanation. I'm wondering what the complexity would be, if we > wanted to do something similar on the subscriber side, i.e., tracking via > pg_subscription_rel. Because we won't utilize sync workers to synchronize the sequence, and the sequence won't necessitate sync states like init, sync, finishedcopy, syncdone, ready, etc., initially, I considered keeping the sequences separate. However, I'm ok with using pg_subscription_rel as it could potentially help in enhancing incremental synchronization and parallelizing later on. Regards, Vignesh
Re: Logical Replication of sequences
On Tue, Jun 11, 2024 at 12:25 PM vignesh C wrote: > > On Mon, 10 Jun 2024 at 14:48, Amit Kapila wrote: > > > > On Mon, Jun 10, 2024 at 12:43 PM Masahiko Sawada > > wrote: > > > > > > On Mon, Jun 10, 2024 at 3:14 PM Masahiko Sawada > > > wrote: > > > > > > > > On Fri, Jun 7, 2024 at 7:30 PM Amit Kapila > > > > wrote: > > > > > > > > > > > > > > > Are you imagining the behavior for sequences associated with tables > > > > > differently than the ones defined by the CREATE SEQUENCE .. command? I > > > > > was thinking that users would associate sequences with publications > > > > > similar to what we do for tables for both cases. For example, they > > > > > need to explicitly mention the sequences they want to replicate by > > > > > commands like CREATE PUBLICATION ... FOR SEQUENCE s1, s2, ...; CREATE > > > > > PUBLICATION ... FOR ALL SEQUENCES, or CREATE PUBLICATION ... FOR > > > > > SEQUENCES IN SCHEMA sch1; > > > > > > > > > > In this, variants FOR ALL SEQUENCES and SEQUENCES IN SCHEMA sch1 > > > > > should copy both the explicitly defined sequences and sequences > > > > > defined with the tables. Do you think a different variant for just > > > > > copying sequences implicitly associated with tables (say for identity > > > > > columns)? > > > > > > > > Oh, I was thinking that your proposal was to copy literally all > > > > sequences by REPLICA/REFRESH SEQUENCE command. > > > > > > > > I am trying to keep the behavior as close to tables as possible. > > > > > > But it seems to make > > > > sense to explicitly specify the sequences they want to replicate. It > > > > also means that they can create a publication that has only sequences. > > > > In this case, even if they create a subscription for that publication, > > > > we don't launch any apply workers for that subscription. Right? > > > > > > > > Right, good point. I had not thought about this. > > > > > > Also, given that the main use case (at least as the first step) is > > > > version upgrade, do we really need to support SEQUENCES IN SCHEMA and > > > > even FOR SEQUENCE? > > > > > > > At the very least, we can split the patch to move these variants to a > > separate patch. Once the main patch is finalized, we can try to > > evaluate the remaining separately. > > I engaged in an offline discussion with Amit about strategizing the > division of patches to facilitate the review process. We agreed on the > following split: The first patch will encompass the setting and > getting of sequence values (core sequence changes). The second patch > will cover all changes on the publisher side related to "FOR ALL > SEQUENCES." The third patch will address subscriber side changes aimed > at synchronizing "FOR ALL SEQUENCES" publications. The fourth patch > will focus on supporting "FOR SEQUENCE" publication. Lastly, the fifth > patch will introduce support for "FOR ALL SEQUENCES IN SCHEMA" > publication. > > I will work on this and share an updated patch for the same soon. +1. Sounds like a good plan. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Logical Replication of sequences
On Tue, 11 Jun 2024 at 12:38, Masahiko Sawada wrote: > > On Tue, Jun 11, 2024 at 12:25 PM vignesh C wrote: > > > > On Mon, 10 Jun 2024 at 14:48, Amit Kapila wrote: > > > > > > On Mon, Jun 10, 2024 at 12:43 PM Masahiko Sawada > > > wrote: > > > > > > > > On Mon, Jun 10, 2024 at 3:14 PM Masahiko Sawada > > > > wrote: > > > > > > > > > > On Fri, Jun 7, 2024 at 7:30 PM Amit Kapila > > > > > wrote: > > > > > > > > > > > > > > > > > > Are you imagining the behavior for sequences associated with tables > > > > > > differently than the ones defined by the CREATE SEQUENCE .. > > > > > > command? I > > > > > > was thinking that users would associate sequences with publications > > > > > > similar to what we do for tables for both cases. For example, they > > > > > > need to explicitly mention the sequences they want to replicate by > > > > > > commands like CREATE PUBLICATION ... FOR SEQUENCE s1, s2, ...; > > > > > > CREATE > > > > > > PUBLICATION ... FOR ALL SEQUENCES, or CREATE PUBLICATION ... FOR > > > > > > SEQUENCES IN SCHEMA sch1; > > > > > > > > > > > > In this, variants FOR ALL SEQUENCES and SEQUENCES IN SCHEMA sch1 > > > > > > should copy both the explicitly defined sequences and sequences > > > > > > defined with the tables. Do you think a different variant for just > > > > > > copying sequences implicitly associated with tables (say for > > > > > > identity > > > > > > columns)? > > > > > > > > > > Oh, I was thinking that your proposal was to copy literally all > > > > > sequences by REPLICA/REFRESH SEQUENCE command. > > > > > > > > > > > I am trying to keep the behavior as close to tables as possible. > > > > > > > > But it seems to make > > > > > sense to explicitly specify the sequences they want to replicate. It > > > > > also means that they can create a publication that has only sequences. > > > > > In this case, even if they create a subscription for that publication, > > > > > we don't launch any apply workers for that subscription. Right? > > > > > > > > > > > Right, good point. I had not thought about this. > > > > > > > > Also, given that the main use case (at least as the first step) is > > > > > version upgrade, do we really need to support SEQUENCES IN SCHEMA and > > > > > even FOR SEQUENCE? > > > > > > > > > > At the very least, we can split the patch to move these variants to a > > > separate patch. Once the main patch is finalized, we can try to > > > evaluate the remaining separately. > > > > I engaged in an offline discussion with Amit about strategizing the > > division of patches to facilitate the review process. We agreed on the > > following split: The first patch will encompass the setting and > > getting of sequence values (core sequence changes). The second patch > > will cover all changes on the publisher side related to "FOR ALL > > SEQUENCES." The third patch will address subscriber side changes aimed > > at synchronizing "FOR ALL SEQUENCES" publications. The fourth patch > > will focus on supporting "FOR SEQUENCE" publication. Lastly, the fifth > > patch will introduce support for "FOR ALL SEQUENCES IN SCHEMA" > > publication. > > > > I will work on this and share an updated patch for the same soon. > > +1. Sounds like a good plan. Amit and I engaged in an offline discussion regarding the design and contemplated that it could be like below: 1) CREATE PUBLICATION syntax enhancement: CREATE PUBLICATION ... FOR ALL SEQUENCES; The addition of a new column titled "all sequences" in the pg_publication system table will signify whether the publication is designated as all sequences publication or not. 2) CREATE SUBSCRIPTION -- no syntax change. Upon creation of a subscription, the following additional steps will be managed by the subscriber: i) The subscriber will retrieve the list of sequences associated with the subscription's publications. ii) For each sequence: a) Retrieve the sequence value from the publisher by invoking the pg_sequence_state function. b) Set the sequence with the value obtained from the publisher. iv) Once the subscription creation is completed, all sequence values will become visible at the subscriber's end. An alternative design approach could involve retrieving the sequence list from the publisher during subscription creation and inserting the sequences with an "init" state into the pg_subscription_rel system table. These tasks could be executed by a single sequence sync worker, which would: i) Retrieve the list of sequences in the "init" state from the pg_subscription_rel system table. ii) Initiate a transaction. iii) For each sequence: a) Obtain the sequence value from the publisher by utilizing the pg_sequence_state function. b) Update the sequence with the value obtained from the publisher. iv) Commit the transaction. The benefit with the second approach is that if there are large number of sequences, the sequence sync can be enhanced to happen in parallel and also if there are any locks held on the sequences in the publisher, the sequ
Re: Logical Replication of sequences
On Tue, Jun 11, 2024 at 7:36 PM vignesh C wrote: > > On Tue, 11 Jun 2024 at 12:38, Masahiko Sawada wrote: > > > > On Tue, Jun 11, 2024 at 12:25 PM vignesh C wrote: > > > > > > On Mon, 10 Jun 2024 at 14:48, Amit Kapila wrote: > > > > > > > > On Mon, Jun 10, 2024 at 12:43 PM Masahiko Sawada > > > > wrote: > > > > > > > > > > On Mon, Jun 10, 2024 at 3:14 PM Masahiko Sawada > > > > > wrote: > > > > > > > > > > > > On Fri, Jun 7, 2024 at 7:30 PM Amit Kapila > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Are you imagining the behavior for sequences associated with > > > > > > > tables > > > > > > > differently than the ones defined by the CREATE SEQUENCE .. > > > > > > > command? I > > > > > > > was thinking that users would associate sequences with > > > > > > > publications > > > > > > > similar to what we do for tables for both cases. For example, they > > > > > > > need to explicitly mention the sequences they want to replicate by > > > > > > > commands like CREATE PUBLICATION ... FOR SEQUENCE s1, s2, ...; > > > > > > > CREATE > > > > > > > PUBLICATION ... FOR ALL SEQUENCES, or CREATE PUBLICATION ... FOR > > > > > > > SEQUENCES IN SCHEMA sch1; > > > > > > > > > > > > > > In this, variants FOR ALL SEQUENCES and SEQUENCES IN SCHEMA sch1 > > > > > > > should copy both the explicitly defined sequences and sequences > > > > > > > defined with the tables. Do you think a different variant for just > > > > > > > copying sequences implicitly associated with tables (say for > > > > > > > identity > > > > > > > columns)? > > > > > > > > > > > > Oh, I was thinking that your proposal was to copy literally all > > > > > > sequences by REPLICA/REFRESH SEQUENCE command. > > > > > > > > > > > > > > I am trying to keep the behavior as close to tables as possible. > > > > > > > > > > But it seems to make > > > > > > sense to explicitly specify the sequences they want to replicate. It > > > > > > also means that they can create a publication that has only > > > > > > sequences. > > > > > > In this case, even if they create a subscription for that > > > > > > publication, > > > > > > we don't launch any apply workers for that subscription. Right? > > > > > > > > > > > > > > Right, good point. I had not thought about this. > > > > > > > > > > Also, given that the main use case (at least as the first step) is > > > > > > version upgrade, do we really need to support SEQUENCES IN SCHEMA > > > > > > and > > > > > > even FOR SEQUENCE? > > > > > > > > > > > > > At the very least, we can split the patch to move these variants to a > > > > separate patch. Once the main patch is finalized, we can try to > > > > evaluate the remaining separately. > > > > > > I engaged in an offline discussion with Amit about strategizing the > > > division of patches to facilitate the review process. We agreed on the > > > following split: The first patch will encompass the setting and > > > getting of sequence values (core sequence changes). The second patch > > > will cover all changes on the publisher side related to "FOR ALL > > > SEQUENCES." The third patch will address subscriber side changes aimed > > > at synchronizing "FOR ALL SEQUENCES" publications. The fourth patch > > > will focus on supporting "FOR SEQUENCE" publication. Lastly, the fifth > > > patch will introduce support for "FOR ALL SEQUENCES IN SCHEMA" > > > publication. > > > > > > I will work on this and share an updated patch for the same soon. > > > > +1. Sounds like a good plan. > > Amit and I engaged in an offline discussion regarding the design and > contemplated that it could be like below: > 1) CREATE PUBLICATION syntax enhancement: > CREATE PUBLICATION ... FOR ALL SEQUENCES; > The addition of a new column titled "all sequences" in the > pg_publication system table will signify whether the publication is > designated as all sequences publication or not. > The first approach sounds like we don't create entries for sequences in pg_subscription_rel. In this case, how do we know all sequences that we need to refresh when executing the REFRESH PUBLICATION SEQUENCES command you mentioned below? > 2) CREATE SUBSCRIPTION -- no syntax change. > Upon creation of a subscription, the following additional steps will > be managed by the subscriber: > i) The subscriber will retrieve the list of sequences associated with > the subscription's publications. > ii) For each sequence: a) Retrieve the sequence value from the > publisher by invoking the pg_sequence_state function. b) Set the > sequence with the value obtained from the publisher. iv) Once the > subscription creation is completed, all sequence values will become > visible at the subscriber's end. Sequence values are always copied from the publisher? or does it happen only when copy_data = true? > > An alternative design approach could involve retrieving the sequence > list from the publisher during subscription creation and inserting the > sequences with an "init" state into the pg_subscription_rel
Re: Logical Replication of sequences
On Tue, Jun 11, 2024 at 4:06 PM vignesh C wrote: > > Amit and I engaged in an offline discussion regarding the design and > contemplated that it could be like below: If I understand correctly, does this require the sequences to already exist on the subscribing node before creating the subscription, or will it also copy any non-existing sequences? > 1) CREATE PUBLICATION syntax enhancement: > CREATE PUBLICATION ... FOR ALL SEQUENCES; > The addition of a new column titled "all sequences" in the > pg_publication system table will signify whether the publication is > designated as all sequences publication or not. > > 2) CREATE SUBSCRIPTION -- no syntax change. > Upon creation of a subscription, the following additional steps will > be managed by the subscriber: > i) The subscriber will retrieve the list of sequences associated with > the subscription's publications. > ii) For each sequence: a) Retrieve the sequence value from the > publisher by invoking the pg_sequence_state function. b) Set the > sequence with the value obtained from the publisher. iv) Once the > subscription creation is completed, all sequence values will become > visible at the subscriber's end. > > An alternative design approach could involve retrieving the sequence > list from the publisher during subscription creation and inserting the > sequences with an "init" state into the pg_subscription_rel system > table. These tasks could be executed by a single sequence sync worker, > which would: > i) Retrieve the list of sequences in the "init" state from the > pg_subscription_rel system table. > ii) Initiate a transaction. > iii) For each sequence: a) Obtain the sequence value from the > publisher by utilizing the pg_sequence_state function. b) Update the > sequence with the value obtained from the publisher. > iv) Commit the transaction. > > The benefit with the second approach is that if there are large number > of sequences, the sequence sync can be enhanced to happen in parallel > and also if there are any locks held on the sequences in the > publisher, the sequence worker can wait to acquire the lock instead of > blocking the whole create subscription command which will delay the > initial copy of the tables too. Yeah w.r.t. this point second approach seems better. > 3) Refreshing the sequence can be achieved through the existing > command: ALTER SUBSCRIPTION ... REFRESH PUBLICATION(no syntax change > here). > The subscriber identifies stale sequences, meaning sequences present > in pg_subscription_rel but absent from the publication, and removes > them from the pg_subscription_rel system table. The subscriber also > checks for newly added sequences in the publisher and synchronizes > their values from the publisher using the steps outlined in the > subscription creation process. It's worth noting that previously > synchronized sequences won't be synchronized again; the sequence sync > will occur solely for the newly added sequences. > 4) Introducing a new command for refreshing all sequences: ALTER > SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES. > The subscriber will remove stale sequences and add newly added > sequences from the publisher. Following this, it will re-synchronize > the sequence values for all sequences in the updated list from the > publisher, following the steps outlined in the subscription creation > process. Okay, this answers my first question: we will remove the sequences that are removed from the publisher and add the new sequences. I don't see any problem with this, but doesn't it seem like we are effectively doing DDL replication only for sequences without having a comprehensive plan for overall DDL replication? > 5) Incorporate the pg_sequence_state function to fetch the sequence > value from the publisher, along with the page LSN. Incorporate > SetSequence function, which will procure a new relfilenode for the > sequence and set the new relfilenode with the specified value. This > will facilitate rollback in case of any failures. I do not understand this point, you mean whenever we are fetching the sequence value from the publisher we need to create a new relfilenode on the subscriber? Why not just update the catalog tuple is sufficient? Or this is for handling the ALTER SEQUENCE case? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Logical Replication of sequences
On Wed, Jun 12, 2024 at 10:44 AM Masahiko Sawada wrote: > > On Tue, Jun 11, 2024 at 7:36 PM vignesh C wrote: > > > > 1) CREATE PUBLICATION syntax enhancement: > > CREATE PUBLICATION ... FOR ALL SEQUENCES; > > The addition of a new column titled "all sequences" in the > > pg_publication system table will signify whether the publication is > > designated as all sequences publication or not. > > > > The first approach sounds like we don't create entries for sequences > in pg_subscription_rel. In this case, how do we know all sequences > that we need to refresh when executing the REFRESH PUBLICATION > SEQUENCES command you mentioned below? > As per my understanding, we should be creating entries for sequences in pg_subscription_rel similar to tables. The difference would be that we won't need all the sync_states (i = initialize, d = data is being copied, f = finished table copy, s = synchronized, r = ready) as we don't need any synchronization with apply workers. > > 2) CREATE SUBSCRIPTION -- no syntax change. > > Upon creation of a subscription, the following additional steps will > > be managed by the subscriber: > > i) The subscriber will retrieve the list of sequences associated with > > the subscription's publications. > > ii) For each sequence: a) Retrieve the sequence value from the > > publisher by invoking the pg_sequence_state function. b) Set the > > sequence with the value obtained from the publisher. iv) Once the > > subscription creation is completed, all sequence values will become > > visible at the subscriber's end. > > Sequence values are always copied from the publisher? or does it > happen only when copy_data = true? > It is better to do it when "copy_data = true" to keep it compatible with the table's behavior. > > > > An alternative design approach could involve retrieving the sequence > > list from the publisher during subscription creation and inserting the > > sequences with an "init" state into the pg_subscription_rel system > > table. These tasks could be executed by a single sequence sync worker, > > which would: > > i) Retrieve the list of sequences in the "init" state from the > > pg_subscription_rel system table. > > ii) Initiate a transaction. > > iii) For each sequence: a) Obtain the sequence value from the > > publisher by utilizing the pg_sequence_state function. b) Update the > > sequence with the value obtained from the publisher. > > iv) Commit the transaction. > > > > The benefit with the second approach is that if there are large number > > of sequences, the sequence sync can be enhanced to happen in parallel > > and also if there are any locks held on the sequences in the > > publisher, the sequence worker can wait to acquire the lock instead of > > blocking the whole create subscription command which will delay the > > initial copy of the tables too. > > I prefer to have separate workers to sync sequences. > +1. > Probably we can > start with a single worker and extend it to have multiple workers. Yeah, starting with a single worker sounds good for now. Do you think we should sync all the sequences in a single transaction or have some threshold value above which a different transaction would be required or maybe a different sequence sync worker altogether? Now, having multiple sequence-sync workers requires some synchronization so that only a single worker is allocated for one sequence. The simplest thing is to use a single sequence sync worker that syncs all sequences in one transaction but with a large number of sequences, it could be inefficient. OTOH, I am not sure if it would be a problem in reality. > > BTW > the sequence-sync worker will be taken from > max_sync_workers_per_subscription pool? > I think so. > Or yet another idea I came up with is that a tablesync worker will > synchronize both the table and sequences owned by the table. That is, > after the tablesync worker caught up with the apply worker, the > tablesync worker synchronizes sequences associated with the target > table as well. One benefit would be that at the time of initial table > sync being completed, the table and its sequence data are consistent. > As soon as new changes come to the table, it would become inconsistent > so it might not be helpful much, though. Also, sequences that are not > owned by any table will still need to be synchronized by someone. > The other thing to consider in this idea is that we somehow need to distinguish the sequences owned by the table. > > > > 3) Refreshing the sequence can be achieved through the existing > > command: ALTER SUBSCRIPTION ... REFRESH PUBLICATION(no syntax change > > here). > > The subscriber identifies stale sequences, meaning sequences present > > in pg_subscription_rel but absent from the publication, and removes > > them from the pg_subscription_rel system table. The subscriber also > > checks for newly added sequences in the publisher and synchronizes > > their values from the publisher using the steps outlined in the > >
Re: Logical Replication of sequences
On Wed, 12 Jun 2024 at 10:51, Dilip Kumar wrote: > > On Tue, Jun 11, 2024 at 4:06 PM vignesh C wrote: > > > > Amit and I engaged in an offline discussion regarding the design and > > contemplated that it could be like below: > > If I understand correctly, does this require the sequences to already > exist on the subscribing node before creating the subscription, or > will it also copy any non-existing sequences? Sequences must exist in the subscriber; we'll synchronize only their values. Any sequences that are not present in the subscriber will trigger an error. > > 1) CREATE PUBLICATION syntax enhancement: > > CREATE PUBLICATION ... FOR ALL SEQUENCES; > > The addition of a new column titled "all sequences" in the > > pg_publication system table will signify whether the publication is > > designated as all sequences publication or not. > > > > 2) CREATE SUBSCRIPTION -- no syntax change. > > Upon creation of a subscription, the following additional steps will > > be managed by the subscriber: > > i) The subscriber will retrieve the list of sequences associated with > > the subscription's publications. > > ii) For each sequence: a) Retrieve the sequence value from the > > publisher by invoking the pg_sequence_state function. b) Set the > > sequence with the value obtained from the publisher. iv) Once the > > subscription creation is completed, all sequence values will become > > visible at the subscriber's end. > > > > An alternative design approach could involve retrieving the sequence > > list from the publisher during subscription creation and inserting the > > sequences with an "init" state into the pg_subscription_rel system > > table. These tasks could be executed by a single sequence sync worker, > > which would: > > i) Retrieve the list of sequences in the "init" state from the > > pg_subscription_rel system table. > > ii) Initiate a transaction. > > iii) For each sequence: a) Obtain the sequence value from the > > publisher by utilizing the pg_sequence_state function. b) Update the > > sequence with the value obtained from the publisher. > > iv) Commit the transaction. > > > > The benefit with the second approach is that if there are large number > > of sequences, the sequence sync can be enhanced to happen in parallel > > and also if there are any locks held on the sequences in the > > publisher, the sequence worker can wait to acquire the lock instead of > > blocking the whole create subscription command which will delay the > > initial copy of the tables too. > > Yeah w.r.t. this point second approach seems better. ok > > 3) Refreshing the sequence can be achieved through the existing > > command: ALTER SUBSCRIPTION ... REFRESH PUBLICATION(no syntax change > > here). > > The subscriber identifies stale sequences, meaning sequences present > > in pg_subscription_rel but absent from the publication, and removes > > them from the pg_subscription_rel system table. The subscriber also > > checks for newly added sequences in the publisher and synchronizes > > their values from the publisher using the steps outlined in the > > subscription creation process. It's worth noting that previously > > synchronized sequences won't be synchronized again; the sequence sync > > will occur solely for the newly added sequences. > > > 4) Introducing a new command for refreshing all sequences: ALTER > > SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES. > > The subscriber will remove stale sequences and add newly added > > sequences from the publisher. Following this, it will re-synchronize > > the sequence values for all sequences in the updated list from the > > publisher, following the steps outlined in the subscription creation > > process. > > Okay, this answers my first question: we will remove the sequences > that are removed from the publisher and add the new sequences. I don't > see any problem with this, but doesn't it seem like we are effectively > doing DDL replication only for sequences without having a > comprehensive plan for overall DDL replication? What I intended to convey is that we'll eliminate the sequences from pg_subscription_rel. We won't facilitate the DDL replication of sequences; instead, we anticipate users to create the sequences themselves. > > 5) Incorporate the pg_sequence_state function to fetch the sequence > > value from the publisher, along with the page LSN. Incorporate > > SetSequence function, which will procure a new relfilenode for the > > sequence and set the new relfilenode with the specified value. This > > will facilitate rollback in case of any failures. > > I do not understand this point, you mean whenever we are fetching the > sequence value from the publisher we need to create a new relfilenode > on the subscriber? Why not just update the catalog tuple is > sufficient? Or this is for handling the ALTER SEQUENCE case? Sequences operate distinctively from tables. Alterations to sequences reflect instantly in another session, even before committing the transaction
Re: Logical Replication of sequences
On Wed, Jun 12, 2024 at 4:08 PM vignesh C wrote: > > On Wed, 12 Jun 2024 at 10:51, Dilip Kumar wrote: > > > > On Tue, Jun 11, 2024 at 4:06 PM vignesh C wrote: > > > > > > Amit and I engaged in an offline discussion regarding the design and > > > contemplated that it could be like below: > > > > If I understand correctly, does this require the sequences to already > > exist on the subscribing node before creating the subscription, or > > will it also copy any non-existing sequences? > > Sequences must exist in the subscriber; we'll synchronize only their > values. Any sequences that are not present in the subscriber will > trigger an error. Okay, that makes sense. > > > > 3) Refreshing the sequence can be achieved through the existing > > > command: ALTER SUBSCRIPTION ... REFRESH PUBLICATION(no syntax change > > > here). > > > The subscriber identifies stale sequences, meaning sequences present > > > in pg_subscription_rel but absent from the publication, and removes > > > them from the pg_subscription_rel system table. The subscriber also > > > checks for newly added sequences in the publisher and synchronizes > > > their values from the publisher using the steps outlined in the > > > subscription creation process. It's worth noting that previously > > > synchronized sequences won't be synchronized again; the sequence sync > > > will occur solely for the newly added sequences. > > > > > 4) Introducing a new command for refreshing all sequences: ALTER > > > SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES. > > > The subscriber will remove stale sequences and add newly added > > > sequences from the publisher. Following this, it will re-synchronize > > > the sequence values for all sequences in the updated list from the > > > publisher, following the steps outlined in the subscription creation > > > process. > > > > Okay, this answers my first question: we will remove the sequences > > that are removed from the publisher and add the new sequences. I don't > > see any problem with this, but doesn't it seem like we are effectively > > doing DDL replication only for sequences without having a > > comprehensive plan for overall DDL replication? > > What I intended to convey is that we'll eliminate the sequences from > pg_subscription_rel. We won't facilitate the DDL replication of > sequences; instead, we anticipate users to create the sequences > themselves. hmm okay. > > > 5) Incorporate the pg_sequence_state function to fetch the sequence > > > value from the publisher, along with the page LSN. Incorporate > > > SetSequence function, which will procure a new relfilenode for the > > > sequence and set the new relfilenode with the specified value. This > > > will facilitate rollback in case of any failures. > > > > I do not understand this point, you mean whenever we are fetching the > > sequence value from the publisher we need to create a new relfilenode > > on the subscriber? Why not just update the catalog tuple is > > sufficient? Or this is for handling the ALTER SEQUENCE case? > > Sequences operate distinctively from tables. Alterations to sequences > reflect instantly in another session, even before committing the > transaction. To ensure the synchronization of sequence value and state > updates in pg_subscription_rel, we assign it a new relfilenode. This > strategy ensures that any potential errors allow for the rollback of > both the sequence state in pg_subscription_rel and the sequence values > simultaneously. So, you're saying that when we synchronize the sequence values on the subscriber side, we will create a new relfilenode to allow reverting to the old state of the sequence in case of an error or transaction rollback? But why would we want to do that? Generally, even if you call nextval() on a sequence and then roll back the transaction, the sequence value doesn't revert to the old value. So, what specific problem on the subscriber side are we trying to avoid by operating on a new relfilenode? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Logical Replication of sequences
On Wed, 12 Jun 2024 at 17:09, Dilip Kumar wrote: > > On Wed, Jun 12, 2024 at 4:08 PM vignesh C wrote: > > > > On Wed, 12 Jun 2024 at 10:51, Dilip Kumar wrote: > > > > > > On Tue, Jun 11, 2024 at 4:06 PM vignesh C wrote: > > > > > > > > Amit and I engaged in an offline discussion regarding the design and > > > > contemplated that it could be like below: > > > > > > If I understand correctly, does this require the sequences to already > > > exist on the subscribing node before creating the subscription, or > > > will it also copy any non-existing sequences? > > > > Sequences must exist in the subscriber; we'll synchronize only their > > values. Any sequences that are not present in the subscriber will > > trigger an error. > > Okay, that makes sense. > > > > > > > 3) Refreshing the sequence can be achieved through the existing > > > > command: ALTER SUBSCRIPTION ... REFRESH PUBLICATION(no syntax change > > > > here). > > > > The subscriber identifies stale sequences, meaning sequences present > > > > in pg_subscription_rel but absent from the publication, and removes > > > > them from the pg_subscription_rel system table. The subscriber also > > > > checks for newly added sequences in the publisher and synchronizes > > > > their values from the publisher using the steps outlined in the > > > > subscription creation process. It's worth noting that previously > > > > synchronized sequences won't be synchronized again; the sequence sync > > > > will occur solely for the newly added sequences. > > > > > > > 4) Introducing a new command for refreshing all sequences: ALTER > > > > SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES. > > > > The subscriber will remove stale sequences and add newly added > > > > sequences from the publisher. Following this, it will re-synchronize > > > > the sequence values for all sequences in the updated list from the > > > > publisher, following the steps outlined in the subscription creation > > > > process. > > > > > > Okay, this answers my first question: we will remove the sequences > > > that are removed from the publisher and add the new sequences. I don't > > > see any problem with this, but doesn't it seem like we are effectively > > > doing DDL replication only for sequences without having a > > > comprehensive plan for overall DDL replication? > > > > What I intended to convey is that we'll eliminate the sequences from > > pg_subscription_rel. We won't facilitate the DDL replication of > > sequences; instead, we anticipate users to create the sequences > > themselves. > > hmm okay. > > > > > 5) Incorporate the pg_sequence_state function to fetch the sequence > > > > value from the publisher, along with the page LSN. Incorporate > > > > SetSequence function, which will procure a new relfilenode for the > > > > sequence and set the new relfilenode with the specified value. This > > > > will facilitate rollback in case of any failures. > > > > > > I do not understand this point, you mean whenever we are fetching the > > > sequence value from the publisher we need to create a new relfilenode > > > on the subscriber? Why not just update the catalog tuple is > > > sufficient? Or this is for handling the ALTER SEQUENCE case? > > > > Sequences operate distinctively from tables. Alterations to sequences > > reflect instantly in another session, even before committing the > > transaction. To ensure the synchronization of sequence value and state > > updates in pg_subscription_rel, we assign it a new relfilenode. This > > strategy ensures that any potential errors allow for the rollback of > > both the sequence state in pg_subscription_rel and the sequence values > > simultaneously. > > So, you're saying that when we synchronize the sequence values on the > subscriber side, we will create a new relfilenode to allow reverting > to the old state of the sequence in case of an error or transaction > rollback? But why would we want to do that? Generally, even if you > call nextval() on a sequence and then roll back the transaction, the > sequence value doesn't revert to the old value. So, what specific > problem on the subscriber side are we trying to avoid by operating on > a new relfilenode? Let's consider a situation where we have two sequences: seq1 with a value of 100 and seq2 with a value of 200. Now, let's say seq1 is synced and updated to 100, then we attempt to synchronize seq2, there's a failure due to the sequence not existing or encountering some other issue. In this scenario, we don't want to halt operations where seq1 is synchronized, but the sequence state for sequence isn't changed to "ready" in pg_subscription_rel. Updating the sequence data directly reflects the sequence change immediately. However, if we assign a new relfile node for the sequence and update the sequence value for the new relfile node, until the transaction is committed, other concurrent users will still be utilizing the old relfile node for the sequence, and only the old data will be visible. Once
Re: Logical Replication of sequences
On Thu, Jun 13, 2024 at 10:10 AM vignesh C wrote: > > > So, you're saying that when we synchronize the sequence values on the > > subscriber side, we will create a new relfilenode to allow reverting > > to the old state of the sequence in case of an error or transaction > > rollback? But why would we want to do that? Generally, even if you > > call nextval() on a sequence and then roll back the transaction, the > > sequence value doesn't revert to the old value. So, what specific > > problem on the subscriber side are we trying to avoid by operating on > > a new relfilenode? > > Let's consider a situation where we have two sequences: seq1 with a > value of 100 and seq2 with a value of 200. Now, let's say seq1 is > synced and updated to 100, then we attempt to synchronize seq2, > there's a failure due to the sequence not existing or encountering > some other issue. In this scenario, we don't want to halt operations > where seq1 is synchronized, but the sequence state for sequence isn't > changed to "ready" in pg_subscription_rel. Thanks for the explanation, but I am still not getting it completely, do you mean to say unless all the sequences are not synced any of the sequences would not be marked "ready" in pg_subscription_rel? Is that necessary? I mean why we can not sync the sequences one by one and mark them ready? Why it is necessary to either have all the sequences synced or none of them? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Logical Replication of sequences
On Thu, 13 Jun 2024 at 10:27, Dilip Kumar wrote: > > On Thu, Jun 13, 2024 at 10:10 AM vignesh C wrote: > > > > > So, you're saying that when we synchronize the sequence values on the > > > subscriber side, we will create a new relfilenode to allow reverting > > > to the old state of the sequence in case of an error or transaction > > > rollback? But why would we want to do that? Generally, even if you > > > call nextval() on a sequence and then roll back the transaction, the > > > sequence value doesn't revert to the old value. So, what specific > > > problem on the subscriber side are we trying to avoid by operating on > > > a new relfilenode? > > > > Let's consider a situation where we have two sequences: seq1 with a > > value of 100 and seq2 with a value of 200. Now, let's say seq1 is > > synced and updated to 100, then we attempt to synchronize seq2, > > there's a failure due to the sequence not existing or encountering > > some other issue. In this scenario, we don't want to halt operations > > where seq1 is synchronized, but the sequence state for sequence isn't > > changed to "ready" in pg_subscription_rel. > > Thanks for the explanation, but I am still not getting it completely, > do you mean to say unless all the sequences are not synced any of the > sequences would not be marked "ready" in pg_subscription_rel? Is that > necessary? I mean why we can not sync the sequences one by one and > mark them ready? Why it is necessary to either have all the sequences > synced or none of them? Since updating the sequence is one operation and setting pg_subscription_rel is another, I was trying to avoid a situation where the sequence is updated but its state is not reflected in pg_subscription_rel. It seems you are suggesting that it's acceptable for the sequence to be updated even if its state isn't updated in pg_subscription_rel, and in such cases, the sequence value does not need to be reverted. Regards, Vignesh
Re: Logical Replication of sequences
On Thu, Jun 13, 2024 at 11:53 AM vignesh C wrote: > > On Thu, 13 Jun 2024 at 10:27, Dilip Kumar wrote: > > Thanks for the explanation, but I am still not getting it completely, > > do you mean to say unless all the sequences are not synced any of the > > sequences would not be marked "ready" in pg_subscription_rel? Is that > > necessary? I mean why we can not sync the sequences one by one and > > mark them ready? Why it is necessary to either have all the sequences > > synced or none of them? > > Since updating the sequence is one operation and setting > pg_subscription_rel is another, I was trying to avoid a situation > where the sequence is updated but its state is not reflected in > pg_subscription_rel. It seems you are suggesting that it's acceptable > for the sequence to be updated even if its state isn't updated in > pg_subscription_rel, and in such cases, the sequence value does not > need to be reverted. Right, the complexity we're adding to achieve a behavior that may not be truly desirable is a concern. For instance, if we mark the status as ready but do not sync the sequences, it could lead to issues. However, if we have synced some sequences but encounter a failure without marking the status as ready, I don't consider it inconsistent in any way. But anyway, now I understand your thinking behind that so it's a good idea to leave this design behavior for a later decision. Gathering more opinions and insights during later stages will provide a clearer perspective on how to proceed with this aspect. Thanks. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Logical Replication of sequences
On Wed, Jun 12, 2024 at 6:59 PM Amit Kapila wrote: > > On Wed, Jun 12, 2024 at 10:44 AM Masahiko Sawada > wrote: > > > > On Tue, Jun 11, 2024 at 7:36 PM vignesh C wrote: > > > > > > 1) CREATE PUBLICATION syntax enhancement: > > > CREATE PUBLICATION ... FOR ALL SEQUENCES; > > > The addition of a new column titled "all sequences" in the > > > pg_publication system table will signify whether the publication is > > > designated as all sequences publication or not. > > > > > > > The first approach sounds like we don't create entries for sequences > > in pg_subscription_rel. In this case, how do we know all sequences > > that we need to refresh when executing the REFRESH PUBLICATION > > SEQUENCES command you mentioned below? > > > > As per my understanding, we should be creating entries for sequences > in pg_subscription_rel similar to tables. The difference would be that > we won't need all the sync_states (i = initialize, d = data is being > copied, f = finished table copy, s = synchronized, r = ready) as we > don't need any synchronization with apply workers. Agreed. > > > > 2) CREATE SUBSCRIPTION -- no syntax change. > > > Upon creation of a subscription, the following additional steps will > > > be managed by the subscriber: > > > i) The subscriber will retrieve the list of sequences associated with > > > the subscription's publications. > > > ii) For each sequence: a) Retrieve the sequence value from the > > > publisher by invoking the pg_sequence_state function. b) Set the > > > sequence with the value obtained from the publisher. iv) Once the > > > subscription creation is completed, all sequence values will become > > > visible at the subscriber's end. > > > > Sequence values are always copied from the publisher? or does it > > happen only when copy_data = true? > > > > It is better to do it when "copy_data = true" to keep it compatible > with the table's behavior. +1 > > > Probably we can > > start with a single worker and extend it to have multiple workers. > > Yeah, starting with a single worker sounds good for now. Do you think > we should sync all the sequences in a single transaction or have some > threshold value above which a different transaction would be required > or maybe a different sequence sync worker altogether? Now, having > multiple sequence-sync workers requires some synchronization so that > only a single worker is allocated for one sequence. > > The simplest thing is to use a single sequence sync worker that syncs > all sequences in one transaction but with a large number of sequences, > it could be inefficient. OTOH, I am not sure if it would be a problem > in reality. I think that we can start with using a single worker and one transaction, and measure the performance with a large number of sequences. > > Or yet another idea I came up with is that a tablesync worker will > > synchronize both the table and sequences owned by the table. That is, > > after the tablesync worker caught up with the apply worker, the > > tablesync worker synchronizes sequences associated with the target > > table as well. One benefit would be that at the time of initial table > > sync being completed, the table and its sequence data are consistent. Correction; it's not guaranteed that the sequence data and table data are consistent even in this case since the tablesync worker could get on-disk sequence data that might have already been updated. > > As soon as new changes come to the table, it would become inconsistent > > so it might not be helpful much, though. Also, sequences that are not > > owned by any table will still need to be synchronized by someone. > > > > The other thing to consider in this idea is that we somehow need to > distinguish the sequences owned by the table. I think we can check pg_depend. The owned sequences reference to the table. > > > > > > > 3) Refreshing the sequence can be achieved through the existing > > > command: ALTER SUBSCRIPTION ... REFRESH PUBLICATION(no syntax change > > > here). > > > The subscriber identifies stale sequences, meaning sequences present > > > in pg_subscription_rel but absent from the publication, and removes > > > them from the pg_subscription_rel system table. The subscriber also > > > checks for newly added sequences in the publisher and synchronizes > > > their values from the publisher using the steps outlined in the > > > subscription creation process. It's worth noting that previously > > > synchronized sequences won't be synchronized again; the sequence sync > > > will occur solely for the newly added sequences. > > > > > > 4) Introducing a new command for refreshing all sequences: ALTER > > > SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES. > > > The subscriber will remove stale sequences and add newly added > > > sequences from the publisher. Following this, it will re-synchronize > > > the sequence values for all sequences in the updated list from the > > > publisher, following the steps outlined in the subscription creation
Re: Logical Replication of sequences
On Thu, Jun 13, 2024 at 1:09 PM Masahiko Sawada wrote: > > On Wed, Jun 12, 2024 at 6:59 PM Amit Kapila wrote: > > > > > > Yeah, starting with a single worker sounds good for now. Do you think > > we should sync all the sequences in a single transaction or have some > > threshold value above which a different transaction would be required > > or maybe a different sequence sync worker altogether? Now, having > > multiple sequence-sync workers requires some synchronization so that > > only a single worker is allocated for one sequence. > > > > The simplest thing is to use a single sequence sync worker that syncs > > all sequences in one transaction but with a large number of sequences, > > it could be inefficient. OTOH, I am not sure if it would be a problem > > in reality. > > I think that we can start with using a single worker and one > transaction, and measure the performance with a large number of > sequences. > Fair enough. However, this raises the question Dilip and Vignesh are discussing whether we need a new relfilenode for sequence update even during initial sync? As per my understanding, the idea is that similar to tables, the CREATE SUBSCRIPTION command (with copy_data = true) will create the new sequence entries in pg_subscription_rel with the state as 'i'. Then the sequence-sync worker would start a transaction and one-by-one copy the latest sequence values for each sequence (that has state as 'i' in pg_subscription_rel) and mark its state as ready 'r' and commit the transaction. Now if there is an error during this operation it will restart the entire operation. The idea of creating a new relfilenode is to handle the error so that if there is a rollback, the sequence state will be rolled back to 'i' and the sequence value will also be rolled back. The other option could be that we update the sequence value without a new relfilenode and if the transaction rolled back then only the sequence's state will be rolled back to 'i'. This would work with a minor inconsistency that sequence values will be up-to-date even when the sequence state is 'i' in pg_subscription_rel. I am not sure if that matters because anyway, they can quickly be out-of-sync with the publisher again. Now, say we don't want to maintain the state of sequences for initial sync at all then after the error how will we detect if there are any pending sequences to be synced? One possibility is that we maintain a subscription level flag 'subsequencesync' in 'pg_subscription' to indicate whether sequences need sync. This flag would indicate whether to sync all the sequences in pg_susbcription_rel. This would mean that if there is an error while syncing the sequences we will resync all the sequences again. This could be acceptable considering the chances of error during sequence sync are low. The benefit is that both the REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same idea and sync all sequences without needing a new relfilenode. Users can always refer 'subsequencesync' flag in 'pg_subscription' to see if all the sequences are synced after executing the command. > > > Or yet another idea I came up with is that a tablesync worker will > > > synchronize both the table and sequences owned by the table. That is, > > > after the tablesync worker caught up with the apply worker, the > > > tablesync worker synchronizes sequences associated with the target > > > table as well. One benefit would be that at the time of initial table > > > sync being completed, the table and its sequence data are consistent. > > Correction; it's not guaranteed that the sequence data and table data > are consistent even in this case since the tablesync worker could get > on-disk sequence data that might have already been updated. > The benefit of this approach is not clear to me. Our aim is to sync all sequences before the upgrade, so not sure if this helps because anyway both table values and corresponding sequences can again be out-of-sync very quickly. > > > > > > > > > > 3) Refreshing the sequence can be achieved through the existing > > > > command: ALTER SUBSCRIPTION ... REFRESH PUBLICATION(no syntax change > > > > here). > > > > The subscriber identifies stale sequences, meaning sequences present > > > > in pg_subscription_rel but absent from the publication, and removes > > > > them from the pg_subscription_rel system table. The subscriber also > > > > checks for newly added sequences in the publisher and synchronizes > > > > their values from the publisher using the steps outlined in the > > > > subscription creation process. It's worth noting that previously > > > > synchronized sequences won't be synchronized again; the sequence sync > > > > will occur solely for the newly added sequences. > > > > > > > > 4) Introducing a new command for refreshing all sequences: ALTER > > > > SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES. > > > > The subscriber will remove stale sequences and add newly added > > > > sequences from the publis
Re: Logical Replication of sequences
On Thu, Jun 13, 2024 at 7:06 PM Amit Kapila wrote:
>
> On Thu, Jun 13, 2024 at 1:09 PM Masahiko Sawada wrote:
> >
> > On Wed, Jun 12, 2024 at 6:59 PM Amit Kapila wrote:
> > >
> > >
> > > Yeah, starting with a single worker sounds good for now. Do you think
> > > we should sync all the sequences in a single transaction or have some
> > > threshold value above which a different transaction would be required
> > > or maybe a different sequence sync worker altogether? Now, having
> > > multiple sequence-sync workers requires some synchronization so that
> > > only a single worker is allocated for one sequence.
> > >
> > > The simplest thing is to use a single sequence sync worker that syncs
> > > all sequences in one transaction but with a large number of sequences,
> > > it could be inefficient. OTOH, I am not sure if it would be a problem
> > > in reality.
> >
> > I think that we can start with using a single worker and one
> > transaction, and measure the performance with a large number of
> > sequences.
> >
>
> Fair enough. However, this raises the question Dilip and Vignesh are
> discussing whether we need a new relfilenode for sequence update even
> during initial sync? As per my understanding, the idea is that similar
> to tables, the CREATE SUBSCRIPTION command (with copy_data = true)
> will create the new sequence entries in pg_subscription_rel with the
> state as 'i'. Then the sequence-sync worker would start a transaction
> and one-by-one copy the latest sequence values for each sequence (that
> has state as 'i' in pg_subscription_rel) and mark its state as ready
> 'r' and commit the transaction. Now if there is an error during this
> operation it will restart the entire operation. The idea of creating a
> new relfilenode is to handle the error so that if there is a rollback,
> the sequence state will be rolled back to 'i' and the sequence value
> will also be rolled back. The other option could be that we update the
> sequence value without a new relfilenode and if the transaction rolled
> back then only the sequence's state will be rolled back to 'i'. This
> would work with a minor inconsistency that sequence values will be
> up-to-date even when the sequence state is 'i' in pg_subscription_rel.
> I am not sure if that matters because anyway, they can quickly be
> out-of-sync with the publisher again.
I think it would be fine in many cases even if the sequence value is
up-to-date even when the sequence state is 'i' in pg_subscription_rel.
But the case we would like to avoid is where suppose the sequence-sync
worker does both synchronizing sequence values and updating the
sequence states for all sequences in one transaction, and if there is
an error we end up retrying the synchronization for all sequences.
>
> Now, say we don't want to maintain the state of sequences for initial
> sync at all then after the error how will we detect if there are any
> pending sequences to be synced? One possibility is that we maintain a
> subscription level flag 'subsequencesync' in 'pg_subscription' to
> indicate whether sequences need sync. This flag would indicate whether
> to sync all the sequences in pg_susbcription_rel. This would mean that
> if there is an error while syncing the sequences we will resync all
> the sequences again. This could be acceptable considering the chances
> of error during sequence sync are low. The benefit is that both the
> REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same
> idea and sync all sequences without needing a new relfilenode. Users
> can always refer 'subsequencesync' flag in 'pg_subscription' to see if
> all the sequences are synced after executing the command.
I think that REFRESH PUBLICATION {SEQUENCES} can be executed even
while the sequence-sync worker is synchronizing sequences. In this
case, the worker might not see new sequences added by the concurrent
REFRESH PUBLICATION {SEQUENCES} command since it's already running.
The worker could end up marking the subsequencesync as completed while
not synchronizing these new sequences.
>
> > > > Or yet another idea I came up with is that a tablesync worker will
> > > > synchronize both the table and sequences owned by the table. That is,
> > > > after the tablesync worker caught up with the apply worker, the
> > > > tablesync worker synchronizes sequences associated with the target
> > > > table as well. One benefit would be that at the time of initial table
> > > > sync being completed, the table and its sequence data are consistent.
> >
> > Correction; it's not guaranteed that the sequence data and table data
> > are consistent even in this case since the tablesync worker could get
> > on-disk sequence data that might have already been updated.
> >
>
> The benefit of this approach is not clear to me. Our aim is to sync
> all sequences before the upgrade, so not sure if this helps because
> anyway both table values and corresponding sequences can again be
> out-of-sync very quickly.
Right.
Giv
Re: Logical Replication of sequences
On Thu, Jun 13, 2024 at 03:36:05PM +0530, Amit Kapila wrote: > Fair enough. However, this raises the question Dilip and Vignesh are > discussing whether we need a new relfilenode for sequence update even > during initial sync? As per my understanding, the idea is that similar > to tables, the CREATE SUBSCRIPTION command (with copy_data = true) > will create the new sequence entries in pg_subscription_rel with the > state as 'i'. Then the sequence-sync worker would start a transaction > and one-by-one copy the latest sequence values for each sequence (that > has state as 'i' in pg_subscription_rel) and mark its state as ready > 'r' and commit the transaction. Now if there is an error during this > operation it will restart the entire operation. Hmm. You mean to use only one transaction for all the sequences? I've heard about deployments with a lot of them. Could it be a problem to process them in batches, as well? If you maintain a state for each one of them in pg_subscription_rel, it does not strike me as an issue, while being more flexible than an all-or-nothing. > The idea of creating a > new relfilenode is to handle the error so that if there is a rollback, > the sequence state will be rolled back to 'i' and the sequence value > will also be rolled back. The other option could be that we update the > sequence value without a new relfilenode and if the transaction rolled > back then only the sequence's state will be rolled back to 'i'. This > would work with a minor inconsistency that sequence values will be > up-to-date even when the sequence state is 'i' in pg_subscription_rel. > I am not sure if that matters because anyway, they can quickly be > out-of-sync with the publisher again. Seeing a mention to relfilenodes specifically for sequences freaks me out a bit, because there's some work I have been doing in this area and sequences may not have a need for a physical relfilenode at all. But I guess that you refer to the fact that like tables, relfilenodes would only be created as required because anything you'd do in the apply worker path would just call some of the routines of sequence.h, right? > Now, say we don't want to maintain the state of sequences for initial > sync at all then after the error how will we detect if there are any > pending sequences to be synced? One possibility is that we maintain a > subscription level flag 'subsequencesync' in 'pg_subscription' to > indicate whether sequences need sync. This flag would indicate whether > to sync all the sequences in pg_susbcription_rel. This would mean that > if there is an error while syncing the sequences we will resync all > the sequences again. This could be acceptable considering the chances > of error during sequence sync are low. There could be multiple subscriptions to a single database that point to the same set of sequences. Is there any conflict issue to worry about here? > The benefit is that both the > REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same > idea and sync all sequences without needing a new relfilenode. Users > can always refer 'subsequencesync' flag in 'pg_subscription' to see if > all the sequences are synced after executing the command. That would be cheaper, indeed. Isn't a boolean too limiting? Isn't that something you'd want to track with a LSN as "the point in WAL where all the sequences have been synced"? The approach of doing all the sync work from the subscriber, while having a command that can be kicked from the subscriber side is a good user experience. -- Michael signature.asc Description: PGP signature
Re: Logical Replication of sequences
On Thu, Jun 13, 2024 at 6:14 PM Masahiko Sawada wrote:
>
> On Thu, Jun 13, 2024 at 7:06 PM Amit Kapila wrote:
> >
> > On Thu, Jun 13, 2024 at 1:09 PM Masahiko Sawada
> > wrote:
> > >
> > > On Wed, Jun 12, 2024 at 6:59 PM Amit Kapila
> > > wrote:
> > > >
> > > >
> > > > Yeah, starting with a single worker sounds good for now. Do you think
> > > > we should sync all the sequences in a single transaction or have some
> > > > threshold value above which a different transaction would be required
> > > > or maybe a different sequence sync worker altogether? Now, having
> > > > multiple sequence-sync workers requires some synchronization so that
> > > > only a single worker is allocated for one sequence.
> > > >
> > > > The simplest thing is to use a single sequence sync worker that syncs
> > > > all sequences in one transaction but with a large number of sequences,
> > > > it could be inefficient. OTOH, I am not sure if it would be a problem
> > > > in reality.
> > >
> > > I think that we can start with using a single worker and one
> > > transaction, and measure the performance with a large number of
> > > sequences.
> > >
> >
> > Fair enough. However, this raises the question Dilip and Vignesh are
> > discussing whether we need a new relfilenode for sequence update even
> > during initial sync? As per my understanding, the idea is that similar
> > to tables, the CREATE SUBSCRIPTION command (with copy_data = true)
> > will create the new sequence entries in pg_subscription_rel with the
> > state as 'i'. Then the sequence-sync worker would start a transaction
> > and one-by-one copy the latest sequence values for each sequence (that
> > has state as 'i' in pg_subscription_rel) and mark its state as ready
> > 'r' and commit the transaction. Now if there is an error during this
> > operation it will restart the entire operation. The idea of creating a
> > new relfilenode is to handle the error so that if there is a rollback,
> > the sequence state will be rolled back to 'i' and the sequence value
> > will also be rolled back. The other option could be that we update the
> > sequence value without a new relfilenode and if the transaction rolled
> > back then only the sequence's state will be rolled back to 'i'. This
> > would work with a minor inconsistency that sequence values will be
> > up-to-date even when the sequence state is 'i' in pg_subscription_rel.
> > I am not sure if that matters because anyway, they can quickly be
> > out-of-sync with the publisher again.
>
> I think it would be fine in many cases even if the sequence value is
> up-to-date even when the sequence state is 'i' in pg_subscription_rel.
> But the case we would like to avoid is where suppose the sequence-sync
> worker does both synchronizing sequence values and updating the
> sequence states for all sequences in one transaction, and if there is
> an error we end up retrying the synchronization for all sequences.
>
The one idea to avoid this is to update sequences in chunks (say 100
or some threshold number of sequences in one transaction). Then we
would only redo the sync for the last and pending set of sequences.
> >
> > Now, say we don't want to maintain the state of sequences for initial
> > sync at all then after the error how will we detect if there are any
> > pending sequences to be synced? One possibility is that we maintain a
> > subscription level flag 'subsequencesync' in 'pg_subscription' to
> > indicate whether sequences need sync. This flag would indicate whether
> > to sync all the sequences in pg_susbcription_rel. This would mean that
> > if there is an error while syncing the sequences we will resync all
> > the sequences again. This could be acceptable considering the chances
> > of error during sequence sync are low. The benefit is that both the
> > REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same
> > idea and sync all sequences without needing a new relfilenode. Users
> > can always refer 'subsequencesync' flag in 'pg_subscription' to see if
> > all the sequences are synced after executing the command.
>
> I think that REFRESH PUBLICATION {SEQUENCES} can be executed even
> while the sequence-sync worker is synchronizing sequences. In this
> case, the worker might not see new sequences added by the concurrent
> REFRESH PUBLICATION {SEQUENCES} command since it's already running.
> The worker could end up marking the subsequencesync as completed while
> not synchronizing these new sequences.
>
This is possible but we could avoid REFRESH PUBLICATION {SEQUENCES} by
not allowing to change the subsequencestate during the time
sequence-worker is syncing the sequences. This could be restrictive
but there doesn't seem to be cases where user would like to
immediately refresh sequences after creating the subscription.
> >
> > > > > Or yet another idea I came up with is that a tablesync worker will
> > > > > synchronize both the table and sequences owned by the table. That is,
> > > > > after the table
Re: Logical Replication of sequences
On Fri, Jun 14, 2024 at 5:16 AM Michael Paquier wrote: > > On Thu, Jun 13, 2024 at 03:36:05PM +0530, Amit Kapila wrote: > > Fair enough. However, this raises the question Dilip and Vignesh are > > discussing whether we need a new relfilenode for sequence update even > > during initial sync? As per my understanding, the idea is that similar > > to tables, the CREATE SUBSCRIPTION command (with copy_data = true) > > will create the new sequence entries in pg_subscription_rel with the > > state as 'i'. Then the sequence-sync worker would start a transaction > > and one-by-one copy the latest sequence values for each sequence (that > > has state as 'i' in pg_subscription_rel) and mark its state as ready > > 'r' and commit the transaction. Now if there is an error during this > > operation it will restart the entire operation. > > Hmm. You mean to use only one transaction for all the sequences? > I've heard about deployments with a lot of them. Could it be a > problem to process them in batches, as well? I don't think so. We can even sync one sequence per transaction but then it would be resource and time consuming without much gain. As mentioned in a previous email, we might want to sync 100 or some other threshold number of sequences per transaction. The other possibility is to make a subscription-level option for this batch size but I don't see much advantage in doing so as it won't be convenient for users to set it. I feel we should pick some threshold number that is neither too low nor too high and if we later see any problem with it, we can make it a configurable knob. > > > The idea of creating a > > new relfilenode is to handle the error so that if there is a rollback, > > the sequence state will be rolled back to 'i' and the sequence value > > will also be rolled back. The other option could be that we update the > > sequence value without a new relfilenode and if the transaction rolled > > back then only the sequence's state will be rolled back to 'i'. This > > would work with a minor inconsistency that sequence values will be > > up-to-date even when the sequence state is 'i' in pg_subscription_rel. > > I am not sure if that matters because anyway, they can quickly be > > out-of-sync with the publisher again. > > Seeing a mention to relfilenodes specifically for sequences freaks me > out a bit, because there's some work I have been doing in this area > and sequences may not have a need for a physical relfilenode at all. > But I guess that you refer to the fact that like tables, relfilenodes > would only be created as required because anything you'd do in the > apply worker path would just call some of the routines of sequence.h, > right? > Yes, I think so. The only thing the patch expects is a way to rollback the sequence changes if the transaction rolls back during the initial sync. But I am not sure if we need such a behavior. The discussion for the same is in progress. Let's wait for the outcome. > > Now, say we don't want to maintain the state of sequences for initial > > sync at all then after the error how will we detect if there are any > > pending sequences to be synced? One possibility is that we maintain a > > subscription level flag 'subsequencesync' in 'pg_subscription' to > > indicate whether sequences need sync. This flag would indicate whether > > to sync all the sequences in pg_susbcription_rel. This would mean that > > if there is an error while syncing the sequences we will resync all > > the sequences again. This could be acceptable considering the chances > > of error during sequence sync are low. > > There could be multiple subscriptions to a single database that point > to the same set of sequences. Is there any conflict issue to worry > about here? > I don't think so. In the worst case, the same value would be copied twice. The same scenario in case of tables could lead to duplicate data or unique key violation ERRORs which is much worse. So, I expect users to be careful about the same. > > The benefit is that both the > > REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same > > idea and sync all sequences without needing a new relfilenode. Users > > can always refer 'subsequencesync' flag in 'pg_subscription' to see if > > all the sequences are synced after executing the command. > > That would be cheaper, indeed. Isn't a boolean too limiting? > In this idea, we only need a flag to say whether the sequence sync is required or not. > Isn't that something you'd want to track with a LSN as "the point in > WAL where all the sequences have been synced"? > It won't be any better for the required purpose because after CREATE SUBSCRIPTION, if REFERESH wants to toggle the flag to indicate the sequences need sync again then using LSN would mean we need to set it to Invalid value. > The approach of doing all the sync work from the subscriber, while > having a command that can be kicked from the subscriber side is a good > user experience. > Thank you for
Re: Logical Replication of sequences
On Fri, Jun 14, 2024 at 4:04 PM Amit Kapila wrote:
>
> On Thu, Jun 13, 2024 at 6:14 PM Masahiko Sawada wrote:
> >
> > On Thu, Jun 13, 2024 at 7:06 PM Amit Kapila wrote:
> > >
> > > On Thu, Jun 13, 2024 at 1:09 PM Masahiko Sawada
> > > wrote:
> > > >
> > > > On Wed, Jun 12, 2024 at 6:59 PM Amit Kapila
> > > > wrote:
> > > > >
> > > > >
> > > > > Yeah, starting with a single worker sounds good for now. Do you think
> > > > > we should sync all the sequences in a single transaction or have some
> > > > > threshold value above which a different transaction would be required
> > > > > or maybe a different sequence sync worker altogether? Now, having
> > > > > multiple sequence-sync workers requires some synchronization so that
> > > > > only a single worker is allocated for one sequence.
> > > > >
> > > > > The simplest thing is to use a single sequence sync worker that syncs
> > > > > all sequences in one transaction but with a large number of sequences,
> > > > > it could be inefficient. OTOH, I am not sure if it would be a problem
> > > > > in reality.
> > > >
> > > > I think that we can start with using a single worker and one
> > > > transaction, and measure the performance with a large number of
> > > > sequences.
> > > >
> > >
> > > Fair enough. However, this raises the question Dilip and Vignesh are
> > > discussing whether we need a new relfilenode for sequence update even
> > > during initial sync? As per my understanding, the idea is that similar
> > > to tables, the CREATE SUBSCRIPTION command (with copy_data = true)
> > > will create the new sequence entries in pg_subscription_rel with the
> > > state as 'i'. Then the sequence-sync worker would start a transaction
> > > and one-by-one copy the latest sequence values for each sequence (that
> > > has state as 'i' in pg_subscription_rel) and mark its state as ready
> > > 'r' and commit the transaction. Now if there is an error during this
> > > operation it will restart the entire operation. The idea of creating a
> > > new relfilenode is to handle the error so that if there is a rollback,
> > > the sequence state will be rolled back to 'i' and the sequence value
> > > will also be rolled back. The other option could be that we update the
> > > sequence value without a new relfilenode and if the transaction rolled
> > > back then only the sequence's state will be rolled back to 'i'. This
> > > would work with a minor inconsistency that sequence values will be
> > > up-to-date even when the sequence state is 'i' in pg_subscription_rel.
> > > I am not sure if that matters because anyway, they can quickly be
> > > out-of-sync with the publisher again.
> >
> > I think it would be fine in many cases even if the sequence value is
> > up-to-date even when the sequence state is 'i' in pg_subscription_rel.
> > But the case we would like to avoid is where suppose the sequence-sync
> > worker does both synchronizing sequence values and updating the
> > sequence states for all sequences in one transaction, and if there is
> > an error we end up retrying the synchronization for all sequences.
> >
>
> The one idea to avoid this is to update sequences in chunks (say 100
> or some threshold number of sequences in one transaction). Then we
> would only redo the sync for the last and pending set of sequences.
That could be one idea.
>
> > >
> > > Now, say we don't want to maintain the state of sequences for initial
> > > sync at all then after the error how will we detect if there are any
> > > pending sequences to be synced? One possibility is that we maintain a
> > > subscription level flag 'subsequencesync' in 'pg_subscription' to
> > > indicate whether sequences need sync. This flag would indicate whether
> > > to sync all the sequences in pg_susbcription_rel. This would mean that
> > > if there is an error while syncing the sequences we will resync all
> > > the sequences again. This could be acceptable considering the chances
> > > of error during sequence sync are low. The benefit is that both the
> > > REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same
> > > idea and sync all sequences without needing a new relfilenode. Users
> > > can always refer 'subsequencesync' flag in 'pg_subscription' to see if
> > > all the sequences are synced after executing the command.
> >
> > I think that REFRESH PUBLICATION {SEQUENCES} can be executed even
> > while the sequence-sync worker is synchronizing sequences. In this
> > case, the worker might not see new sequences added by the concurrent
> > REFRESH PUBLICATION {SEQUENCES} command since it's already running.
> > The worker could end up marking the subsequencesync as completed while
> > not synchronizing these new sequences.
> >
>
> This is possible but we could avoid REFRESH PUBLICATION {SEQUENCES} by
> not allowing to change the subsequencestate during the time
> sequence-worker is syncing the sequences. This could be restrictive
> but there doesn't seem to be cases where user would like to
Re: Logical Replication of sequences
On Tue, Jun 18, 2024 at 7:30 AM Masahiko Sawada wrote:
>
> On Fri, Jun 14, 2024 at 4:04 PM Amit Kapila wrote:
> >
> > On Thu, Jun 13, 2024 at 6:14 PM Masahiko Sawada
> > wrote:
> > >
> >
> > > >
> > > > Now, say we don't want to maintain the state of sequences for initial
> > > > sync at all then after the error how will we detect if there are any
> > > > pending sequences to be synced? One possibility is that we maintain a
> > > > subscription level flag 'subsequencesync' in 'pg_subscription' to
> > > > indicate whether sequences need sync. This flag would indicate whether
> > > > to sync all the sequences in pg_susbcription_rel. This would mean that
> > > > if there is an error while syncing the sequences we will resync all
> > > > the sequences again. This could be acceptable considering the chances
> > > > of error during sequence sync are low. The benefit is that both the
> > > > REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same
> > > > idea and sync all sequences without needing a new relfilenode. Users
> > > > can always refer 'subsequencesync' flag in 'pg_subscription' to see if
> > > > all the sequences are synced after executing the command.
> > >
> > > I think that REFRESH PUBLICATION {SEQUENCES} can be executed even
> > > while the sequence-sync worker is synchronizing sequences. In this
> > > case, the worker might not see new sequences added by the concurrent
> > > REFRESH PUBLICATION {SEQUENCES} command since it's already running.
> > > The worker could end up marking the subsequencesync as completed while
> > > not synchronizing these new sequences.
> > >
> >
> > This is possible but we could avoid REFRESH PUBLICATION {SEQUENCES} by
> > not allowing to change the subsequencestate during the time
> > sequence-worker is syncing the sequences. This could be restrictive
> > but there doesn't seem to be cases where user would like to
> > immediately refresh sequences after creating the subscription.
>
> I'm concerned that users would not be able to add sequences during the
> time the sequence-worker is syncing the sequences. For example,
> suppose we have 1 sequences and execute REFRESH PUBLICATION
> {SEQUENCES} to synchronize 1 sequences. Now if we add one sequence
> to the publication and want to synchronize it to the subscriber, we
> have to wait for the current REFRESH PUBLICATION {SEQUENCES} to
> complete, and then execute it again, synchronizing 10001 sequences,
> instead of synchronizing only the new one.
>
I see your point and it could hurt such scenarios even though they
won't be frequent. So, let's focus on our other approach of
maintaining the flag at a per-sequence level in pg_subscription_rel.
> >
> > > >
> > > > > > > Or yet another idea I came up with is that a tablesync worker will
> > > > > > > synchronize both the table and sequences owned by the table. That
> > > > > > > is,
> > > > > > > after the tablesync worker caught up with the apply worker, the
> > > > > > > tablesync worker synchronizes sequences associated with the target
> > > > > > > table as well. One benefit would be that at the time of initial
> > > > > > > table
> > > > > > > sync being completed, the table and its sequence data are
> > > > > > > consistent.
> > > > >
> > > > > Correction; it's not guaranteed that the sequence data and table data
> > > > > are consistent even in this case since the tablesync worker could get
> > > > > on-disk sequence data that might have already been updated.
> > > > >
> > > >
> > > > The benefit of this approach is not clear to me. Our aim is to sync
> > > > all sequences before the upgrade, so not sure if this helps because
> > > > anyway both table values and corresponding sequences can again be
> > > > out-of-sync very quickly.
> > >
> > > Right.
> > >
> > > Given that our aim is to sync all sequences before the upgrade, do we
> > > need to synchronize sequences even at CREATE SUBSCRIPTION time? In
> > > cases where there are a large number of sequences, synchronizing
> > > sequences in addition to tables could be overhead and make less sense,
> > > because sequences can again be out-of-sync quickly and typically
> > > CREATE SUBSCRIPTION is not created just before the upgrade.
> > >
> >
> > I think for the upgrade one should be creating a subscription just
> > before the upgrade. Isn't something similar is done even in the
> > upgrade steps you shared once [1]?
>
> I might be missing something but in the blog post they created
> subscriptions in various ways, waited for the initial table data sync
> to complete, and then set the sequence values with a buffer based on
> the old cluster. What I imagined with this sequence synchronization
> feature is that after the initial table sync completes, we stop to
> execute further transactions on the publisher, synchronize sequences
> using REFRESH PUBLICATION {SEQUENCES}, and resume the application to
> execute transactions on the subscriber. So a subscription would be
> created just before the
Re: Logical Replication of sequences
On Wed, Jun 19, 2024 at 8:33 PM vignesh C wrote: > > On Tue, 18 Jun 2024 at 16:10, Amit Kapila wrote: > > > > > > Agreed and I am not sure which is better because there is a value in > > keeping the state name the same for both sequences and tables. We > > probably need more comments in code and doc updates to make the > > behavior clear. We can start with the sequence state as 'init' for > > 'needs-to-be-sycned' and 'ready' for 'synced' and can change if others > > feel so during the review. > > Here is a patch which does the sequence synchronization in the > following lines from the above discussion: > Thanks for summarizing the points discussed. I would like to confirm whether the patch replicates new sequences that are created implicitly/explicitly for a publication defined as ALL SEQUENCES. -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Thu, 20 Jun 2024 at 18:45, Amit Kapila wrote: > > On Wed, Jun 19, 2024 at 8:33 PM vignesh C wrote: > > > > On Tue, 18 Jun 2024 at 16:10, Amit Kapila wrote: > > > > > > > > > Agreed and I am not sure which is better because there is a value in > > > keeping the state name the same for both sequences and tables. We > > > probably need more comments in code and doc updates to make the > > > behavior clear. We can start with the sequence state as 'init' for > > > 'needs-to-be-sycned' and 'ready' for 'synced' and can change if others > > > feel so during the review. > > > > Here is a patch which does the sequence synchronization in the > > following lines from the above discussion: > > > > Thanks for summarizing the points discussed. I would like to confirm > whether the patch replicates new sequences that are created > implicitly/explicitly for a publication defined as ALL SEQUENCES. Currently, FOR ALL SEQUENCES publication both explicitly created sequences and implicitly created sequences will be synchronized during the creation of subscriptions (using CREATE SUBSCRIPTION) and refreshing publication sequences(using ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES). Therefore, the explicitly created sequence seq1: CREATE SEQUENCE seq1; and the implicitly created sequence seq_test2_c2_seq for seq_test2 table: CREATE TABLE seq_test2 (c1 int, c2 SERIAL); will both be synchronized. Regards, Vignesh
Re: Logical Replication of sequences
On Thu, 20 Jun 2024 at 18:24, vignesh C wrote:
>
> On Wed, 19 Jun 2024 at 20:33, vignesh C wrote:
> >
> > On Tue, 18 Jun 2024 at 16:10, Amit Kapila wrote:
> > >
> > >
> > > Agreed and I am not sure which is better because there is a value in
> > > keeping the state name the same for both sequences and tables. We
> > > probably need more comments in code and doc updates to make the
> > > behavior clear. We can start with the sequence state as 'init' for
> > > 'needs-to-be-sycned' and 'ready' for 'synced' and can change if others
> > > feel so during the review.
> >
> > Here is a patch which does the sequence synchronization in the
> > following lines from the above discussion:
> > This commit introduces sequence synchronization during 1) creation of
> > subscription for initial sync of sequences 2) refresh publication to
> > synchronize the sequences for the newly created sequences 3) refresh
> > publication sequences for synchronizing all the sequences.
> > 1) During subscription creation with CREATE SUBSCRIPTION (no syntax change):
> >- The subscriber retrieves sequences associated with publications.
> >- Sequences are added in the 'init' state to the pg_subscription_rel
> > table.
> >- Sequence synchronization worker will be started if there are any
> > sequences to be synchronized
> >- A new sequence synchronization worker handles synchronization in
> > batches of 100 sequences:
> > a) Retrieves sequence values using pg_sequence_state from the
> > publisher.
> > b) Sets sequence values accordingly.
> > c) Updates sequence state to 'READY' in pg_susbcripion_rel
> > d) Commits batches of 100 synchronized sequences.
> > 2) Refreshing sequences with ALTER SUBSCRIPTION ... REFRESH
> > PUBLICATION (no syntax change):
> >- Stale sequences are removed from pg_subscription_rel.
> >- Newly added sequences in the publisher are added in 'init' state
> > to pg_subscription_rel.
> >- Sequence synchronization will be done by sequence sync worker as
> > listed in subscription creation process.
> >- Sequence synchronization occurs for newly added sequences only.
> > 3) Introduce new command ALTER SUBSCRIPTION ... REFRESH PUBLICATION
> > SEQUENCES for refreshing all sequences:
> >- Removes stale sequences and adds newly added sequences from the
> > publisher to pg_subscription_rel.
> >- Resets all sequences in pg_subscription_rel to 'init' state.
> >- Initiates sequence synchronization for all sequences by sequence
> > sync worker as listed in subscription creation process.
>
> Here is an updated patch with a few fixes to remove an unused
> function, changed a few references of table to sequence and added one
> CHECK_FOR_INTERRUPTS in the sequence sync worker loop.
Hi Vignesh,
I have reviewed the patches and I have following comments:
= tablesync.c ==
1. process_syncing_sequences_for_apply can crash with:
2024-06-21 15:25:17.208 IST [3681269] LOG: logical replication apply
worker for subscription "test1" has started
2024-06-21 15:28:10.127 IST [3682329] LOG: logical replication
sequences synchronization worker for subscription "test1" has started
2024-06-21 15:28:10.146 IST [3682329] LOG: logical replication
synchronization for subscription "test1", sequence "s1" has finished
2024-06-21 15:28:10.149 IST [3682329] LOG: logical replication
synchronization for subscription "test1", sequence "s2" has finished
2024-06-21 15:28:10.149 IST [3682329] LOG: logical replication
sequences synchronization worker for subscription "test1" has finished
2024-06-21 15:29:53.535 IST [3682767] LOG: logical replication
sequences synchronization worker for subscription "test1" has started
TRAP: failed Assert("nestLevel > 0 && (nestLevel <= GUCNestLevel ||
(nestLevel == GUCNestLevel + 1 && !isCommit))"), File: "guc.c", Line:
2273, PID: 3682767
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (ExceptionalCondition+0xbb)[0x5b2a61861c99]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (AtEOXact_GUC+0x7b)[0x5b2a618bddfa]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (RestoreUserContext+0xc7)[0x5b2a618a6937]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (+0x1ff7dfa)[0x5b2a61115dfa]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (+0x1ff7eb4)[0x5b2a61115eb4]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (SequencesyncWorkerMain+0x33)[0x5b2a61115fe7]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (BackgroundWorkerMain+0x4ad)[0x5b2a61029cae]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (postmaster_child_launch+0x236)[0x5b2a6102fb36]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (+0x1f1d12a)[0x5b2a6103b12a]
postgres: logical replication sequencesync worker for subscription
Re: Logical Replication of sequences
Here are my initial review comments for the first patch v20240625-0001.
==
General
1. Missing docs?
Section 9.17. "Sequence Manipulation Functions" [1] describes some
functions. Shouldn't your new function be documented here also?
~~~
2. Missing tests?
Shouldn't there be some test code that at least executes your new
pg_sequence_state function to verify that sane values are returned?
==
Commit Message
3.
This patch introduces new functionalities to PostgreSQL:
- pg_sequence_state allows retrieval of sequence values using LSN.
- SetSequence enables updating sequences with user-specified values.
~
3a.
I didn't understand why this says "using LSN" because IIUC 'lsn' is an
output parameter of that function. Don't you mean "... retrieval of
sequence values including LSN"?
~
3b.
Does "user-specified" make sense? Is this going to be exposed to a
user? How about just "specified"?
==
src/backend/commands/sequence.c
4. SetSequence:
+void
+SetSequence(Oid seq_relid, int64 value)
Would 'new_last_value' be a better parameter name here?
~~~
5.
This new function logic looks pretty similar to the do_setval()
function. Can you explain (maybe in the function comment) some info
about how and why it differs from that other function?
~~~
6.
I saw that RelationNeedsWAL() is called 2 times. It may make no sense,
but is it possible to assign that to a variable 1st time so you don't
need to call it 2nd time within the critical section?
~~~
NITPICK - remove junk (') char in comment
NITPICK - missing periods (.) in multi-sentence comment
~~~
7.
-read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple)
+read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple,
+XLogRecPtr *lsn)
7a.
The existing parameters were described in the function comment. So,
the new 'lsn' parameter should be described here also.
~
7b.
Maybe the new parameter name should be 'lsn_res' or 'lsn_out' or
similar to emphasise that this is a returned value.
~~
NITPICK - tweaked comment. YMMV.
~~~
8. pg_sequence_state:
Should you give descriptions of the output parameters in the function
header comment? Otherwise, where are they described so called knows
what they mean?
~~~
NITPICK - /relid/seq_relid/
NITPICK - declare the variables in the same order as the output parameters
NITPICK - An alternative to the memset for nulls is just to use static
initialisation
"bool nulls[4] = {false, false, false, false};"
==
+extern void SetSequence(Oid seq_relid, int64 value);
9.
Would 'SetSequenceLastValue' be a better name for what this function is doing?
==
99.
See also my attached diff which is a top-up patch implementing those
nitpicks mentioned above. Please apply any of these that you agree
with.
==
[1] https://www.postgresql.org/docs/devel/functions-sequence.html
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 57453a7..9bad121 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -349,7 +349,7 @@ SetSequence(Oid seq_relid, int64 value)
/* open and lock sequence */
init_sequence(seq_relid, &elm, &seqrel);
- /* lock page' buffer and read tuple */
+ /* lock page buffer and read tuple */
seq = read_seq_tuple(seqrel, &buf, &seqdatatuple, NULL);
/* check the comment above nextval_internal()'s equivalent call. */
@@ -397,8 +397,10 @@ SetSequence(Oid seq_relid, int64 value)
UnlockReleaseBuffer(buf);
- /* Clear local cache so that we don't think we have cached numbers */
- /* Note that we do not change the currval() state */
+ /*
+* Clear local cache so that we don't think we have cached numbers.
+* Note that we do not change the currval() state.
+*/
elm->cached = elm->last;
relation_close(seqrel, NoLock);
@@ -1275,8 +1277,9 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple
seqdatatuple,
RelationGetRelationName(rel), sm->magic);
/*
-* If the caller requested it, set the page LSN. This allows deciding
-* which sequence changes are before/after the returned sequence state.
+* If the caller requested it, return the page LSN. This allows the
+* caller to determine which sequence changes are before/after the
+* returned sequence state.
*/
if (lsn)
*lsn = PageGetLSN(page);
@@ -1912,7 +1915,7 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Datum
pg_sequence_state(PG_FUNCTION_ARGS)
{
- Oid relid = PG_GETARG_OID(0);
+ Oid seq_relid = PG_GETARG_OID(0);
SeqTableelm;
Relationseqrel;
Buffer buf;
@@ -1920,21 +1923,21 @@ pg_sequence_state(PG_FUNCTION_ARGS)
Form_pg_sequence_data seq;
Datum result;
+ XLogRecPtr lsn;
int64
Re: Logical Replication of sequences
Here are some review comments for the patch v20240625-0002
==
Commit Message
1.
This commit enhances logical replication by enabling the inclusion of all
sequences in publications. This improvement facilitates seamless
synchronization of sequence data during operations such as
CREATE SUBSCRIPTION, REFRESH PUBLICATION, and REFRESH PUBLICATION SEQUENCES.
~
Isn't this description getting ahead of the functionality a bit? For
example, it talks about operations like REFRESH PUBLICATION SEQUENCES
but AFAIK that syntax does not exist just yet.
~~~
2.
The commit message should mention that you are only introducing new
syntax for "FOR ALL SEQUENCES" here, but syntax for "FOR SEQUENCE" is
being deferred to some later patch. Without such a note it is not
clear why the gram.y syntax and docs seemed only half done.
==
doc/src/sgml/ref/create_publication.sgml
3.
FOR ALL TABLES
+FOR ALL SEQUENCES
- Marks the publication as one that replicates changes for all tables in
- the database, including tables created in the future.
+ Marks the publication as one that replicates changes for all tables or
+ sequences in the database, including tables created in the future.
It might be better here to keep descriptions for "ALL TABLES" and "ALL
SEQUENCES" separated, otherwise the wording does not quite seem
appropriate for sequences (e.g. where it says "including tables
created in the future").
~~~
NITPICK - missing spaces
NITPICK - removed Oxford commas since previously there were none
~~~
4.
+ If FOR TABLE, FOR ALL TABLES,
+ FOR ALL SEQUENCES,or FOR TABLES IN
SCHEMA
+ are not specified, then the publication starts out with an empty set of
+ tables. That is useful if tables or schemas are to be added later.
It seems like "FOR ALL SEQUENCES" is out of place since it is jammed
between other clauses referring to TABLES. Would it be better to
mention SEQUENCES last in the list?
~~~
5.
+ rights on the table. The FOR ALL TABLES,
+ FOR ALL SEQUENCES, and
FOR TABLES IN SCHEMA clauses require the invoking
ditto of #4 above.
==
src/backend/catalog/pg_publication.c
GetAllSequencesPublicationRelations:
NITPICK - typo /relation/relations/
==
src/backend/commands/publicationcmds.c
6.
+ foreach(lc, stmt->for_all_objects)
+ {
+ char*val = strVal(lfirst(lc));
+
+ if (strcmp(val, "tables") == 0)
+ for_all_tables = true;
+ else if (strcmp(val, "sequences") == 0)
+ for_all_sequences = true;
+ }
Consider the foreach_ptr macro to slightly simplify this code.
Actually, this whole logic seems cumbersome -- can’t the parser assign
flags automatically. Please see my more detailed comment #10 below
about this in gram.y
~~~
7.
/* FOR ALL TABLES requires superuser */
- if (stmt->for_all_tables && !superuser())
+ if (for_all_tables && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to create FOR ALL TABLES publication")));
+ /* FOR ALL SEQUENCES requires superuser */
+ if (for_all_sequences && !superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be superuser to create FOR ALL SEQUENCES publication")));
+
The current code is easy to read, but I wonder if it should try harder
to share common code, or at least a common translatable message like
"must be superuser to create %s publication".
~~~
8.
- else
+
+ /*
+ * If the publication might have either tables or sequences (directly or
+ * through a schema), process that.
+ */
+ if (!for_all_tables || !for_all_sequences)
I did not understand why this code cannot just say "else" like before,
because the direct or through-schema syntax cannot be specified at the
same time as "FOR ALL ...", so why is the more complicated condition
necessary? Also, the similar code in AlterPublicationOptions() was not
changed to be like this.
==
src/backend/parser/gram.y
9. comment
*
* CREATE PUBLICATION FOR ALL TABLES [WITH options]
*
+ * CREATE PUBLICATION FOR ALL SEQUENCES [WITH options]
+ *
* CREATE PUBLICATION FOR pub_obj [, ...] [WITH options]
The comment is not quite correct because actually you are allowing
simultaneous FOR ALL TABLES, SEQUENCES. It should be more like:
CREATE PUBLICATION FOR ALL pub_obj_type [,...] [WITH options]
pub_obj_type is one of:
TABLES
SEQUENCES
~~~
10.
+pub_obj_type: TABLES
+ { $$ = (Node *) makeString("tables"); }
+ | SEQUENCES
+ { $$ = (Node *) makeString("sequences"); }
+ ;
+
+pub_obj_type_list: pub_obj_type
+ { $$ = list_make1($1); }
+ | pub_obj_type_list ',' pub_obj_type
+ { $$ = lappend($1, $3); }
+ ;
IIUC the only thing you need is a flag to say if FOR ALL TABLE is in
effect and another flag to say if FOR ALL SEQUENCES is in effect. So,
It seemed clunky to build up a temporary list of "tables" or
"sequences" strings here, which is subsequently scanned by
CreatePublication to be turned back into booleans.
Can't we just change the CreatePublicationStmt field to have:
A) a
Re: Logical Replication of sequences
Here are my comments for patch v20240702-0001
They are all cosmetic and/or typos. Apart from these the 0001 patch LGTM.
==
doc/src/sgml/func.sgml
Section 9.17. Sequence Manipulation Functions
pg_sequence_state:
nitpick - typo /whethere/whether/
nitpick - reworded slightly using a ChatGPT suggestion. (YMMV, so it
is fine also if you prefer the current wording)
==
src/backend/commands/sequence.c
SetSequenceLastValue:
nitpick - typo in function comment /diffrent/different/
pg_sequence_state:
nitpick - function comment wording: /page LSN/the page LSN/
nitpick - moved some comment details about 'lsn_ret' into the function header
nitpick - rearranged variable assignments to have consistent order
with the values
nitpick - tweaked comments
nitpick - typo /whethere/whether/
==
99.
Please see the attached diffs patch which implements all those
nitpicks mentioned above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 3dd4805..83f87c1 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19521,11 +19521,11 @@ SELECT setval('myseq', 42, false);
Next nextval
Returns information about the sequence. lsn is the
-page lsn of the sequence, last_value is the value
-most recently returned by nextval in the current
+page lsn of the sequence, last_value is the most
+recent value returned by nextval in the current
session, log_cnt shows how many fetches remain
-before a new WAL record has to be written and
-is_called indicates whethere the sequence has been
+before a new WAL record has to be written, and
+is_called indicates whether the sequence has been
used.
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index cd7639f..f33f689 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -338,7 +338,7 @@ ResetSequence(Oid seq_relid)
* responsible for permissions checking.
*
* Note: This function resembles do_setval but does not include the locking and
- * verification steps, as those are managed in a slightly diffrent manner for
+ * verification steps, as those are managed in a slightly different manner for
* logical replication.
*/
void
@@ -1262,7 +1262,9 @@ init_sequence(Oid relid, SeqTable *p_elm, Relation *p_rel)
* *buf receives the reference to the pinned-and-ex-locked buffer
* *seqdatatuple receives the reference to the sequence tuple proper
* (this arg should point to a local variable of type
HeapTupleData)
- * *lsn_ret will be set to page LSN if the caller requested it
+ * *lsn_ret will be set to the page LSN if the caller requested it.
+ * This allows the caller to determine which sequence changes are
+ * before/after the returned sequence state.
*
* Function's return value points to the data payload of the tuple
*/
@@ -1285,11 +1287,7 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple
seqdatatuple,
elog(ERROR, "bad magic number in sequence \"%s\": %08X",
RelationGetRelationName(rel), sm->magic);
- /*
-* If the caller requested it, return the page LSN. This allows the
-* caller to determine which sequence changes are before/after the
-* returned sequence state.
-*/
+ /* If the caller requested it, return the page LSN. */
if (lsn_ret)
*lsn_ret = PageGetLSN(page);
@@ -1957,9 +1955,9 @@ pg_sequence_state(PG_FUNCTION_ARGS)
seq = read_seq_tuple(seqrel, &buf, &seqtuple, &lsn);
- is_called = seq->is_called;
last_value = seq->last_value;
log_cnt = seq->log_cnt;
+ is_called = seq->is_called;
UnlockReleaseBuffer(buf);
relation_close(seqrel, NoLock);
@@ -1970,13 +1968,10 @@ pg_sequence_state(PG_FUNCTION_ARGS)
/* The value most recently returned by nextval in the current session */
values[1] = Int64GetDatum(last_value);
- /*
-* Shows how many fetches remain before a new WAL record has to be
-* written.
-*/
+ /* How many fetches remain before a new WAL record has to be written */
values[2] = Int64GetDatum(log_cnt);
- /* Indicates whethere the sequence has been used */
+ /* Indicates whether the sequence has been used */
values[3] = BoolGetDatum(is_called);
tuple = heap_form_tuple(tupdesc, values, nulls);
Re: Logical Replication of sequences
On Wed, 3 Jul 2024 at 08:24, Peter Smith wrote: > > Here are my comments for patch v20240702-0001 > > They are all cosmetic and/or typos. Apart from these the 0001 patch LGTM. > > == > doc/src/sgml/func.sgml > > Section 9.17. Sequence Manipulation Functions > > pg_sequence_state: > nitpick - typo /whethere/whether/ > nitpick - reworded slightly using a ChatGPT suggestion. (YMMV, so it > is fine also if you prefer the current wording) > > == > src/backend/commands/sequence.c > > SetSequenceLastValue: > nitpick - typo in function comment /diffrent/different/ > > pg_sequence_state: > nitpick - function comment wording: /page LSN/the page LSN/ > nitpick - moved some comment details about 'lsn_ret' into the function header > nitpick - rearranged variable assignments to have consistent order > with the values > nitpick - tweaked comments > nitpick - typo /whethere/whether/ > > == > 99. > Please see the attached diffs patch which implements all those > nitpicks mentioned above. Thank you for your feedback. I have addressed all the comments in the v20240703 version patch attached at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm0mSSrvHNRnC67f0HWMpoLW9UzxGVXimhwbRtKjE7Aa-Q%40mail.gmail.com Regards, Vignesh
Re: Logical Replication of sequences
Hi Vignesh. Here are my comments for the latest patch v20240703-0001.
==
doc/src/sgml/func.sgml
nitpick - /lsn/LSN/ (all other doc pages I found use uppercase for this acronym)
==
src/backend/commands/sequence.c
nitpick - /lsn/LSN/
==
Please see attached nitpicks diff.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index d30864e..fb3f973 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19521,7 +19521,7 @@ SELECT setval('myseq', 42, false);
Next nextval
Returns information about the sequence. lsn is the
-page lsn of the sequence, last_value is the most
+page LSN of the sequence, last_value is the most
recent value returned by nextval in the current
session, log_cnt shows how many fetches remain
before a new WAL record has to be written, and
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index d83dbd2..bff990a 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1958,7 +1958,7 @@ pg_sequence_state(PG_FUNCTION_ARGS)
UnlockReleaseBuffer(buf);
relation_close(seqrel, NoLock);
- /* Page lsn for the sequence */
+ /* Page LSN for the sequence */
values[0] = LSNGetDatum(lsn);
/* The value most recently returned by nextval in the current session */
Re: Logical Replication of sequences
Here are my review comments for the patch v20240703-0002
==
doc/src/sgml/ref/create_publication.sgml
nitpick - consider putting the "FOR ALL SEQUENCES" para last, because
eventually when more sequence syntax is added IMO it will be better to
describe all the TABLES together, and then describe all the SEQUENCES
together.
nitpick - /synchronizing changes/synchronizes changes/
Question: Was there a reason you chose wording "synchronizes changes"
instead of having same "replicates changes" wording of FOR ALL TABLES?
==
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.
==
src/backend/commands/publicationcmds.c
2.
+ errmsg("must be superuser to create FOR ALL %s publication",
+ stmt->for_all_tables ? "TABLES" : "SEQUENCES")));
nitpick - the combined error message may be fine, but I think
translators will prefer the substitution to be the full "FOR ALL
TABLES" and "FOR ALL SEQUENCES" instead of just the keywords that are
different.
==
src/backend/parser/gram.y
3.
Some of these new things maybe could be named better?
'preprocess_allpubobjtype_list' => 'preprocess_pub_all_objtype_list'
'AllPublicationObjSpec *allpublicationobjectspec;' =>
'PublicationAllObjSpec *publicationallobjectspec;'
(I didn't include these in nitpicks diffs because you probably have
better ideas than I do for good names)
~~~
nitpick - typo in comment /SCHEMAS/SEQUENCES/
preprocess_allpubobjtype_list:
nitpick - typo /allbjects_list/all_objects_list/
nitpick - simplify /allpubob/obj/
nitpick - add underscores in the enums
==
src/bin/pg_dump/pg_dump.c
4.
+ if (pubinfo->puballtables || pubinfo->puballsequences)
+ {
+ appendPQExpBufferStr(query, " FOR ALL");
+ if (pubinfo->puballtables && pubinfo->puballsequences)
+ appendPQExpBufferStr(query, " TABLES, SEQUENCES");
+ else if (pubinfo->puballtables)
+ appendPQExpBufferStr(query, " TABLES");
+ else
+ appendPQExpBufferStr(query, " SEQUENCES");
+ }
nitpick - it seems over-complicated; See nitpicks diff for my suggestion.
==
src/include/nodes/parsenodes.h
nitpick - put underscores in the enum values
~~
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.
==
src/test/regress/sql/publication.sql
nitpick - some small changes to comments, e.g. writing keywords in uppercase
~~~
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?
~~~
7.
Other things to consider:
(I didn't include these in my attached diff)
* could use a single CREATE SEQUENCE stmt instead of multiple
* could use a single DROP PUBLICATION stmt instead of multiple
* shouldn't all publication names ideally have a 'regress_' prefix?
==
99.
Please refer to the attached nitpicks diff which has implementation
for the nitpicks cited above.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPvrk75vSDkaXJVmhhZuuqQSY98btWJV%3DBMZAnyTtKRB4g%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/create_publication.sgml
b/doc/src/sgml/ref/create_publication.sgml
index 0c1b469..9891fef 100644
--- a/doc/src/sgml/ref/create_publication.sgml
+++ b/doc/src/sgml/ref/create_publication.sgml
@@ -122,16 +122,6 @@ CREATE PUBLICATION name
-
-FOR ALL SEQUENCES
-
-
- Marks the publication as one that synchronizing changes for all sequences
- in the database, including sequences created in the future.
-
-
-
-
FOR ALL TABLES
@@ -173,6 +163,16 @@ CREATE PUBLICATION name
+
+FOR ALL SEQUENCES
+
+
+ Marks the publication as one that synchronizes changes for all sequences
+ in the database, including sequences created in the future.
+
+
+
+
WITH ( publication_parameter [= value] [, ... ] )
diff --git a/src/backend/commands/publicationcmds.c
b/src/backend/commands/publicationcmds.c
index 35c7dda..a2ccbf3 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -751,8 +751,9 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt
*stmt)
if ((stmt->for_all_tables || stmt->for_all_sequences) && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-errmsg("must be superuser to create FOR ALL %s
publication",
- stmt->for_all_tables ? "TABLES"
: "SEQUENCES")));
+errmsg("must be superuser to create a %s
publication",
+
Re: Logical Replication of sequences
The latest (v20240704) patch 0001 LGTM == Kind Regards, Peter Smith. Fujitsu Australia
Re: Logical Replication of sequences
Hi Vignesh.
After applying the v20240703-0003 patch, I was always getting errors
when running the subscription TAP tests.
# +++ tap check in src/test/subscription +++
t/001_rep_changes.pl ... ok
t/002_types.pl . ok
t/003_constraints.pl ... ok
t/004_sync.pl .. ok
t/005_encoding.pl .. ok
t/006_rewrite.pl ... ok
t/007_ddl.pl ... 3/?
# Failed test 'Alter subscription set publication throws warning for
non-existent publication'
# at t/007_ddl.pl line 67.
Bailout called. Further testing stopped: pg_ctl stop failed
# Tests were run but no plan was declared and done_testing() was not seen.
FAILED--Further testing stopped: pg_ctl stop failed
make: *** [check] Error 255
~~~
The publisher log shows an Assert TRAP occurred:
2024-07-04 18:15:40.089 AEST [745] mysub1 LOG: statement: SELECT
DISTINCT s.schemaname, s.sequencename
FROM pg_catalog.pg_publication_sequences s
WHERE s.pubname IN ('mypub', 'non_existent_pub', 'non_existent_pub1',
'non_existent_pub2')
TRAP: failed Assert("IsA(list, OidList)"), File:
"../../../src/include/nodes/pg_list.h", Line: 323, PID: 745
~~~
A debugging backtrace looks like below:
Core was generated by `postgres: publisher: walsender postgres
postgres [local] SELECT '.
Program terminated with signal 6, Aborted.
#0 0x7f36f44f02c7 in raise () from /lib64/libc.so.6
Missing separate debuginfos, use: debuginfo-install
glibc-2.17-260.el7_6.6.x86_64 pcre-8.32-17.el7.x86_64
(gdb) bt
#0 0x7f36f44f02c7 in raise () from /lib64/libc.so.6
#1 0x7f36f44f19b8 in abort () from /lib64/libc.so.6
#2 0x00bb8be1 in ExceptionalCondition (conditionName=0xc7aa6c
"IsA(list, OidList)",
fileName=0xc7aa10 "../../../src/include/nodes/pg_list.h",
lineNumber=323) at assert.c:66
#3 0x005f2c57 in list_nth_oid (list=0x27948f0, n=0) at
../../../src/include/nodes/pg_list.h:323
#4 0x005f5491 in pg_get_publication_sequences
(fcinfo=0x2796a00) at pg_publication.c:1334
#5 0x00763d10 in ExecMakeTableFunctionResult
(setexpr=0x27b2fd8, econtext=0x27b2ef8, argContext=0x2796900,
...
Something goes wrong indexing into that 'sequences' list.
1329 funcctx = SRF_PERCALL_SETUP();
1330 sequences = (List *) funcctx->user_fctx;
1331
1332 if (funcctx->call_cntr < list_length(sequences))
1333 {
1334 Oid relid = list_nth_oid(sequences, funcctx->call_cntr);
1335
1336 SRF_RETURN_NEXT(funcctx, ObjectIdGetDatum(relid));
1337 }
==
Perhaps now it is time to create a CF entry for this thread because
the cfbot could have detected the error earlier.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Logical Replication of sequences
On Fri, 5 Jul 2024 at 09:46, Peter Smith wrote:
>
> Hi Vignesh.
>
> After applying the v20240703-0003 patch, I was always getting errors
> when running the subscription TAP tests.
>
> # +++ tap check in src/test/subscription +++
> t/001_rep_changes.pl ... ok
> t/002_types.pl . ok
> t/003_constraints.pl ... ok
> t/004_sync.pl .. ok
> t/005_encoding.pl .. ok
> t/006_rewrite.pl ... ok
> t/007_ddl.pl ... 3/?
> # Failed test 'Alter subscription set publication throws warning for
> non-existent publication'
> # at t/007_ddl.pl line 67.
> Bailout called. Further testing stopped: pg_ctl stop failed
> # Tests were run but no plan was declared and done_testing() was not seen.
> FAILED--Further testing stopped: pg_ctl stop failed
> make: *** [check] Error 255
>
> ~~~
>
> The publisher log shows an Assert TRAP occurred:
>
> 2024-07-04 18:15:40.089 AEST [745] mysub1 LOG: statement: SELECT
> DISTINCT s.schemaname, s.sequencename
> FROM pg_catalog.pg_publication_sequences s
> WHERE s.pubname IN ('mypub', 'non_existent_pub', 'non_existent_pub1',
> 'non_existent_pub2')
> TRAP: failed Assert("IsA(list, OidList)"), File:
> "../../../src/include/nodes/pg_list.h", Line: 323, PID: 745
>
> ~~~
>
> A debugging backtrace looks like below:
>
> Core was generated by `postgres: publisher: walsender postgres
> postgres [local] SELECT '.
> Program terminated with signal 6, Aborted.
> #0 0x7f36f44f02c7 in raise () from /lib64/libc.so.6
> Missing separate debuginfos, use: debuginfo-install
> glibc-2.17-260.el7_6.6.x86_64 pcre-8.32-17.el7.x86_64
> (gdb) bt
> #0 0x7f36f44f02c7 in raise () from /lib64/libc.so.6
> #1 0x7f36f44f19b8 in abort () from /lib64/libc.so.6
> #2 0x00bb8be1 in ExceptionalCondition (conditionName=0xc7aa6c
> "IsA(list, OidList)",
> fileName=0xc7aa10 "../../../src/include/nodes/pg_list.h",
> lineNumber=323) at assert.c:66
> #3 0x005f2c57 in list_nth_oid (list=0x27948f0, n=0) at
> ../../../src/include/nodes/pg_list.h:323
> #4 0x005f5491 in pg_get_publication_sequences
> (fcinfo=0x2796a00) at pg_publication.c:1334
> #5 0x00763d10 in ExecMakeTableFunctionResult
> (setexpr=0x27b2fd8, econtext=0x27b2ef8, argContext=0x2796900,
> ...
>
> Something goes wrong indexing into that 'sequences' list.
>
> 1329 funcctx = SRF_PERCALL_SETUP();
> 1330 sequences = (List *) funcctx->user_fctx;
> 1331
> 1332 if (funcctx->call_cntr < list_length(sequences))
> 1333 {
> 1334 Oid relid = list_nth_oid(sequences, funcctx->call_cntr);
> 1335
> 1336 SRF_RETURN_NEXT(funcctx, ObjectIdGetDatum(relid));
> 1337 }
I was not able to reproduce this issue after several runs, but looks
like sequences need to be initialized here.
> Perhaps now it is time to create a CF entry for this thread because
> the cfbot could have detected the error earlier.
I have added a commitfest entry for the same at [1].
The v20240705 version patch attached at [2] has the change for the same.
[1] - https://commitfest.postgresql.org/49/5111/
[2] -
https://www.postgresql.org/message-id/CALDaNm3WvLUesGq54JagEkbBh4CBfMoT84Rw7HjL8KML_BSzPw%40mail.gmail.com
Regards,
Vignesh
Re: Logical Replication of sequences
On Fri, Jul 5, 2024 at 9:58 PM vignesh C wrote:
>
> On Thu, 4 Jul 2024 at 12:44, Peter Smith wrote:
> >
> > 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.
>
OTOH, if there had been such a test here then the ("sequence = NIL")
bug in patch 0002 code would have been caught earlier in patch 0002
testing instead of later in patch 0003 testing. In general, I think
each patch should be self-contained w.r.t. to testing all of its new
code, but if you think another test here is overkill then I am fine
with that too.
//
Meanwhile, here are my review comments for patch v20240705-0002
==
doc/src/sgml/ref/create_publication.sgml
1.
The CREATE PUBLICATION page has many examples showing many different
combinations of syntax. I think it would not hurt to add another one
showing SEQUENCES being used.
==
src/backend/commands/publicationcmds.c
2.
+ if (form->puballsequences && !superuser_arg(newOwnerId))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to change owner of publication \"%s\"",
+ NameStr(form->pubname)),
+ errhint("The owner of a FOR ALL SEQUENCES publication must be a
superuser.")));
You might consider combining this with the previous error in the same
way that the "FOR ALL TABLES" and "FOR ALL SEQUENCES" errors were
combined in CreatePublication. The result would be less code. But, I
also think your current code is fine, so I am just putting this out as
an idea in case you prefer it.
==
src/backend/parser/gram.y
nitpick - added a space in the comment
nitpick - changed the call order slightly because $6 comes before $7
==
src/bin/pg_dump/pg_dump.c
3. getPublications
- if (fout->remoteVersion >= 13)
+ if (fout->remoteVersion >= 17)
This should be 18.
==
src/bin/psql/describe.c
4. describeOneTableDetails
+ /* print any publications */
+ if (pset.sversion >= 17)
+ {
This should be 18.
~~~
describeOneTableDetails:
nitpick - removed a redundant "else"
nitpick - simplified the "Publications:" header logic slightly
~~~
5. listPublications
+ if (pset.sversion >= 17)
+ appendPQExpBuffer(&buf,
+ ",\n puballsequences AS \"%s\"",
+ gettext_noop("All sequences"));
This should be 18.
~~~
6. describePublications
+ has_pubsequence = (pset.sversion >= 17);
This should be 18.
~
nitpick - remove some blank lines for consistency with nearby code
==
src/include/nodes/parsenodes.h
nitpick - minor change to comment for PublicationAllObjType
nitpick - the meanings of the enums are self-evident; I didn't think
comments were very useful
==
src/test/regress/sql/publication.sql
7.
I think it will also be helpful to arrange for a SEQUENCE to be
published by *multiple* publications. This would test that they get
listed as expected in the "Publications:" part of the "describe" (\d+)
for the sequence.
==
99.
Please also see the attached diffs patch which implements any nitpicks
mentioned above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index f06584f..ead6299 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10561,7 +10561,7 @@ AlterOwnerStmt: ALTER AGGREGATE aggregate_with_argtypes
OWNER TO RoleSpec
*
* CREATE PUBLICATION name [WITH options]
*
- * CREATE PUBLICATION FOR ALL pub_obj_type [,...] [WITH options]
+ * CREATE PUBLICATION FOR ALL pub_obj_type [, ...] [WITH options]
*
* pub_obj_type is one of:
*
@@ -10591,8 +10591,8 @@ CreatePublicationStmt:
CreatePublicationStmt *n =
makeNode(CreatePublicationStmt);
n->pubname = $3;
- n->options = $7;
preprocess_pub_all_objtype_list($6,
&n->for_all_tables, &n->for_all_sequences, yyscanner);
+ n->options = $7;
$$ = (Node *) n;
}
| CREATE PUBLICATION name FOR pub_obj_list
opt_definition
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index cdd6f11..ea7bb57 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1866,19 +1866,20 @@ describeOneTableDetails(const char *schemaname,
result = PSQLexec(buf.data);
if (!result)
goto error_return;
- else
- tuples = PQntuples(result);
+ tuples = PQntuples(result);
+ /* Mig
Re: Logical Replication of sequences
Here are a few comments for patch v20240705-0003. (This is a WIP. I have only looked at the docs so far.) == doc/src/sgml/config.sgml nitpick - max_logical_replication_workers: /and sequence synchornization worker/and a sequence synchornization worker/ == doc/src/sgml/logical-replication.sgml nitpick - max_logical_replication_workers: re-order list of workers to be consistent with other docs 1-apply,2-parallel,3-tablesync,4-seqsync == doc/src/sgml/ref/alter_subscription.sgml 1. IIUC the existing "REFRESH PUBLICATION" command will fetch and sync all new sequences, etc., and/or remove old ones no longer in the publication. But current docs do not say anything at all about sequences here. It should say something about sequence behaviour. ~~~ 2. For the existing "REFRESH PUBLICATION" there is a sub-option "copy_data=true/false". Won't this need some explanation about how it behaves for sequences? Or will there be another option "copy_sequences=true/false". ~~~ 3. IIUC the main difference between REFRESH PUBLICATION and REFRESH PUBLICATION SEQUENCES is that the 2nd command will try synchronize with all the *existing* sequences to bring them to the same point as on the publisher, but otherwise, they are the same command. If that is correct understanding I don't think that distinction is made very clear in the current docs. ~~~ nitpick - the synopsis is misplaced. It should not be between ENABLE and DISABLE. I moved it. Also, it should say "REFRESH PUBLICATION SEQUENCES" because that is how the new syntax is defined in gram.y nitpick - REFRESH SEQUENCES. Renamed to "REFRESH PUBLICATION SEQUENCES". And, shouldn't "from the publisher" say "with the publisher"? nitpick - changed the varlistentry "id". == 99. Please also see the attached diffs patch which implements any nitpicks mentioned above. == Kind Regards, Peter Smith. Fujitsu Australia diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 981ca51..645fc48 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -5201,7 +5201,7 @@ ANY num_sync ( max_logical_replication_workers must be set to at least the number of subscriptions (for leader apply -workers), plus some reserve for the table synchronization workers, sequence -synchronization worker and parallel apply workers. +workers), plus some reserve for the parallel apply workers, table synchronization workers, and a sequence +synchronization worker. diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml index fc8a33c..f81faf9 100644 --- a/doc/src/sgml/ref/alter_subscription.sgml +++ b/doc/src/sgml/ref/alter_subscription.sgml @@ -26,8 +26,8 @@ ALTER SUBSCRIPTION name SET PUBLICA ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ] ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ] ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( refresh_option [= value] [, ... ] ) ] +ALTER SUBSCRIPTION name REFRESH PUBLICATION SEQUENCES ALTER SUBSCRIPTION name ENABLE -ALTER SUBSCRIPTION name REFRESH SEQUENCES ALTER SUBSCRIPTION name DISABLE ALTER SUBSCRIPTION name SET ( subscription_parameter [= value] [, ... ] ) ALTER SUBSCRIPTION name SKIP ( skip_option = value ) @@ -195,12 +195,12 @@ ALTER SUBSCRIPTION name RENAME TO < - -REFRESH SEQUENCES + +REFRESH PUBLICATION SEQUENCES Fetch missing sequences information from publisher and re-synchronize the - sequence data from the publisher. + sequence data with the publisher.
Re: Logical Replication of sequences
ve code-comment (not in the 0003 patch) seems stale. This
should now also mention sequence sync workers, right?
~~~
11.
- Assert(is_tablesync_worker == OidIsValid(relid));
+ Assert(is_tablesync_worker == OidIsValid(relid) ||
is_sequencesync_worker == OidIsValid(relid));
IIUC there is only a single sequence sync worker for handling all the
sequences. So, what does the 'relid' actually mean here when there are
multiple sequences?
~~~
12. logicalrep_seqsyncworker_failuretime
+/*
+ * Set the sequence sync worker failure time
+ *
+ * Called on sequence sync worker failure exit.
+ */
12a.
The comment should be improved to make it more clear that the failure
time of the sync worker information is stored with the *apply* worker.
See also other review comments in this post about this area -- perhaps
all this can be removed?
~
12b.
Curious if this had to be a separate exit handler or if may this could
have been handled by the existing logicalrep_worker_onexit handler.
See also other review comments int this post about this area --
perhaps all this can be removed?
==
.../replication/logical/sequencesync.c
13. fetch_sequence_data
13a.
The function comment has no explanation of what exactly the returned
value means. It seems like it is what you will assign as 'last_value'
on the subscriber-side.
~
13b.
Some of the table functions like this are called like
'fetch_remote_table_info()'. Maybe it is better to do similar here
(e.g. include the word "remote" in the function name).
~
14.
The reason for the addition logic "(last_value + log_cnt)" is not
obvious. I am guessing it might be related to code from
'nextval_internal' (fetch = log = fetch + SEQ_LOG_VALS;) but it is
complicated. It is unfortunate that the field 'log_cnt' seems hardly
commented anywhere at all.
Also, I am not 100% sure if I trust the logic in the first place. The
caller of this function is doing:
sequence_value = fetch_sequence_data(conn, remoteid, &lsn);
/* sets the sequence with sequence_value */
SetSequenceLastValue(RelationGetRelid(rel), sequence_value);
Won't that mean you can get to a situation where subscriber-side
result of lastval('s') can be *ahead* from lastval('s') on the
publisher? That doesn't seem good.
~~~
copy_sequence:
nitpick - ERROR message. Reword "for table..." to be more like the 2nd
error message immediately below.
nitpick - /RelationGetRelationName(rel)/relname/
nitpick - moved the Assert for 'relkind' to be nearer the assignment.
~
15.
+ /*
+ * Logical replication of sequences is based on decoding WAL records,
+ * describing the "next" state of the sequence the current state in the
+ * relfilenode is yet to reach. But during the initial sync we read the
+ * current state, so we need to reconstruct the WAL record logged when we
+ * started the current batch of sequence values.
+ *
+ * Otherwise we might get duplicate values (on subscriber) if we failed
+ * over right after the sync.
+ */
+ sequence_value = fetch_sequence_data(conn, remoteid, &lsn);
+
+ /* sets the sequence with sequence_value */
+ SetSequenceLastValue(RelationGetRelid(rel), sequence_value);
(This is related to some earlier review comment #14 above). IMO all
this tricky commentary belongs in the function header of
"fetch_sequence_data", where it should be describing that function's
return value.
~~~
LogicalRepSyncSequences:
nitpick - declare void param
nitpick indentation
nitpick - wrapping
nitpick - /sequencerel/sequence_rel/
nitpick - blank lines
~
16.
+ if (check_enable_rls(RelationGetRelid(sequencerel), InvalidOid,
false) == RLS_ENABLED)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("user \"%s\" cannot replicate into relation with row-level
security enabled: \"%s\"",
+ GetUserNameFromId(GetUserId(), true),
+ RelationGetRelationName(sequencerel)));
This should be reworded to refer to sequences instead of relations. Maybe like:
user \"%s\" cannot replicate into sequence \"%s\" with row-level
security enabled"
~
17.
The Calculations involving the BATCH size seem a bit tricky.
e.g. in 1st place it is doing: (curr_seq % MAX_SEQUENCES_SYNC_PER_BATCH == 0)
e.g. in 2nd place it is doing: (next_seq % MAX_SEQUENCES_SYNC_PER_BATCH) == 0)
Maybe this batch logic can be simplified somehow using a bool variable
for the calculation?
Also, where does the number 100 come from? Why not 1000? Why not 10?
Why have batching at all? Maybe there should be some comment to
describe the reason and the chosen value.
~
18.
+ next_seq = curr_seq + 1;
+ if (((next_seq % MAX_SEQUENCES_SYNC_PER_BATCH) == 0) || next_seq == seq_count)
+ {
+ /* LOG all the sequences synchronized during current batch. */
+ int i = curr_seq - (curr_seq % MAX_SEQUENCES_SYNC_PER_BATCH);
+ for (; i <= curr_seq; i++)
+ {
+ SubscriptionRelState *done_seq
Re: Logical Replication of sequences
Hi, I was reading back through this thread to find out how the proposed new command for refreshing sequences, came about. The patch 0705 introduces a new command syntax for ALTER SUBSCRIPTION ... REFRESH SEQUENCES So now there are 2 forms of subscription refresh. #1. ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( refresh_option [= value] [, ... ] ) ] #2. ALTER SUBSCRIPTION name REFRESH SEQUENCES IMO, that separation seems complicated. It leaves many questions like: * It causes a bit of initial confusion. e.g. When I saw the REFRESH SEQUENCES I first assumed that was needed because sequences were not covered by the existing REFRESH PUBLICATION * Why wasn't command #2 called ALTER SUBSCRIPTION REFRESH PUBLICATION SEQUENCES? E.g. missing keyword PUBLICATION. It seems inconsistent. * I expect sequence values can become stale pretty much immediately after command #1, so the user will want to use command #2 anyway... * ... but if command #2 also does add/remove changed sequences same as command #1 then what benefit was there of having the command #1 for sequences? * There is a separation of sequences (from tables) in command #2 but there is no separation currently possible in command #1. It seemed inconsistent. ~~~ IIUC some of the goals I saw in the thread are to: * provide a way to fetch and refresh sequences that also keeps behaviors (e.g. copy_data etc.) consistent with the refresh of subscription tables * provide a way to fetch and refresh *only* sequences I felt you could just enhance the existing refresh command syntax (command #1), instead of introducing a new one it would be simpler and it would still meet those same objectives. Synopsis: ALTER SUBSCRIPTION name REFRESH PUBLICATION [TABLES | SEQUENCES | ALL] [ WITH ( refresh_option [= value] [, ... ] ) ] My only change is the introduction of the optional "[TABLES | SEQUENCES | ALL]" clause. I believe that can do everything your current patch does, plus more: * Can refresh *only* TABLES if that is what you want (current patch 0705 cannot do this) * Can refresh *only* SEQUENCES (same as current patch 0705 command #2) * Has better integration with refresh options like "copy_data" (current patch 0705 command #2 doesn't have options) * Existing REFRESH PUBLICATION syntax still works as-is. You can decide later what is PG18 default if the "[TABLES | SEQUENCES | ALL]" is omitted. ~~~ More examples using proposed syntax. ex1. ALTER SUBSCRIPTION sub REFRESH PUBLICATION TABLES WITH (copy_data = false) - same as PG17 functionality for ALTER SUBSCRIPTION sub REFRESH PUBLICATION WITH (copy_data = false) ex2. ALTER SUBSCRIPTION sub REFRESH PUBLICATION TABLES WITH (copy_data = true) - same as PG17 functionality for ALTER SUBSCRIPTION sub REFRESH PUBLICATION WITH (copy_data = true) ex3. ALTER SUBSCRIPTION sub REFRESH PUBLICATION SEQUENCES WITH (copy data = false) - this adds/removes only sequences to pg_subscription_rel but doesn't update their sequence values ex4. ALTER SUBSCRIPTION sub REFRESH PUBLICATION SEQUENCES WITH (copy data = true) - this adds/removes only sequences to pg_subscription_rel and also updates all sequence values. - this is equivalent behaviour of what your current 0705 patch is doing for command #2, ALTER SUBSCRIPTION sub REFRESH SEQUENCES ex5. ALTER SUBSCRIPTION sub REFRESH PUBLICATION ALL WITH (copy_data = false) - this is equivalent behaviour of what your current 0705 patch is doing for command #1, ALTER SUBSCRIPTION sub REFRESH PUBLICATION WITH (copy_data = false) ex6. ALTER SUBSCRIPTION sub REFRESH PUBLICATION ALL WITH (copy_data = true) - this adds/removes tables and sequences and updates all table initial data sequence values.- I think it is equivalent to your current 0705 patch doing command #1 ALTER SUBSCRIPTION sub REFRESH PUBLICATION WITH (copy_data = true), followed by another command #2 ALTER SUBSCRIPTION sub REFRESH SEQUENCES ex7. ALTER SUBSCRIPTION sub REFRESH PUBLICATION SEQUENCES - Because default copy_data is true you do not need to specify options, so this is the same behaviour as your current 0705 patch command #2, ALTER SUBSCRIPTION sub REFRESH SEQUENCES. ~~~ I hope this post was able to demonstrate that by enhancing the existing command: - it is less tricky to understand the separate command distinctions - there is more functionality/flexibility possible - there is better integration with the refresh options like copy_data - behaviour for tables/sequences is more consistent Anyway, it is just my opinion. Maybe there are some pitfalls I'm unaware of. Thoughts? == Kind Regards, Peter Smith. Fujitsu Australia
Re: Logical Replication of sequences
On Wed, 10 Jul 2024 at 13:46, Peter Smith wrote: > > Here are a few comments for patch v20240705-0003. > > (This is a WIP. I have only looked at the docs so far.) > > == > doc/src/sgml/config.sgml > > nitpick - max_logical_replication_workers: /and sequence > synchornization worker/and a sequence synchornization worker/ > > == > doc/src/sgml/logical-replication.sgml > > nitpick - max_logical_replication_workers: re-order list of workers to > be consistent with other docs 1-apply,2-parallel,3-tablesync,4-seqsync > > == > doc/src/sgml/ref/alter_subscription.sgml > > 1. > IIUC the existing "REFRESH PUBLICATION" command will fetch and sync > all new sequences, etc., and/or remove old ones no longer in the > publication. But current docs do not say anything at all about > sequences here. It should say something about sequence behaviour. > > ~~~ > > 2. > For the existing "REFRESH PUBLICATION" there is a sub-option > "copy_data=true/false". Won't this need some explanation about how it > behaves for sequences? Or will there be another option > "copy_sequences=true/false". > > ~~~ > > 3. > IIUC the main difference between REFRESH PUBLICATION and REFRESH > PUBLICATION SEQUENCES is that the 2nd command will try synchronize > with all the *existing* sequences to bring them to the same point as > on the publisher, but otherwise, they are the same command. If that is > correct understanding I don't think that distinction is made very > clear in the current docs. > > ~~~ > > nitpick - the synopsis is misplaced. It should not be between ENABLE > and DISABLE. I moved it. Also, it should say "REFRESH PUBLICATION > SEQUENCES" because that is how the new syntax is defined in gram.y > > nitpick - REFRESH SEQUENCES. Renamed to "REFRESH PUBLICATION > SEQUENCES". And, shouldn't "from the publisher" say "with the > publisher"? > > nitpick - changed the varlistentry "id". > > == > 99. > Please also see the attached diffs patch which implements any nitpicks > mentioned above. All these comments are handled in the v20240720 version patch attached at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm2vuO7Ya4QVTZKR9jY_mkFFcE_hKUJiXx4KUknPgGFjSg%40mail.gmail.com Regards, Vignesh
Re: Logical Replication of sequences
On Fri, 12 Jul 2024 at 08:22, Peter Smith wrote:
>
> Hi Vignesh. Here are the rest of my comments for patch v20240705-0003.
> ==
> src/backend/catalog/pg_subscription.c
>
> 1. GetSubscriptionSequences
>
> +/*
> + * Get the sequences for the subscription.
> + *
> + * The returned list is palloc'ed in the current memory context.
> + */
>
> Is that comment right? The palloc seems to be done in
> CacheMemoryContext, not in the current context.
This function is removed and GetSubscriptionRelations is being used instead.
> ~
>
> 2.
> The code is very similar to the other function
> GetSubscriptionRelations(). In fact I did not understand how the 2
> functions know what they are returning:
>
> E.g. how does GetSubscriptionRelations not return sequences too?
> E.g. how does GetSubscriptionSequences not return relations too?
GetSubscriptionRelations can be used, so removed the
GetSubscriptionSequences function.
>
> 3. AlterSubscription_refresh
>
> - logicalrep_worker_stop(sub->oid, relid);
> + /* Stop the worker if relation kind is not sequence*/
> + if (relkind != RELKIND_SEQUENCE)
> + logicalrep_worker_stop(sub->oid, relid);
>
> Can you give more reasons in the comment why skip the stop for sequence
> worker?
>
> ~
>
> nitpick - period and space in the comment
>
> ~~~
>
> 8. logicalrep_sequence_sync_worker_find
>
> +/*
> + * Walks the workers array and searches for one that matches given
> + * subscription id.
> + *
> + * We are only interested in the sequence sync worker.
> + */
> +LogicalRepWorker *
> +logicalrep_sequence_sync_worker_find(Oid subid, bool only_running)
>
> There are other similar functions for walking the workers array to
> search for a worker. Instead of having different functions for
> different cases, wouldn't it be cleaner to combine these into a single
> function, where you pass a parameter (e.g. a mask of worker types that
> you are interested in finding)?
I will address this in a future version once the patch has become more stable.
> ~~~
>
> 11.
> - Assert(is_tablesync_worker == OidIsValid(relid));
> + Assert(is_tablesync_worker == OidIsValid(relid) ||
> is_sequencesync_worker == OidIsValid(relid));
>
> IIUC there is only a single sequence sync worker for handling all the
> sequences. So, what does the 'relid' actually mean here when there are
> multiple sequences?
Sequence sync workers will not have relid, modified the assert.
> ~~~
>
> 12. logicalrep_seqsyncworker_failuretime
> 12b.
> Curious if this had to be a separate exit handler or if may this could
> have been handled by the existing logicalrep_worker_onexit handler.
> See also other review comments int this post about this area --
> perhaps all this can be removed?
This function cannot be combined with logicalrep_worker_onexit as this
function should be called only in failure case and this exit handler
should be removed in case of success case.
This cannot be removed because of the following reason:
Consider the following situation: a sequence sync worker starts and
then encounters a failure while syncing sequences. At the same time, a
user initiates a "refresh publication sequences" operation. Given only
the start time, it's not possible to distinguish whether the sequence
sync worker failed or completed successfully. This is because the
"refresh publication sequences" operation would have re-added the
sequences, making it unclear whether the sync worker's failure or
success occurred.
>
> 14.
> The reason for the addition logic "(last_value + log_cnt)" is not
> obvious. I am guessing it might be related to code from
> 'nextval_internal' (fetch = log = fetch + SEQ_LOG_VALS;) but it is
> complicated. It is unfortunate that the field 'log_cnt' seems hardly
> commented anywhere at all.
>
> Also, I am not 100% sure if I trust the logic in the first place. The
> caller of this function is doing:
> sequence_value = fetch_sequence_data(conn, remoteid, &lsn);
> /* sets the sequence with sequence_value */
> SetSequenceLastValue(RelationGetRelid(rel), sequence_value);
>
> Won't that mean you can get to a situation where subscriber-side
> result of lastval('s') can be *ahead* from lastval('s') on the
> publisher? That doesn't seem good.
Added comments for "last_value + log_cnt"
Yes it can be ahead in subscribers. This will happen because every
change of the sequence is not wal logged. It is WAL logged once in
SEQ_LOG_VALS. This was discussed earlier and the sequence value being
ahead was ok.
https://www.postgresql.org/message-id/CA%2BTgmoaVLiKDD5vr1bzL-rxhMA37KCS_2xrqjbKVwGyqK%2BPCXQ%40mail.gmail.com
> 15.
>
Re: Logical Replication of sequences
Hi, On Tue, Jun 4, 2024 at 4:27 PM Amit Kapila wrote: > > 3. Replicate published sequences via walsender at the time of shutdown > or incrementally while decoding checkpoint record. The two ways to > achieve this are: (a) WAL log a special NOOP record just before > shutting down checkpointer. Then allow the WALsender to read the > sequence data and send it to the subscriber while decoding the new > NOOP record. (b) Similar to the previous idea but instead of WAL > logging a new record directly invokes a decoding callback after > walsender receives a request to shutdown which will allow pgoutput to > read and send required sequences. This approach has a drawback that we > are adding more work at the time of shutdown but note that we already > waits for all the WAL records to be decoded and sent before shutting > down the walsender during shutdown of the node. Thanks. IIUC, both of the above approaches decode the sequences during only shutdown. I'm wondering, why not periodically decode and replicate the published sequences so that the decoding at the shutdown will not take that longer? I can imagine a case where there are tens of thousands of sequences in a production server, and surely decoding and sending them just during the shutdown can take a lot of time hampering the overall server uptime. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Logical Replication of sequences
On Tue, Jun 4, 2024 at 4:53 PM Bharath Rupireddy wrote: > > On Tue, Jun 4, 2024 at 4:27 PM Amit Kapila wrote: > > > > 3. Replicate published sequences via walsender at the time of shutdown > > or incrementally while decoding checkpoint record. The two ways to > > achieve this are: (a) WAL log a special NOOP record just before > > shutting down checkpointer. Then allow the WALsender to read the > > sequence data and send it to the subscriber while decoding the new > > NOOP record. (b) Similar to the previous idea but instead of WAL > > logging a new record directly invokes a decoding callback after > > walsender receives a request to shutdown which will allow pgoutput to > > read and send required sequences. This approach has a drawback that we > > are adding more work at the time of shutdown but note that we already > > waits for all the WAL records to be decoded and sent before shutting > > down the walsender during shutdown of the node. > > Thanks. IIUC, both of the above approaches decode the sequences during > only shutdown. I'm wondering, why not periodically decode and > replicate the published sequences so that the decoding at the shutdown > will not take that longer? > Even if we decode it periodically (say each time we decode the checkpoint record) then also we need to send the entire set of sequences at shutdown. This is because the sequences may have changed from the last time we sent them. > > I can imagine a case where there are tens > of thousands of sequences in a production server, and surely decoding > and sending them just during the shutdown can take a lot of time > hampering the overall server uptime. > It is possible but we will send only the sequences that belong to publications for which walsender is supposed to send the required data. Now, we can also imagine providing option 2 (Alter Subscription ... Replicate Sequences) so that users can replicate sequences before shutdown and then disable the subscriptions so that there won't be a corresponding walsender. -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Tue, Jun 4, 2024 at 4:27 PM Amit Kapila wrote: > > 3. Replicate published sequences via walsender at the time of shutdown > or incrementally while decoding checkpoint record. The two ways to > achieve this are: (a) WAL log a special NOOP record just before > shutting down checkpointer. Then allow the WALsender to read the > sequence data and send it to the subscriber while decoding the new > NOOP record. (b) Similar to the previous idea but instead of WAL > logging a new record directly invokes a decoding callback after > walsender receives a request to shutdown which will allow pgoutput to > read and send required sequences. This approach has a drawback that we > are adding more work at the time of shutdown but note that we already > waits for all the WAL records to be decoded and sent before shutting > down the walsender during shutdown of the node. > > Any other ideas? > > In case of primary crash the sequence won't get replicated. That is true even with the previous approach in case walsender is shut down because of a crash, but it is more serious with this approach. How about periodically sending this information? -- Best Wishes, Ashutosh Bapat
Re: Logical Replication of sequences
On 6/4/24 06:57, Amit Kapila wrote: 1. Provide a tool to copy all the sequences from publisher to subscriber. The major drawback is that users need to perform this as an additional step during the upgrade which would be inconvenient and probably not as useful as some built-in mechanism. Agree, this requires additional steps. Not a preferred approach in my opinion. When a large set of sequences are present, it will add additional downtime for upgrade process. 2. Provide a command say Alter Subscription ... Replicate Sequences (or something like that) which users can perform before shutdown of the publisher node during upgrade. This will allow copying all the sequences from the publisher node to the subscriber node directly. Similar to previous approach, this could also be inconvenient for users. This is similar to option 1 except that it is a SQL command now. Still not a preferred approach in my opinion. When a large set of sequences are present, it will add additional downtime for upgrade process. 3. Replicate published sequences via walsender at the time of shutdown or incrementally while decoding checkpoint record. The two ways to achieve this are: (a) WAL log a special NOOP record just before shutting down checkpointer. Then allow the WALsender to read the sequence data and send it to the subscriber while decoding the new NOOP record. (b) Similar to the previous idea but instead of WAL logging a new record directly invokes a decoding callback after walsender receives a request to shutdown which will allow pgoutput to read and send required sequences. This approach has a drawback that we are adding more work at the time of shutdown but note that we already waits for all the WAL records to be decoded and sent before shutting down the walsender during shutdown of the node. At the time of shutdown a) most logical upgrades don't necessarily call for shutdown b) it will still add to total downtime with large set of sequences. Incremental option is better as it will not require a shutdown. I do see a scenario where sequence of events can lead to loss of sequence and generate duplicate sequence values, if subscriber starts consuming sequences while publisher is also consuming them. In such cases, subscriber shall not be allowed sequence consumption. -- Kind Regards, Yogesh Sharma Open Source Enthusiast and Advocate PostgreSQL Contributors Team @ RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Logical Replication of sequences
On Tue, Jun 4, 2024 at 7:40 PM Ashutosh Bapat wrote: > > On Tue, Jun 4, 2024 at 4:27 PM Amit Kapila wrote: >> >> >> 3. Replicate published sequences via walsender at the time of shutdown >> or incrementally while decoding checkpoint record. The two ways to >> achieve this are: (a) WAL log a special NOOP record just before >> shutting down checkpointer. Then allow the WALsender to read the >> sequence data and send it to the subscriber while decoding the new >> NOOP record. (b) Similar to the previous idea but instead of WAL >> logging a new record directly invokes a decoding callback after >> walsender receives a request to shutdown which will allow pgoutput to >> read and send required sequences. This approach has a drawback that we >> are adding more work at the time of shutdown but note that we already >> waits for all the WAL records to be decoded and sent before shutting >> down the walsender during shutdown of the node. >> >> Any other ideas? >> > > In case of primary crash the sequence won't get replicated. That is true even > with the previous approach in case walsender is shut down because of a crash, > but it is more serious with this approach. > Right, but if we just want to support a major version upgrade scenario then this should be fine because upgrades require a clean shutdown. > How about periodically sending this information? > Now, if we want to support some sort of failover then probably this will help. Do you have that use case in mind? If we want to send periodically then we can do it when decoding checkpoint (XLOG_CHECKPOINT_ONLINE) or some other periodic WAL record like running_xacts (XLOG_RUNNING_XACTS). -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Tue, Jun 4, 2024 at 8:56 PM Yogesh Sharma wrote: > > On 6/4/24 06:57, Amit Kapila wrote: > > > 2. Provide a command say Alter Subscription ... Replicate Sequences > > (or something like that) which users can perform before shutdown of > > the publisher node during upgrade. This will allow copying all the > > sequences from the publisher node to the subscriber node directly. > > Similar to previous approach, this could also be inconvenient for > > users. > > This is similar to option 1 except that it is a SQL command now. > Right, but I would still prefer a command as it provides clear steps for the upgrade. Users need to perform (a) Replicate Sequences for a particular subscription (b) Disable that subscription (c) Perform (a) and (b) for all the subscriptions corresponding to the publisher we want to shut down for upgrade. I agree there are some manual steps involved here but it is advisable for users to ensure that they have received the required data on the subscriber before the upgrade of the publisher node, otherwise, they may not be able to continue replication after the upgrade. For example, see the "Prepare for publisher upgrades" step in pg_upgrade docs [1]. > > > 3. Replicate published sequences via walsender at the time of shutdown > > or incrementally while decoding checkpoint record. The two ways to > > achieve this are: (a) WAL log a special NOOP record just before > > shutting down checkpointer. Then allow the WALsender to read the > > sequence data and send it to the subscriber while decoding the new > > NOOP record. (b) Similar to the previous idea but instead of WAL > > logging a new record directly invokes a decoding callback after > > walsender receives a request to shutdown which will allow pgoutput to > > read and send required sequences. This approach has a drawback that we > > are adding more work at the time of shutdown but note that we already > > waits for all the WAL records to be decoded and sent before shutting > > down the walsender during shutdown of the node. > > At the time of shutdown a) most logical upgrades don't necessarily call > for shutdown > Won't the major version upgrade expect that the node is down? Refer to step "Stop both servers" in [1]. > b) it will still add to total downtime with large set of > sequences. Incremental option is better as it will not require a shutdown. > > I do see a scenario where sequence of events can lead to loss of sequence > and generate duplicate sequence values, if subscriber starts consuming > sequences while publisher is also consuming them. In such cases, subscriber > shall not be allowed sequence consumption. > It would be fine to not allow subscribers to consume sequences that are being logically replicated but what about the cases where we haven't sent the latest values of sequences before the shutdown of the publisher? In such a case, the publisher would have already consumed some values that wouldn't have been sent to the subscriber and now when the publisher is down then even if we re-allow the sequence values to be consumed from the subscriber, it can lead to duplicate values. [1] - https://www.postgresql.org/docs/devel/pgupgrade.html -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On 04.06.24 12:57, Amit Kapila wrote: 2. Provide a command say Alter Subscription ... Replicate Sequences (or something like that) which users can perform before shutdown of the publisher node during upgrade. This will allow copying all the sequences from the publisher node to the subscriber node directly. Similar to previous approach, this could also be inconvenient for users. I would start with this. In any case, you're going to need to write code to collect all the sequence values, send them over some protocol, apply them on the subscriber. The easiest way to start is to trigger that manually. Then later you can add other ways to trigger it, either by timer or around shutdown, or whatever other ideas there might be.
Re: Logical Replication of sequences
On Wed, Jun 5, 2024 at 9:13 AM Amit Kapila wrote: > > On Tue, Jun 4, 2024 at 8:56 PM Yogesh Sharma > wrote: > > > > On 6/4/24 06:57, Amit Kapila wrote: > > > > > 2. Provide a command say Alter Subscription ... Replicate Sequences > > > (or something like that) which users can perform before shutdown of > > > the publisher node during upgrade. This will allow copying all the > > > sequences from the publisher node to the subscriber node directly. > > > Similar to previous approach, this could also be inconvenient for > > > users. > > > > This is similar to option 1 except that it is a SQL command now. > > > > Right, but I would still prefer a command as it provides clear steps > for the upgrade. Users need to perform (a) Replicate Sequences for a > particular subscription (b) Disable that subscription (c) Perform (a) > and (b) for all the subscriptions corresponding to the publisher we > want to shut down for upgrade. > Another advantage of this approach over just a plain tool to copy all sequences before upgrade is that here we can have the facility to copy just the required sequences. I mean the set sequences that the user has specified as part of the publication. -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
Hi, On Tue, Jun 4, 2024 at 5:40 PM Amit Kapila wrote: > > Even if we decode it periodically (say each time we decode the > checkpoint record) then also we need to send the entire set of > sequences at shutdown. This is because the sequences may have changed > from the last time we sent them. Agree. How about decoding and sending only the sequences that are changed from the last time when they were sent? I know it requires a bit of tracking and more work, but all I'm looking for is to reduce the amount of work that walsenders need to do during the shutdown. Having said that, I like the idea of letting the user sync the sequences via ALTER SUBSCRIPTION command and not weave the logic into the shutdown checkpoint path. As Peter Eisentraut said here https://www.postgresql.org/message-id/42e5cb35-4aeb-4f58-8091-90619c7c3ecc%40eisentraut.org, this can be a good starting point to get going. > > I can imagine a case where there are tens > > of thousands of sequences in a production server, and surely decoding > > and sending them just during the shutdown can take a lot of time > > hampering the overall server uptime. > > It is possible but we will send only the sequences that belong to > publications for which walsender is supposed to send the required > data. Right, but what if all the publication tables can have tens of thousands of sequences. > Now, we can also imagine providing option 2 (Alter Subscription > ... Replicate Sequences) so that users can replicate sequences before > shutdown and then disable the subscriptions so that there won't be a > corresponding walsender. As stated above, I like this idea to start with. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Logical Replication of sequences
On Wed, Jun 5, 2024 at 12:51 PM Peter Eisentraut wrote: > > On 04.06.24 12:57, Amit Kapila wrote: > > 2. Provide a command say Alter Subscription ... Replicate Sequences > > (or something like that) which users can perform before shutdown of > > the publisher node during upgrade. This will allow copying all the > > sequences from the publisher node to the subscriber node directly. > > Similar to previous approach, this could also be inconvenient for > > users. > > I would start with this. In any case, you're going to need to write > code to collect all the sequence values, send them over some protocol, > apply them on the subscriber. The easiest way to start is to trigger > that manually. Then later you can add other ways to trigger it, either > by timer or around shutdown, or whatever other ideas there might be. > Agreed. To achieve this, we can allow sequences to be copied during the initial CREATE SUBSCRIPTION command similar to what we do for tables. And then later by new/existing command, we re-copy the already existing sequences on the subscriber. The options for the new command could be: Alter Subscription ... Refresh Sequences Alter Subscription ... Replicate Sequences In the second option, we need to introduce a new keyword Replicate. Can you think of any better option? In addition to the above, the command Alter Subscription .. Refresh Publication will fetch any missing sequences similar to what it does for tables. Thoughts? -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Wed, Jun 5, 2024 at 8:45 AM Amit Kapila wrote: > On Tue, Jun 4, 2024 at 7:40 PM Ashutosh Bapat > wrote: > > > > On Tue, Jun 4, 2024 at 4:27 PM Amit Kapila > wrote: > >> > >> > >> 3. Replicate published sequences via walsender at the time of shutdown > >> or incrementally while decoding checkpoint record. The two ways to > >> achieve this are: (a) WAL log a special NOOP record just before > >> shutting down checkpointer. Then allow the WALsender to read the > >> sequence data and send it to the subscriber while decoding the new > >> NOOP record. (b) Similar to the previous idea but instead of WAL > >> logging a new record directly invokes a decoding callback after > >> walsender receives a request to shutdown which will allow pgoutput to > >> read and send required sequences. This approach has a drawback that we > >> are adding more work at the time of shutdown but note that we already > >> waits for all the WAL records to be decoded and sent before shutting > >> down the walsender during shutdown of the node. > >> > >> Any other ideas? > >> > > > > In case of primary crash the sequence won't get replicated. That is true > even with the previous approach in case walsender is shut down because of a > crash, but it is more serious with this approach. > > > > Right, but if we just want to support a major version upgrade scenario > then this should be fine because upgrades require a clean shutdown. > > > > How about periodically sending this information? > > > > Now, if we want to support some sort of failover then probably this > will help. Do you have that use case in mind? Regular failover was a goal for supporting logical replication of sequences. That might be more common than major upgrade scenario. > If we want to send > periodically then we can do it when decoding checkpoint > (XLOG_CHECKPOINT_ONLINE) or some other periodic WAL record like > running_xacts (XLOG_RUNNING_XACTS). > > Yeah. I am thinking along those lines. It must be noted, however, that none of those optional make sure that the replicated sequence's states are consistent with the replicated object state which use those sequences. E.g. table t1 uses sequence s1. By last sequence replication, as of time T1, let's say t1 had consumed values upto vl1 from s1. But later, by time T2, it consumed values upto vl2 which were not replicated but the changes to t1 by T2 were replicated. If failover happens at that point, INSERTs on t1 would fail because of duplicate keys (values between vl1 and vl2). Previous attempt to support logical sequence replication solved this problem by replicating a future state of sequences (current value +/- log count). Similarly, if the sequence was ALTERed between T1 and T2, the state of sequence on replica would be inconsistent with the state of t1. Failing over at this stage, might end t1 in an inconsistent state. -- Best Wishes, Ashutosh Bapat
Re: Logical Replication of sequences
On Wed, Jun 5, 2024 at 6:01 PM Ashutosh Bapat wrote: > > On Wed, Jun 5, 2024 at 8:45 AM Amit Kapila wrote: >> >> How about periodically sending this information? >> > >> >> Now, if we want to support some sort of failover then probably this >> will help. Do you have that use case in mind? > > > Regular failover was a goal for supporting logical replication of sequences. > That might be more common than major upgrade scenario. > We can't support regular failovers to subscribers unless we can replicate/copy slots because the existing nodes connected to the current publisher/primary would expect that. It should be primarily useful for major version upgrades at this stage. >> >> If we want to send >> periodically then we can do it when decoding checkpoint >> (XLOG_CHECKPOINT_ONLINE) or some other periodic WAL record like >> running_xacts (XLOG_RUNNING_XACTS). >> > > Yeah. I am thinking along those lines. > > It must be noted, however, that none of those optional make sure that the > replicated sequence's states are consistent with the replicated object state > which use those sequences. > Right, I feel as others are advocating, it seems better to support it manually via command and then later we can extend it to do at shutdown or at some regular intervals. If we do that then we should be able to support major version upgrades and planned switchover. -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Wed, Jun 5, 2024 at 12:43 PM Amit Kapila wrote: > > On Tue, Jun 4, 2024 at 8:56 PM Yogesh Sharma > wrote: > > > > On 6/4/24 06:57, Amit Kapila wrote: > > > > > 2. Provide a command say Alter Subscription ... Replicate Sequences > > > (or something like that) which users can perform before shutdown of > > > the publisher node during upgrade. This will allow copying all the > > > sequences from the publisher node to the subscriber node directly. > > > Similar to previous approach, this could also be inconvenient for > > > users. > > > > This is similar to option 1 except that it is a SQL command now. > > > > Right, but I would still prefer a command as it provides clear steps > for the upgrade. Users need to perform (a) Replicate Sequences for a > particular subscription (b) Disable that subscription (c) Perform (a) > and (b) for all the subscriptions corresponding to the publisher we > want to shut down for upgrade. > > I agree there are some manual steps involved here but it is advisable > for users to ensure that they have received the required data on the > subscriber before the upgrade of the publisher node, otherwise, they > may not be able to continue replication after the upgrade. For > example, see the "Prepare for publisher upgrades" step in pg_upgrade > docs [1]. > > > > > > 3. Replicate published sequences via walsender at the time of shutdown > > > or incrementally while decoding checkpoint record. The two ways to > > > achieve this are: (a) WAL log a special NOOP record just before > > > shutting down checkpointer. Then allow the WALsender to read the > > > sequence data and send it to the subscriber while decoding the new > > > NOOP record. (b) Similar to the previous idea but instead of WAL > > > logging a new record directly invokes a decoding callback after > > > walsender receives a request to shutdown which will allow pgoutput to > > > read and send required sequences. This approach has a drawback that we > > > are adding more work at the time of shutdown but note that we already > > > waits for all the WAL records to be decoded and sent before shutting > > > down the walsender during shutdown of the node. > > > > At the time of shutdown a) most logical upgrades don't necessarily call > > for shutdown > > > > Won't the major version upgrade expect that the node is down? Refer to > step "Stop both servers" in [1]. I think the idea is that the publisher is the old version and the subscriber is the new version, and changes generated on the publisher are replicated to the subscriber via logical replication. And at some point, we change the application (or a router) settings so that no more transactions come to the publisher, do the last upgrade preparation work (e.g. copying the latest sequence values if requried), and then change the application so that new transactions come to the subscriber. I remember the blog post about Knock doing a similar process to upgrade the clusters with minimal downtime[1]. Regards, [1] https://knock.app/blog/zero-downtime-postgres-upgrades -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Logical Replication of sequences
On Wed, Jun 5, 2024 at 3:17 PM Bharath Rupireddy wrote: > > On Tue, Jun 4, 2024 at 5:40 PM Amit Kapila wrote: > > > > Even if we decode it periodically (say each time we decode the > > checkpoint record) then also we need to send the entire set of > > sequences at shutdown. This is because the sequences may have changed > > from the last time we sent them. > > Agree. How about decoding and sending only the sequences that are > changed from the last time when they were sent? I know it requires a > bit of tracking and more work, but all I'm looking for is to reduce > the amount of work that walsenders need to do during the shutdown. > I see your point but going towards tracking the changed sequences sounds like moving towards what we do for incremental backups unless we can invent some other smart way. > Having said that, I like the idea of letting the user sync the > sequences via ALTER SUBSCRIPTION command and not weave the logic into > the shutdown checkpoint path. As Peter Eisentraut said here > https://www.postgresql.org/message-id/42e5cb35-4aeb-4f58-8091-90619c7c3ecc%40eisentraut.org, > this can be a good starting point to get going. > Agreed. > > > I can imagine a case where there are tens > > > of thousands of sequences in a production server, and surely decoding > > > and sending them just during the shutdown can take a lot of time > > > hampering the overall server uptime. > > > > It is possible but we will send only the sequences that belong to > > publications for which walsender is supposed to send the required > > data. > > Right, but what if all the publication tables can have tens of > thousands of sequences. > In such cases we have no option but to send all the sequences. > > Now, we can also imagine providing option 2 (Alter Subscription > > ... Replicate Sequences) so that users can replicate sequences before > > shutdown and then disable the subscriptions so that there won't be a > > corresponding walsender. > > As stated above, I like this idea to start with. > +1. -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Wed, Jun 5, 2024 at 9:30 PM Amit Kapila wrote: > > On Wed, Jun 5, 2024 at 12:51 PM Peter Eisentraut wrote: > > > > On 04.06.24 12:57, Amit Kapila wrote: > > > 2. Provide a command say Alter Subscription ... Replicate Sequences > > > (or something like that) which users can perform before shutdown of > > > the publisher node during upgrade. This will allow copying all the > > > sequences from the publisher node to the subscriber node directly. > > > Similar to previous approach, this could also be inconvenient for > > > users. > > > > I would start with this. In any case, you're going to need to write > > code to collect all the sequence values, send them over some protocol, > > apply them on the subscriber. The easiest way to start is to trigger > > that manually. Then later you can add other ways to trigger it, either > > by timer or around shutdown, or whatever other ideas there might be. > > > > Agreed. +1 > To achieve this, we can allow sequences to be copied during > the initial CREATE SUBSCRIPTION command similar to what we do for > tables. And then later by new/existing command, we re-copy the already > existing sequences on the subscriber. > > The options for the new command could be: > Alter Subscription ... Refresh Sequences > Alter Subscription ... Replicate Sequences > > In the second option, we need to introduce a new keyword Replicate. > Can you think of any better option? Another idea is doing that using options. For example, For initial sequences synchronization: CREATE SUBSCRIPTION ... WITH (copy_sequence = true); For re-copy (or update) sequences: ALTER SUBSCRIPTION ... REFRESH PUBLICATION WITH (copy_sequence = true); > > In addition to the above, the command Alter Subscription .. Refresh > Publication will fetch any missing sequences similar to what it does > for tables. On the subscriber side, do we need to track which sequences are created via CREATE/ALTER SUBSCRIPTION? Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Logical Replication of sequences
On Thu, Jun 6, 2024 at 9:32 AM Masahiko Sawada wrote: > > On Wed, Jun 5, 2024 at 12:43 PM Amit Kapila wrote: > > > > On Tue, Jun 4, 2024 at 8:56 PM Yogesh Sharma > > wrote: > > > > > > On 6/4/24 06:57, Amit Kapila wrote: > > > > > > > 2. Provide a command say Alter Subscription ... Replicate Sequences > > > > (or something like that) which users can perform before shutdown of > > > > the publisher node during upgrade. This will allow copying all the > > > > sequences from the publisher node to the subscriber node directly. > > > > Similar to previous approach, this could also be inconvenient for > > > > users. > > > > > > This is similar to option 1 except that it is a SQL command now. > > > > > > > Right, but I would still prefer a command as it provides clear steps > > for the upgrade. Users need to perform (a) Replicate Sequences for a > > particular subscription (b) Disable that subscription (c) Perform (a) > > and (b) for all the subscriptions corresponding to the publisher we > > want to shut down for upgrade. > > > > I agree there are some manual steps involved here but it is advisable > > for users to ensure that they have received the required data on the > > subscriber before the upgrade of the publisher node, otherwise, they > > may not be able to continue replication after the upgrade. For > > example, see the "Prepare for publisher upgrades" step in pg_upgrade > > docs [1]. > > > > > > > > > 3. Replicate published sequences via walsender at the time of shutdown > > > > or incrementally while decoding checkpoint record. The two ways to > > > > achieve this are: (a) WAL log a special NOOP record just before > > > > shutting down checkpointer. Then allow the WALsender to read the > > > > sequence data and send it to the subscriber while decoding the new > > > > NOOP record. (b) Similar to the previous idea but instead of WAL > > > > logging a new record directly invokes a decoding callback after > > > > walsender receives a request to shutdown which will allow pgoutput to > > > > read and send required sequences. This approach has a drawback that we > > > > are adding more work at the time of shutdown but note that we already > > > > waits for all the WAL records to be decoded and sent before shutting > > > > down the walsender during shutdown of the node. > > > > > > At the time of shutdown a) most logical upgrades don't necessarily call > > > for shutdown > > > > > > > Won't the major version upgrade expect that the node is down? Refer to > > step "Stop both servers" in [1]. > > I think the idea is that the publisher is the old version and the > subscriber is the new version, and changes generated on the publisher > are replicated to the subscriber via logical replication. And at some > point, we change the application (or a router) settings so that no > more transactions come to the publisher, do the last upgrade > preparation work (e.g. copying the latest sequence values if > requried), and then change the application so that new transactions > come to the subscriber. > Okay, thanks for sharing the exact steps. If one has to follow that path then sending incrementally (at checkpoint WAL or other times) won't work because we want to ensure that the sequences are up-to-date before the application starts using the new database. To do that in a bullet-proof way, one has to copy/replicate sequences during the requests to the new database are paused (Reference from the blog you shared: For the first second after flipping the flag, our application artificially paused any new database requests for one second.). Currently, they are using some guesswork to replicate sequences that require manual verification and more manual work for each sequence. The new command (Alter Subscription ... Replicate Sequence) should ease their procedure and can do things where they would require no or very less verification. > I remember the blog post about Knock doing a similar process to > upgrade the clusters with minimal downtime[1]. > Thanks for sharing the blog post. -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Thu, Jun 6, 2024 at 11:10 AM Masahiko Sawada wrote: > > On Wed, Jun 5, 2024 at 9:30 PM Amit Kapila wrote: > > > > > To achieve this, we can allow sequences to be copied during > > the initial CREATE SUBSCRIPTION command similar to what we do for > > tables. And then later by new/existing command, we re-copy the already > > existing sequences on the subscriber. > > > > The options for the new command could be: > > Alter Subscription ... Refresh Sequences > > Alter Subscription ... Replicate Sequences > > > > In the second option, we need to introduce a new keyword Replicate. > > Can you think of any better option? > > Another idea is doing that using options. For example, > > For initial sequences synchronization: > > CREATE SUBSCRIPTION ... WITH (copy_sequence = true); > How will it interact with the existing copy_data option? So copy_data will become equivalent to copy_table_data, right? > For re-copy (or update) sequences: > > ALTER SUBSCRIPTION ... REFRESH PUBLICATION WITH (copy_sequence = true); > Similar to the previous point it can be slightly confusing w.r.t copy_data. And would copy_sequence here mean that it would copy sequence values of both pre-existing and newly added sequences, if so, that would make it behave differently than copy_data? The other possibility in this direction would be to introduce an option like replicate_all_sequences/copy_all_sequences which indicates a copy of both pre-existing and new sequences, if any. If we want to go in the direction of having an option such as copy_(all)_sequences then do you think specifying that copy_data is just for tables in the docs would be sufficient? I am afraid that it would be confusing for users. > > > > In addition to the above, the command Alter Subscription .. Refresh > > Publication will fetch any missing sequences similar to what it does > > for tables. > > On the subscriber side, do we need to track which sequences are > created via CREATE/ALTER SUBSCRIPTION? > I think so unless we find some other way to know at refresh publication time which all new sequences need to be part of the subscription. What should be the behavior w.r.t sequences when the user performs ALTER SUBSCRIPTION ... REFRESH PUBLICATION? I was thinking similar to tables, it should fetch any missing sequence information from the publisher. -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Thu, Jun 6, 2024 at 9:22 AM Amit Kapila wrote: > On Wed, Jun 5, 2024 at 6:01 PM Ashutosh Bapat > wrote: > > > > On Wed, Jun 5, 2024 at 8:45 AM Amit Kapila > wrote: > >> > >> How about periodically sending this information? > >> > > >> > >> Now, if we want to support some sort of failover then probably this > >> will help. Do you have that use case in mind? > > > > > > Regular failover was a goal for supporting logical replication of > sequences. That might be more common than major upgrade scenario. > > > > We can't support regular failovers to subscribers unless we can > replicate/copy slots because the existing nodes connected to the > current publisher/primary would expect that. It should be primarily > useful for major version upgrades at this stage. > We don't want to design it in a way that requires major rework when we are able to copy slots and then support regular failovers. That's when the consistency between a sequence and the table using it would be a must. So it's better that we take that into consideration now. -- Best Wishes, Ashutosh Bapat
Re: Logical Replication of sequences
On Thu, Jun 6, 2024 at 3:44 PM Ashutosh Bapat wrote: > > On Thu, Jun 6, 2024 at 9:22 AM Amit Kapila wrote: >> >> On Wed, Jun 5, 2024 at 6:01 PM Ashutosh Bapat >> wrote: >> > >> > On Wed, Jun 5, 2024 at 8:45 AM Amit Kapila wrote: >> >> >> >> How about periodically sending this information? >> >> > >> >> >> >> Now, if we want to support some sort of failover then probably this >> >> will help. Do you have that use case in mind? >> > >> > >> > Regular failover was a goal for supporting logical replication of >> > sequences. That might be more common than major upgrade scenario. >> > >> >> We can't support regular failovers to subscribers unless we can >> replicate/copy slots because the existing nodes connected to the >> current publisher/primary would expect that. It should be primarily >> useful for major version upgrades at this stage. > > > We don't want to design it in a way that requires major rework when we are > able to copy slots and then support regular failover. > I don't think we can just copy slots like we do for standbys. The slots would require WAL locations to continue, so not sure if we can make it work for failover for subscribers. > That's when the consistency between a sequence and the table using it would be a must. So it's better that we take that into consideration now. > With the ideas being discussed here, I could only see the use case of a major version upgrade or planned switchover to work. If we come up with any other agreeable way that is better than this then we can consider the same. -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Thu, Jun 6, 2024 at 9:34 AM Amit Kapila wrote: > > On Wed, Jun 5, 2024 at 3:17 PM Bharath Rupireddy > wrote: > > > > On Tue, Jun 4, 2024 at 5:40 PM Amit Kapila wrote: > > > > > > Even if we decode it periodically (say each time we decode the > > > checkpoint record) then also we need to send the entire set of > > > sequences at shutdown. This is because the sequences may have changed > > > from the last time we sent them. > > > > Agree. How about decoding and sending only the sequences that are > > changed from the last time when they were sent? I know it requires a > > bit of tracking and more work, but all I'm looking for is to reduce > > the amount of work that walsenders need to do during the shutdown. > > > > I see your point but going towards tracking the changed sequences > sounds like moving towards what we do for incremental backups unless > we can invent some other smart way. Yes, we would need an entirely new infrastructure to track the sequence change since the last sync. We can only determine this from WAL, and relying on it would somehow bring us back to the approach we were trying to achieve with logical decoding of sequences patch. > > Having said that, I like the idea of letting the user sync the > > sequences via ALTER SUBSCRIPTION command and not weave the logic into > > the shutdown checkpoint path. As Peter Eisentraut said here > > https://www.postgresql.org/message-id/42e5cb35-4aeb-4f58-8091-90619c7c3ecc%40eisentraut.org, > > this can be a good starting point to get going. > > > > Agreed. +1 -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Logical Replication of sequences
On Thu, Jun 6, 2024 at 6:40 PM Amit Kapila wrote: > > On Thu, Jun 6, 2024 at 11:10 AM Masahiko Sawada wrote: > > > > On Wed, Jun 5, 2024 at 9:30 PM Amit Kapila wrote: > > > > > > > > To achieve this, we can allow sequences to be copied during > > > the initial CREATE SUBSCRIPTION command similar to what we do for > > > tables. And then later by new/existing command, we re-copy the already > > > existing sequences on the subscriber. > > > > > > The options for the new command could be: > > > Alter Subscription ... Refresh Sequences > > > Alter Subscription ... Replicate Sequences > > > > > > In the second option, we need to introduce a new keyword Replicate. > > > Can you think of any better option? > > > > Another idea is doing that using options. For example, > > > > For initial sequences synchronization: > > > > CREATE SUBSCRIPTION ... WITH (copy_sequence = true); > > > > How will it interact with the existing copy_data option? So copy_data > will become equivalent to copy_table_data, right? Right. > > > For re-copy (or update) sequences: > > > > ALTER SUBSCRIPTION ... REFRESH PUBLICATION WITH (copy_sequence = true); > > > > Similar to the previous point it can be slightly confusing w.r.t > copy_data. And would copy_sequence here mean that it would copy > sequence values of both pre-existing and newly added sequences, if so, > that would make it behave differently than copy_data? The other > possibility in this direction would be to introduce an option like > replicate_all_sequences/copy_all_sequences which indicates a copy of > both pre-existing and new sequences, if any. Copying sequence data works differently than replicating table data (initial data copy and logical replication). So I thought the copy_sequence option (or whatever better name) always does both updating pre-existing sequences and adding new sequences. REFRESH PUBLICATION updates the tables to be subscribed, so we also update or add sequences associated to these tables. > > If we want to go in the direction of having an option such as > copy_(all)_sequences then do you think specifying that copy_data is > just for tables in the docs would be sufficient? I am afraid that it > would be confusing for users. I see your point. But I guess it would not be very problematic as it doesn't break the current behavior and copy_(all)_sequences is primarily for upgrade use cases. > > > > > > > In addition to the above, the command Alter Subscription .. Refresh > > > Publication will fetch any missing sequences similar to what it does > > > for tables. > > > > On the subscriber side, do we need to track which sequences are > > created via CREATE/ALTER SUBSCRIPTION? > > > > I think so unless we find some other way to know at refresh > publication time which all new sequences need to be part of the > subscription. What should be the behavior w.r.t sequences when the > user performs ALTER SUBSCRIPTION ... REFRESH PUBLICATION? I was > thinking similar to tables, it should fetch any missing sequence > information from the publisher. It seems to make sense to me. But I have one question: do we want to support replicating sequences that are not associated with any tables? if yes, what if we refresh two different subscriptions that subscribe to different tables on the same database? On the other hand, if no (i.e. replicating only sequences owned by tables), can we know which sequences to replicate by checking the subscribed tables? Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Logical Replication of sequences
On Fri, Jun 7, 2024 at 7:55 AM Masahiko Sawada wrote: > > On Thu, Jun 6, 2024 at 6:40 PM Amit Kapila wrote: > > > > On Thu, Jun 6, 2024 at 11:10 AM Masahiko Sawada > > wrote: > > > > > > On Wed, Jun 5, 2024 at 9:30 PM Amit Kapila > > > wrote: > > > > > > > > > > > To achieve this, we can allow sequences to be copied during > > > > the initial CREATE SUBSCRIPTION command similar to what we do for > > > > tables. And then later by new/existing command, we re-copy the already > > > > existing sequences on the subscriber. > > > > > > > > The options for the new command could be: > > > > Alter Subscription ... Refresh Sequences > > > > Alter Subscription ... Replicate Sequences > > > > > > > > In the second option, we need to introduce a new keyword Replicate. > > > > Can you think of any better option? > > > > > > Another idea is doing that using options. For example, > > > > > > For initial sequences synchronization: > > > > > > CREATE SUBSCRIPTION ... WITH (copy_sequence = true); > > > > > > > How will it interact with the existing copy_data option? So copy_data > > will become equivalent to copy_table_data, right? > > Right. > > > > > > For re-copy (or update) sequences: > > > > > > ALTER SUBSCRIPTION ... REFRESH PUBLICATION WITH (copy_sequence = true); > > > > > > > Similar to the previous point it can be slightly confusing w.r.t > > copy_data. And would copy_sequence here mean that it would copy > > sequence values of both pre-existing and newly added sequences, if so, > > that would make it behave differently than copy_data? The other > > possibility in this direction would be to introduce an option like > > replicate_all_sequences/copy_all_sequences which indicates a copy of > > both pre-existing and new sequences, if any. > > Copying sequence data works differently than replicating table data > (initial data copy and logical replication). So I thought the > copy_sequence option (or whatever better name) always does both > updating pre-existing sequences and adding new sequences. REFRESH > PUBLICATION updates the tables to be subscribed, so we also update or > add sequences associated to these tables. > Are you imagining the behavior for sequences associated with tables differently than the ones defined by the CREATE SEQUENCE .. command? I was thinking that users would associate sequences with publications similar to what we do for tables for both cases. For example, they need to explicitly mention the sequences they want to replicate by commands like CREATE PUBLICATION ... FOR SEQUENCE s1, s2, ...; CREATE PUBLICATION ... FOR ALL SEQUENCES, or CREATE PUBLICATION ... FOR SEQUENCES IN SCHEMA sch1; In this, variants FOR ALL SEQUENCES and SEQUENCES IN SCHEMA sch1 should copy both the explicitly defined sequences and sequences defined with the tables. Do you think a different variant for just copying sequences implicitly associated with tables (say for identity columns)? > > > > > > > > > > > In addition to the above, the command Alter Subscription .. Refresh > > > > Publication will fetch any missing sequences similar to what it does > > > > for tables. > > > > > > On the subscriber side, do we need to track which sequences are > > > created via CREATE/ALTER SUBSCRIPTION? > > > > > > > I think so unless we find some other way to know at refresh > > publication time which all new sequences need to be part of the > > subscription. What should be the behavior w.r.t sequences when the > > user performs ALTER SUBSCRIPTION ... REFRESH PUBLICATION? I was > > thinking similar to tables, it should fetch any missing sequence > > information from the publisher. > > It seems to make sense to me. But I have one question: do we want to > support replicating sequences that are not associated with any tables? > Yes, unless we see a problem with it. > if yes, what if we refresh two different subscriptions that subscribe > to different tables on the same database? What problem do you see with it? > On the other hand, if no > (i.e. replicating only sequences owned by tables), can we know which > sequences to replicate by checking the subscribed tables? > Sorry, I didn't understand your question. Can you please try to explain in more words or use some examples? -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
Here are some review comments for patch v20240720-0002.
==
1. Commit message:
1a.
The commit message is stale. It is still referring to functions and
views that have been moved to patch 0003.
1b.
"ALL SEQUENCES" is not a command. It is a clause of the CREATE
PUBLICATION command.
==
doc/src/sgml/ref/create_publication.sgml
nitpick - publication name in the example /allsequences/all_sequences/
==
src/bin/psql/describe.c
2. describeOneTableDetails
Although it's not the fault of this patch, this patch propagates the
confusion of 'result' versus 'res'. Basically, I did not understand
the need for the variable 'result'. There is already a "PGResult
*res", and unless I am mistaken we can just keep re-using that instead
of introducing a 2nd variable having almost the same name and purpose.
~
nitpick - comment case
nitpick - rearrange comment
==
src/test/regress/expected/publication.out
(see publication.sql)
==
src/test/regress/sql/publication.sql
nitpick - tweak comment
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/create_publication.sgml
b/doc/src/sgml/ref/create_publication.sgml
index c9c1b92..7dcfe37 100644
--- a/doc/src/sgml/ref/create_publication.sgml
+++ b/doc/src/sgml/ref/create_publication.sgml
@@ -422,7 +422,7 @@ CREATE PUBLICATION users_filtered FOR TABLE users (user_id,
firstname);
Create a publication that synchronizes all the sequences:
-CREATE PUBLICATION allsequences FOR ALL SEQUENCES;
+CREATE PUBLICATION all_sequences FOR ALL SEQUENCES;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 0f3f86b..a92af54 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1851,7 +1851,7 @@ describeOneTableDetails(const char *schemaname,
}
PQclear(result);
- /* print any publications */
+ /* Print any publications */
if (pset.sversion >= 18)
{
int tuples = 0;
@@ -1867,8 +1867,8 @@ describeOneTableDetails(const char *schemaname,
if (!result)
goto error_return;
- tuples = PQntuples(result);
/* Might be an empty set - that's ok */
+ tuples = PQntuples(result);
if (tuples > 0)
{
printTableAddFooter(&cont, _("Publications:"));
diff --git a/src/test/regress/expected/publication.out
b/src/test/regress/expected/publication.out
index 3ea2224..6c573a1 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -259,7 +259,7 @@ Publications:
SET client_min_messages = 'ERROR';
CREATE PUBLICATION regress_pub_forallsequences2 FOR ALL SEQUENCES;
RESET client_min_messages;
--- Check describe sequence lists both the publications
+-- check that describe sequence lists all publications the sequence belongs to
\d+ pub_test.regress_pub_seq1
Sequence "pub_test.regress_pub_seq1"
Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
diff --git a/src/test/regress/sql/publication.sql
b/src/test/regress/sql/publication.sql
index 8d553ed..ac77fe4 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -134,7 +134,7 @@ SET client_min_messages = 'ERROR';
CREATE PUBLICATION regress_pub_forallsequences2 FOR ALL SEQUENCES;
RESET client_min_messages;
--- Check describe sequence lists both the publications
+-- check that describe sequence lists all publications the sequence belongs to
\d+ pub_test.regress_pub_seq1
--- FOR ALL specifying both TABLES and SEQUENCES
Re: Logical Replication of sequences
On Tue, 16 Jul 2024 at 06:00, Peter Smith wrote: > > Hi, > > I was reading back through this thread to find out how the proposed new > command for refreshing sequences, came about. The patch 0705 introduces a > new command syntax for ALTER SUBSCRIPTION ... REFRESH SEQUENCES > > So now there are 2 forms of subscription refresh. > > #1. ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( refresh_option [= > value] [, ... ] ) ] This is correct. > #2. ALTER SUBSCRIPTION name REFRESH SEQUENCES This is not correct, it is actually "ALTER SUBSCRIPTION name REFRESH PUBLICATION SEQUENCES" > > > IMO, that separation seems complicated. It leaves many questions like: > * It causes a bit of initial confusion. e.g. When I saw the REFRESH SEQUENCES > I first assumed that was needed because sequences were not covered by the > existing REFRESH PUBLICATION > * Why wasn't command #2 called ALTER SUBSCRIPTION REFRESH PUBLICATION > SEQUENCES? E.g. missing keyword PUBLICATION. It seems inconsistent. This is not correct, the existing implementation uses the key word PUBLICATION, the actual syntax is: "ALTER SUBSCRIPTION name REFRESH PUBLICATION SEQUENCES" > * I expect sequence values can become stale pretty much immediately after > command #1, so the user will want to use command #2 anyway... Yes > * ... but if command #2 also does add/remove changed sequences same as > command #1 then what benefit was there of having the command #1 for sequences? > * There is a separation of sequences (from tables) in command #2 but there is > no separation currently possible in command #1. It seemed inconsistent. This can be enhanced if required. It is not included as of now because I'm not sure if there is such a use case in case of tables. > ~~~ > > IIUC some of the goals I saw in the thread are to: > * provide a way to fetch and refresh sequences that also keeps behaviors > (e.g. copy_data etc.) consistent with the refresh of subscription tables > * provide a way to fetch and refresh *only* sequences > > I felt you could just enhance the existing refresh command syntax (command > #1), instead of introducing a new one it would be simpler and it would still > meet those same objectives. > > Synopsis: > ALTER SUBSCRIPTION name REFRESH PUBLICATION [TABLES | SEQUENCES | ALL] [ WITH > ( refresh_option [= value] [, ... ] ) ] > > My only change is the introduction of the optional "[TABLES | SEQUENCES | > ALL]" clause. > > I believe that can do everything your current patch does, plus more: > * Can refresh *only* TABLES if that is what you want (current patch 0705 > cannot do this) > * Can refresh *only* SEQUENCES (same as current patch 0705 command #2) > * Has better integration with refresh options like "copy_data" (current patch > 0705 command #2 doesn't have options) > * Existing REFRESH PUBLICATION syntax still works as-is. You can decide later > what is PG18 default if the "[TABLES | SEQUENCES | ALL]" is omitted. > > ~~~ > > More examples using proposed syntax. > > ex1. > ALTER SUBSCRIPTION sub REFRESH PUBLICATION TABLES WITH (copy_data = false) > - same as PG17 functionality for ALTER SUBSCRIPTION sub REFRESH PUBLICATION > WITH (copy_data = false) > > ex2. > ALTER SUBSCRIPTION sub REFRESH PUBLICATION TABLES WITH (copy_data = true) > - same as PG17 functionality for ALTER SUBSCRIPTION sub REFRESH PUBLICATION > WITH (copy_data = true) > > ex3. > ALTER SUBSCRIPTION sub REFRESH PUBLICATION SEQUENCES WITH (copy data = false) > - this adds/removes only sequences to pg_subscription_rel but doesn't update > their sequence values > > ex4. > ALTER SUBSCRIPTION sub REFRESH PUBLICATION SEQUENCES WITH (copy data = true) > - this adds/removes only sequences to pg_subscription_rel and also updates > all sequence values. > - this is equivalent behaviour of what your current 0705 patch is doing for > command #2, ALTER SUBSCRIPTION sub REFRESH SEQUENCES > > ex5. > ALTER SUBSCRIPTION sub REFRESH PUBLICATION ALL WITH (copy_data = false) > - this is equivalent behaviour of what your current 0705 patch is doing for > command #1, ALTER SUBSCRIPTION sub REFRESH PUBLICATION WITH (copy_data = > false) > > ex6. > ALTER SUBSCRIPTION sub REFRESH PUBLICATION ALL WITH (copy_data = true) > - this adds/removes tables and sequences and updates all table initial data > sequence values.- I think it is equivalent to your current 0705 patch doing > command #1 ALTER SUBSCRIPTION sub REFRESH PUBLICATION WITH (copy_data = > true), followed by another command #2 ALTER SUBSCRIPTION sub REFRESH SEQUENCES > > ex7. > ALTER SUBSCRIPTION sub REFRESH PUBLICATION SEQUENCES > - Because default copy_data is true you do not need to specify options, so > this is the same behaviour as your current 0705 patch command #2, ALTER > SUBSCRIPTION sub REFRESH SEQUENCES. I felt ex:4 is equivalent to command #2 "ALTER SUBSCRIPTION name REFRESH PUBLICATION SEQUENCES" and ex:3 just updates the pg_subscription_rel. But I'm not seeing an equivalent for "ALTER SUBS
Re: Logical Replication of sequences
Hi, here are some review comments for patch v20240720-0003. This review is a WIP. This post is only about the docs (*.sgml) of patch 0003. == doc/src/sgml/ref/alter_subscription.sgml 1. REFRESH PUBLICATION and copy_data nitpicks: - IMO the "synchronize the sequence data" info was misleading because synchronization should only occur when copy_data=true. - I also felt it was strange to mention pg_subscription_rel for sequences, but not for tables. I modified this part too. - Then I moved the information about re/synchronization of sequences into the "copy_data" part. - And added another link to ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES Anyway, in summary, I have updated this page quite a lot according to my understanding. Please take a look at the attached nitpick for my suggestions. nitpick - /The supported options are:/The only supported option is:/ ~~~ 2. REFRESH PUBLICATION SEQUENCES nitpick - tweaked the wording nitpicK - typo /syncronizes/synchronizes/ == 3. catalogs.sgml IMO something is missing in Section "1.55. pg_subscription_rel". Currently, this page only talks of relations/tables, but I think it should mention "sequences" here too, particularly since now we are linking to here from ALTER SUBSCRIPTION when talking about sequences. == 99. Please see the attached diffs patch which implements any nitpicks mentioned above. == Kind Regards, Peter Smith. Fujitsu Australia diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml index ba8c2b1..666d9b0 100644 --- a/doc/src/sgml/ref/alter_subscription.sgml +++ b/doc/src/sgml/ref/alter_subscription.sgml @@ -153,7 +153,7 @@ ALTER SUBSCRIPTION name RENAME TO < REFRESH PUBLICATION - Fetch missing table information from publisher. This will start + Fetch missing table information from the publisher. This will start replication of tables that were added to the subscribed-to publications since CREATE SUBSCRIPTION or @@ -161,29 +161,26 @@ ALTER SUBSCRIPTION name RENAME TO < - It also fetches the missing sequence information from the publisher and - synchronize the sequence data for newly added sequences with the - publisher. This will start synchronizing of sequences that were added to - the subscribed-to publications since - CREATE SUBSCRIPTION or the last invocation of - REFRESH PUBLICATION. Additionally, it will remove any - sequences that are no longer part of the publication from the - pg_subscription_rel - system catalog. Sequences that have already been synchronized will not be - re-synchronized. + Also, fetch missing sequence information from the publisher. + + + + The system catalog pg_subscription_rel + is updated to record all tables and sequences known to the subscription, + that are still part of the publication. refresh_option specifies additional options for the - refresh operation. The supported options are: + refresh operation. The only supported option is: copy_data (boolean) - Specifies whether to copy pre-existing data for tables and sequences - in the publications that are being subscribed to when the replication + Specifies whether to copy pre-existing data for tables and synchronize + sequences in the publications that are being subscribed to when the replication starts. The default is true. @@ -191,6 +188,11 @@ ALTER SUBSCRIPTION name RENAME TO < filter WHERE clause has since been modified. + Previously subscribed sequences are not re-synchronized. To do that, + see + ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES + + See for details of how copy_data = true can interact with the origin @@ -212,11 +214,11 @@ ALTER SUBSCRIPTION name RENAME TO < REFRESH PUBLICATION SEQUENCES - Fetch missing sequences information from publisher and re-synchronize the + Fetch missing sequence information from the publisher, then re-synchronize sequence data with the publisher. Unlike ALTER SUBSCRIPTION ... REFRESH PUBLICATION which - only syncronizes the newly added sequences, this option will also - re-synchronize the sequence data for sequences that were previously added. + only synchronizes newly added sequences, REFRESH PUBLICATION SEQUENCES + will re-synchronize the sequence data for all subscribed sequences.
Re: Logical Replication of sequences
On Wed, Jul 24, 2024 at 9:17 AM Peter Smith wrote:
>
I had a look at patches v20240720* (considering these as the latest
one) and tried to do some basic testing (WIP). Few comments:
1)
I see 'last_value' is updated wrongly after create-sub. Steps:
---
pub:
CREATE SEQUENCE myseq0 INCREMENT 5 START 100;
SELECT nextval('myseq0');
SELECT nextval('myseq0');
--last_value on pub is 105
select * from pg_sequences;
create publication pub1 for all tables, sequences;
Sub:
CREATE SEQUENCE myseq0 INCREMENT 5 START 100;
create subscription sub1 connection 'dbname=postgres host=localhost
user=shveta port=5433' publication pub1;
--check 'r' state is reached
select pc.relname, pr.srsubstate, pr.srsublsn from pg_subscription_rel
pr, pg_class pc where (pr.srrelid = pc.oid);
--check 'last_value', it shows some random value as 136
select * from pg_sequences;
---
2)
I can use 'for all sequences' only with 'for all tables' and can not
use it with the below. Shouldn't it be allowed?
create publication pub2 for tables in schema public, for all sequences;
create publication pub2 for table t1, for all sequences;
3)
preprocess_pub_all_objtype_list():
Do we need 'alltables_specified' and 'allsequences_specified' ? Can't
we make a repetition check using *alltables and *allsequences?
4) patch02's commit msg says : 'Additionally, a new system view,
pg_publication_sequences, has been introduced'
But it is not part of patch002.
thanks
Shveta
Re: Logical Replication of sequences
On Tue, 23 Jul 2024 at 11:03, Peter Smith wrote: > > Here are some review comments for patch v20240720-0002. > > == > 1. Commit message: > > 1a. > The commit message is stale. It is still referring to functions and > views that have been moved to patch 0003. Modified > 1b. > "ALL SEQUENCES" is not a command. It is a clause of the CREATE > PUBLICATION command. Modified > src/bin/psql/describe.c > > 2. describeOneTableDetails > > Although it's not the fault of this patch, this patch propagates the > confusion of 'result' versus 'res'. Basically, I did not understand > the need for the variable 'result'. There is already a "PGResult > *res", and unless I am mistaken we can just keep re-using that instead > of introducing a 2nd variable having almost the same name and purpose. This is intentional, we cannot clear res as it will be used in many places of printTable like in printTable->print_aligned_text->pg_wcssize which was earlier stored from printTableAddCell calls. The rest of the nitpicks comments were merged. The v20240724 version patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm1uncevCSMqo5Nk%3DtqqV_o3KNH_jwp8URiGop_nPC8BTg%40mail.gmail.com Regards, Vignesh
Re: Logical Replication of sequences
On Wed, Jul 24, 2024 at 11:52 AM shveta malik wrote:
>
> On Wed, Jul 24, 2024 at 9:17 AM Peter Smith wrote:
> >
>
> I had a look at patches v20240720* (considering these as the latest
> one) and tried to do some basic testing (WIP). Few comments:
>
> 1)
> I see 'last_value' is updated wrongly after create-sub. Steps:
>
> ---
> pub:
> CREATE SEQUENCE myseq0 INCREMENT 5 START 100;
> SELECT nextval('myseq0');
> SELECT nextval('myseq0');
> --last_value on pub is 105
> select * from pg_sequences;
> create publication pub1 for all tables, sequences;
>
> Sub:
> CREATE SEQUENCE myseq0 INCREMENT 5 START 100;
> create subscription sub1 connection 'dbname=postgres host=localhost
> user=shveta port=5433' publication pub1;
>
> --check 'r' state is reached
> select pc.relname, pr.srsubstate, pr.srsublsn from pg_subscription_rel
> pr, pg_class pc where (pr.srrelid = pc.oid);
>
> --check 'last_value', it shows some random value as 136
> select * from pg_sequences;
Okay, I see that in fetch_remote_sequence_data(), we are inserting
'last_value + log_cnt' fetched from remote as 'last_val' on subscriber
and thus leading to above behaviour. I did not understand why this is
done? This may result into issue when we insert data into a table with
identity column on subscriber (whose internal sequence is replicated);
the identity column in this case will end up having wrong value.
thanks
Shveta
Re: Logical Replication of sequences
On Thu, Jul 25, 2024 at 9:06 AM vignesh C wrote:
>
> The attached v20240725 version patch has the changes for the same.
Thank You for addressing the comments. Please review below issues:
1) Sub ahead of pub due to wrong initial sync of last_value for
non-incremented sequences. Steps at [1]
2) Sequence's min value is not honored on sub during replication. Steps at [2]
[1]:
---
on PUB:
CREATE SEQUENCE myseq001 INCREMENT 5 START 100;
SELECT * from pg_sequences; -->shows last_val as NULL
on SUB:
CREATE SEQUENCE myseq001 INCREMENT 5 START 100;
SELECT * from pg_sequences; -->correctly shows last_val as NULL
ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION SEQUENCES;
SELECT * from pg_sequences; -->wrongly updates last_val to 100; it is
still NULL on Pub.
Thus , SELECT nextval('myseq001') on pub gives 100, while on sub gives 105.
---
[2]:
---
Pub:
CREATE SEQUENCE myseq0 INCREMENT 5 START 10;
SELECT * from pg_sequences;
Sub:
CREATE SEQUENCE myseq0 INCREMENT 5 MINVALUE 100;
Pub:
SELECT nextval('myseq0');
SELECT nextval('myseq0');
Sub:
ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION SEQUENCES;
--check 'last_value', it is 15 while min_value is 100
SELECT * from pg_sequences;
---
thanks
Shveta
Re: Logical Replication of sequences
Hi, here are more review comments for patch v20240720-0003.
==
src/backend/catalog/pg_subscription.c
(Numbers are starting at #4 because this is a continuation of the docs review)
4. GetSubscriptionRelations
nitpick - rearranged the function header comment
~
5.
TBH, I'm thinking that just passing 2 parameters:
- bool get_tables
- bool get_sequences
where one or both can be true, would have resulted in simpler code,
instead of introducing this new enum SubscriptionRelKind.
~
6.
The 'not_all_relations' parameter/logic feels really awkward. IMO it
needs a better name and reverse the meaning to remove all the "nots".
For example, commenting it and calling it like below could be much simpler.
'all_relations'
If returning sequences, if all_relations=true get all sequences,
otherwise only get sequences that are in 'init' state.
If returning tables, if all_relation=true get all tables, otherwise
only get tables that have not reached 'READY' state.
==
src/backend/commands/subscriptioncmds.c
AlterSubscription_refresh:
nitpick - this function comment is difficult to understand. I've
rearranged it a bit but it could still do with some further
improvement.
nitpick - move some code comments
nitpick - I adjusted the "stop worker" comment slightly. Please check
it is still correct.
nitpick - add a blank line
~
7.
The logic seems over-complicated. For example, why is the sequence
list *always* fetched, but the tables list is only sometimes fetched?
Furthermore, this 'refresh_all_sequences' parameter seems to have a
strange interference with tables (e.g. even though it is possible to
refresh all tables and sequences at the same time). It is as if the
meaning is 'refresh_publication_sequences' yet it is not called that
(???)
These gripes may be related to my other thread [1] about the new ALTER
syntax. (I feel that there should be the ability to refresh ALL TABLES
or ALL SEQUENCES independently if the user wants to). IIUC, it would
simplify this function logic as well as being more flexible. Anyway, I
will leave the discussion about syntax to that other thread.
~
8.
+ if (relkind != RELKIND_SEQUENCE)
+ logicalrep_worker_stop(sub->oid, relid);
/*
* For READY state, we would have already dropped the
* tablesync origin.
*/
- if (state != SUBREL_STATE_READY)
+ if (state != SUBREL_STATE_READY && relkind != RELKIND_SEQUENCE)
It might be better to have a single "if (relkind != RELKIND_SEQUENCE)"
here and combine both of these codes under that.
~
9.
ereport(DEBUG1,
- (errmsg_internal("table \"%s.%s\" removed from subscription \"%s\"",
+ (errmsg_internal("%s \"%s.%s\" removed from subscription \"%s\"",
+ get_namespace_name(get_rel_namespace(relid)),
+ get_rel_name(relid),
+ sub->name,
+ get_rel_relkind(relid) == RELKIND_SEQUENCE ? "sequence" : "table")));
IIUC prior conDitions mean get_rel_relkind(relid) == RELKIND_SEQUENCE
will be impossible here.
~~~
10. AlterSubscription
+ PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ...
REFRESH PUBLICATION SEQUENCES");
IIUC the docs page for ALTER SUBSCRIPTION was missing this information
about "REFRESH PUBLICATION SEQUENCES" in transactions. Docs need more
updates.
==
src/backend/replication/logical/launcher.c
logicalrep_worker_find:
nitpick - tweak comment to say "or" instead of "and"
~~~
11.
+/*
+ * Return the pid of the apply worker for one that matches given
+ * subscription id.
+ */
+static LogicalRepWorker *
+logicalrep_apply_worker_find(Oid subid, bool only_running)
The function comment is wrong. This is not returning a PID.
~~~
12.
+ if (is_sequencesync_worker)
+ Assert(!OidIsValid(relid));
Should we the Assert to something more like:
Assert(!is_sequencesync_worker || !OidIsValid(relid));
Otherwise, in NODEBUG current code will compile into an empty
condition statement, which is a bit odd.
~~~
logicalrep_seqsyncworker_failuretime:
nitpick - tweak function comment
nitpick - add blank line
==
.../replication/logical/sequencesync.c
13. fetch_remote_sequence_data
The "current state" mentioned in the function comment is a bit vague.
Can't tell from this comment what it is returning without looking
deeper into the function code.
~
nitpick - typo "scenarios" in comment
~~~
copy_sequence:
nitpick - typo "withe" in function comment
nitpick - typo /retreived/retrieved/
nitpick - add/remove blank lines
~~~
LogicalRepSyncSequences:
nitpick - move a comment.
nitpick - remove blank line
14.
+ /*
+ * Verify whether the current batch of sequences is synchronized or if
+ * there are no remaining sequences to synchronize.
+ */
+ if curr_seq + 1) % MAX_SEQUENCES_SYNC_PER_BATCH) == 0) ||
+ (curr_seq + 1) == seq_count)
All this "curr_seq + 1" maths seems unnecessarily tricky. Can't we
just increment the cur_seq? before this calculation?
~
nitpick - simplify the comment about batching
nitpick - added a comment to the commit
==
src/backend/replication/logical/tablesync.c
finish_sync_worker:
nitpick - added an A
Re: Logical Replication of sequences
On Thu, Jul 25, 2024 at 12:08 PM shveta malik wrote:
>
> On Thu, Jul 25, 2024 at 9:06 AM vignesh C wrote:
> >
> > The attached v20240725 version patch has the changes for the same.
>
> Thank You for addressing the comments. Please review below issues:
>
> 1) Sub ahead of pub due to wrong initial sync of last_value for
> non-incremented sequences. Steps at [1]
> 2) Sequence's min value is not honored on sub during replication. Steps at [2]
One more issue:
3) Sequence datatype's range is not honored on sub during
replication, while it is honored for tables.
Behaviour for tables:
-
Pub: create table tab1( i integer);
Sub: create table tab1( i smallint);
Pub: insert into tab1 values(generate_series(1, 32768));
Error on sub:
2024-07-25 10:38:06.446 IST [178680] ERROR: value "32768" is out of
range for type smallint
-
Behaviour for sequences:
-
Pub:
CREATE SEQUENCE myseq_i as integer INCREMENT 1 START 1;
Sub:
CREATE SEQUENCE myseq_i as smallint INCREMENT 1 START 1;
Pub:
SELECT nextval('myseq_i');
SELECT nextval('myseq_i');
SELECT nextval('myseq_i');
SELECT nextval('myseq_i');
SELECT nextval('myseq_i'); -->brings value to 40001
Sub:
ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION SEQUENCES;
SELECT * from pg_sequences; -->last_val reached till 40001, while the
range is till 32767.
thanks
Shveta
Re: Logical Replication of sequences
Here are some review comments for latest patch v20240725-0002
==
doc/src/sgml/ref/create_publication.sgml
nitpick - tweak to the description of the example.
==
src/backend/parser/gram.y
preprocess_pub_all_objtype_list:
nitpick - typo "allbjects_list"
nitpick - reword function header
nitpick - /alltables/all_tables/
nitpick - /allsequences/all_sequences/
nitpick - I think code is safe as-is because makeNode internally does
palloc0, but OTOH adding Assert would be nicer just to remove any
doubts.
==
src/bin/psql/describe.c
1.
+ /* Print any publications */
+ if (pset.sversion >= 18)
+ {
+ int tuples = 0;
No need to assign value 0 here, because this will be unconditionally
assigned before use anyway.
2. describePublications
has_pubviaroot = (pset.sversion >= 13);
+ has_pubsequence = (pset.sversion >= 18000);
That's a bug! Should be 18, not 18000.
==
And, please see the attached diffs patch, which implements the
nitpicks mentioned above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/create_publication.sgml
b/doc/src/sgml/ref/create_publication.sgml
index 7dcfe37..783874f 100644
--- a/doc/src/sgml/ref/create_publication.sgml
+++ b/doc/src/sgml/ref/create_publication.sgml
@@ -420,7 +420,7 @@ CREATE PUBLICATION users_filtered FOR TABLE users (user_id,
firstname);
- Create a publication that synchronizes all the sequences:
+ Create a publication that publishes all sequences for synchronization:
CREATE PUBLICATION all_sequences FOR ALL SEQUENCES;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 585f61e..9b3cad1 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -215,9 +215,9 @@ static void processCASbits(int cas_bits, int location,
const char *constrType,
static PartitionStrategy parsePartitionStrategy(char *strategy);
static void preprocess_pubobj_list(List *pubobjspec_list,
core_yyscan_t yyscanner);
-static void preprocess_pub_all_objtype_list(List *allbjects_list,
-
bool *alltables,
-
bool *allsequences,
+static void preprocess_pub_all_objtype_list(List *all_objects_list,
+
bool *all_tables,
+
bool *all_sequences,
core_yyscan_t yyscanner);
static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node
*query);
@@ -19440,39 +19440,42 @@ parsePartitionStrategy(char *strategy)
}
/*
- * Process all_objects_list to check if the options have been specified more
than
- * once and set alltables/allsequences.
+ * Process all_objects_list to set all_tables/all_sequences.
+ * Also, checks if the pub_object_type has been specified more than once.
*/
static void
-preprocess_pub_all_objtype_list(List *all_objects_list, bool *alltables,
- bool
*allsequences, core_yyscan_t yyscanner)
+preprocess_pub_all_objtype_list(List *all_objects_list, bool *all_tables,
+ bool
*all_sequences, core_yyscan_t yyscanner)
{
if (!all_objects_list)
return;
+ Assert(all_tables && *all_tables == false);
+ Assert(all_sequences && *all_sequences == false);
+
foreach_ptr(PublicationAllObjSpec, obj, all_objects_list)
{
if (obj->pubobjtype == PUBLICATION_ALL_TABLES)
{
- if (*alltables)
+ if (*all_tables)
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid publication
object list"),
errdetail("TABLES can be
specified only once."),
parser_errposition(obj->location));
- *alltables = true;
+ *all_tables = true;
}
else if (obj->pubobjtype == PUBLICATION_ALL_SEQUENCES)
{
- if (*allsequences)
+ if (*all_sequences)
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid publication
object list"),
errdetail("SEQUENCES can be
specified only once."),
Re: Logical Replication of sequences
Hi Vignesh, There are still pending changes from my previous review of the 0720-0003 patch [1], but here are some new review comments for your latest patch v20240525-0003. == doc/src/sgml/catalogs.sgml nitpick - fix plurals and tweak the description. ~~~ 1. - This catalog only contains tables known to the subscription after running - either CREATE SUBSCRIPTION or - ALTER SUBSCRIPTION ... REFRESH + This catalog only contains tables and sequences known to the subscription + after running either + CREATE SUBSCRIPTION + or ALTER SUBSCRIPTION ... REFRESH PUBLICATION. Shouldn't this mention "REFRESH PUBLICATION SEQUENCES" too? == src/backend/commands/sequence.c SetSequenceLastValue: nitpick - maybe change: /log_cnt/new_log_cnt/ for consistency with the other parameter, and to emphasise the old log_cnt is overwritten == src/backend/replication/logical/sequencesync.c 2. +/* + * fetch_remote_sequence_data + * + * Fetch sequence data (current state) from the remote node, including + * the latest sequence value from the publisher and the Page LSN for the + * sequence. + */ +static int64 +fetch_remote_sequence_data(WalReceiverConn *conn, Oid remoteid, +int64 *log_cnt, XLogRecPtr *lsn) 2a. Now you are also returning the 'log_cnt' but that is not mentioned by the function comment. ~ 2b. Is it better to name these returned by-ref ptrs like 'ret_log_cnt', and 'ret_lsn' to emphasise they are output variables? YMMV. ~~~ 3. + /* Process the sequence. */ + slot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple); + while (tuplestore_gettupleslot(res->tuplestore, true, false, slot)) This will have one-and-only-one tuple for the discovered sequence, won't it? So, why is this a while loop? == src/include/commands/sequence.h nitpick - maybe change: /log_cnt/new_log_cnt/ (same as earlier in this post) == src/test/subscription/t/034_sequences.pl 4. Q. Should we be suspicious that log_cnt changes from '32' to '31', or is there a valid explanation? It smells like some calculation is off-by-one, but without debugging I can't tell if it is right or wrong. == Please also see the attached diffs patch, which implements the nitpicks mentioned above. == [1] 0720-0003 review - https://www.postgresql.org/message-id/CAHut%2BPsfsfzyBrmo8E43qFMp9_bmen2tuCsNYN8sX%3Dfa86SdfA%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 19d04b1..dcd0b98 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -8102,8 +8102,8 @@ SCRAM-SHA-256$:&l - The catalog pg_subscription_rel contains the - state for each replicated tables and sequences in each subscription. This + The catalog pg_subscription_rel stores the + state of each replicated table and sequence for each subscription. This is a many-to-many mapping. diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c index a3e7c79..f292fbc 100644 --- a/src/backend/commands/sequence.c +++ b/src/backend/commands/sequence.c @@ -342,7 +342,7 @@ ResetSequence(Oid seq_relid) * logical replication. */ void -SetSequenceLastValue(Oid seq_relid, int64 new_last_value, int64 log_cnt) +SetSequenceLastValue(Oid seq_relid, int64 new_last_value, int64 new_log_cnt) { SeqTableelm; Relationseqrel; @@ -370,7 +370,7 @@ SetSequenceLastValue(Oid seq_relid, int64 new_last_value, int64 log_cnt) seq->last_value = new_last_value; seq->is_called = true; - seq->log_cnt = log_cnt; + seq->log_cnt = new_log_cnt; MarkBufferDirty(buf); diff --git a/src/include/commands/sequence.h b/src/include/commands/sequence.h index a302890..4c6aee0 100644 --- a/src/include/commands/sequence.h +++ b/src/include/commands/sequence.h @@ -60,7 +60,7 @@ extern ObjectAddress AlterSequence(ParseState *pstate, AlterSeqStmt *stmt); extern void SequenceChangePersistence(Oid relid, char newrelpersistence); extern void DeleteSequenceTuple(Oid relid); extern void ResetSequence(Oid seq_relid); -extern void SetSequenceLastValue(Oid seq_relid, int64 new_last_value, int64 log_cnt); +extern void SetSequenceLastValue(Oid seq_relid, int64 new_last_value, int64 new_log_cnt); extern void ResetSequenceCaches(void); extern void seq_redo(XLogReaderState *record);
Re: Logical Replication of sequences
On Fri, 26 Jul 2024 at 08:04, Peter Smith wrote:
>
> Here are some review comments for latest patch v20240725-0002
>
> ==
> doc/src/sgml/ref/create_publication.sgml
>
> nitpick - tweak to the description of the example.
>
> ==
> src/backend/parser/gram.y
>
> preprocess_pub_all_objtype_list:
> nitpick - typo "allbjects_list"
> nitpick - reword function header
> nitpick - /alltables/all_tables/
> nitpick - /allsequences/all_sequences/
> nitpick - I think code is safe as-is because makeNode internally does
> palloc0, but OTOH adding Assert would be nicer just to remove any
> doubts.
>
> ==
> src/bin/psql/describe.c
>
> 1.
> + /* Print any publications */
> + if (pset.sversion >= 18)
> + {
> + int tuples = 0;
>
> No need to assign value 0 here, because this will be unconditionally
> assigned before use anyway.
>
>
>
> 2. describePublications
>
> has_pubviaroot = (pset.sversion >= 13);
> + has_pubsequence = (pset.sversion >= 18000);
>
> That's a bug! Should be 18, not 18000.
>
> ==
>
> And, please see the attached diffs patch, which implements the
> nitpicks mentioned above.
These are handled in the v20240729 version attached at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm3SucGGLe-B-a_aqWNWQZ-yfxFTiAA0JyP-SwX4jq9Y3A%40mail.gmail.com
Regards,
Vignesh
Re: Logical Replication of sequences
On Fri, 26 Jul 2024 at 11:46, Peter Smith wrote: > > Hi Vignesh, > > There are still pending changes from my previous review of the > 0720-0003 patch [1], but here are some new review comments for your > latest patch v20240525-0003. > 2b. > Is it better to name these returned by-ref ptrs like 'ret_log_cnt', > and 'ret_lsn' to emphasise they are output variables? YMMV. I felt this is ok as we have mentioned in function header too > == > src/test/subscription/t/034_sequences.pl > > 4. > Q. Should we be suspicious that log_cnt changes from '32' to '31', or > is there a valid explanation? It smells like some calculation is > off-by-one, but without debugging I can't tell if it is right or > wrong. It works like this: for every 33 nextval we will get log_cnt as 0. So for 33 * 6(198) log_cnt will be 0, then for 199 log_cnt will be 32 and for 200 log_cnt will be 31. This pattern repeats, so this is ok. These are handled in the v20240729 version attached at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm3SucGGLe-B-a_aqWNWQZ-yfxFTiAA0JyP-SwX4jq9Y3A%40mail.gmail.com Regards, Vignesh
Re: Logical Replication of sequences
On Thu, 25 Jul 2024 at 15:41, shveta malik wrote:
>
> On Thu, Jul 25, 2024 at 12:08 PM shveta malik wrote:
> >
> > On Thu, Jul 25, 2024 at 9:06 AM vignesh C wrote:
> > >
> > > The attached v20240725 version patch has the changes for the same.
> >
> > Thank You for addressing the comments. Please review below issues:
> >
> > 1) Sub ahead of pub due to wrong initial sync of last_value for
> > non-incremented sequences. Steps at [1]
> > 2) Sequence's min value is not honored on sub during replication. Steps at
> > [2]
>
> One more issue:
> 3) Sequence datatype's range is not honored on sub during
> replication, while it is honored for tables.
>
>
> Behaviour for tables:
> -
> Pub: create table tab1( i integer);
> Sub: create table tab1( i smallint);
>
> Pub: insert into tab1 values(generate_series(1, 32768));
>
> Error on sub:
> 2024-07-25 10:38:06.446 IST [178680] ERROR: value "32768" is out of
> range for type smallint
>
> -
> Behaviour for sequences:
> -
>
> Pub:
> CREATE SEQUENCE myseq_i as integer INCREMENT 1 START 1;
>
> Sub:
> CREATE SEQUENCE myseq_i as smallint INCREMENT 1 START 1;
>
> Pub:
> SELECT nextval('myseq_i');
> SELECT nextval('myseq_i');
> SELECT nextval('myseq_i');
> SELECT nextval('myseq_i');
> SELECT nextval('myseq_i'); -->brings value to 40001
>
> Sub:
> ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION SEQUENCES;
> SELECT * from pg_sequences; -->last_val reached till 40001, while the
> range is till 32767.
This issue is addressed in the v20240730_2 version patch attached at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm3%2BXzHAbgyn8gmbBLK5goyv_uyGgHEsTQxRZ8bVk6nAEg%40mail.gmail.com
Regards,
Vignesh
Re: Logical Replication of sequences
Hi Vignesh,
Here are my review comments for your latest 0730_2* patches.
Patch v20240730_2-0001 looks good to me.
Patch v20240730_2-0002 looks good to me.
My comments for the v20240730_2-0003 patch are below:
//
GENERAL
1. Inconsistent terms
I've noticed there are many variations of how the sequence sync worker is known:
- "sequencesync worker"
- "sequence sync worker"
- "sequence-sync worker"
- "sequence synchronization worker"
- more?
We must settle on some standardized name.
AFAICT we generally use "table synchronization worker" in the docs,
and "tablesync worker" in the code and comments. IMO, we should do
same as that for sequences -- e.g. "sequence synchronization worker"
in the docs, and "sequencesync worker" in the code and comments.
==
doc/src/sgml/catalogs.sgml
nitpick - the links should jump directly to REFRESH PUBLICATION or
REFRESH PUBLICATION SEQUENCES. Currently they go to the top of the
ALTER SUBSCRIPTION page which is not as useful.
==
src/backend/commands/sequence.c
do_setval:
nitpick - minor wording in the function header
nitpick - change some param names to more closely resemble the fields
they get assigned to (/logcnt/log_cnt/, /iscalled/is_called/)
~
2.
seq->is_called = iscalled;
- seq->log_cnt = 0;
+ seq->log_cnt = (logcnt == SEQ_LOG_CNT_INVALID) ? 0: logcnt;
The logic here for SEQ_LOG_CNT_INVALID seemed strange. Why not just
#define SEQ_LOG_CNT_INVALID as 0 in the first place if that is what
you will assign for invalid? Then you won't need to do anything here
except seq->log_cnt = log_cnt;
==
src/backend/catalog/pg_subscription.c
HasSubscriptionRelations:
nitpick - I think the comment "If even a single tuple exists..." is
not quite accurate. e.g. It also has to be the right kind of tuple.
~~
GetSubscriptionRelations:
nitpick - Give more description in the function header about the other
parameters.
nitpick - I felt that a better name for 'all_relations' is all_states.
Because in my mind *all relations* sounds more like when both
'all_tables' and 'all_sequences' are true.
nitpick - IMO add an Assert to be sure something is being fetched.
Assert(get_tables || get_sequences);
nitpick - Rephrase the "skip the tables" and "skip the sequences"
comments to be more aligned with the code condition.
~
3.
- if (not_ready)
+ /* Get the relations that are not in ready state */
+ if (get_tables && !all_relations)
ScanKeyInit(&skey[nkeys++],
Anum_pg_subscription_rel_srsubstate,
BTEqualStrategyNumber, F_CHARNE,
CharGetDatum(SUBREL_STATE_READY));
+ /* Get the sequences that are in init state */
+ else if (get_sequences && !all_relations)
+ ScanKeyInit(&skey[nkeys++],
+ Anum_pg_subscription_rel_srsubstate,
+ BTEqualStrategyNumber, F_CHAREQ,
+ CharGetDatum(SUBREL_STATE_INIT));
This is quite tricky, using multiple flags (get_tables and
get_sequences) in such a way. It might even be a bug -- e.g. Is the
'else' keyword correct? Otherwise, when both get_tables and
get_sequences are true, and all_relations is false, then the sequence
part wouldn't even get executed (???).
==
src/backend/commands/subscriptioncmds.c
CreateSubscription:
nitpick - let's move the 'tables' declaration to be beside the
'sequences' var for consistency. (in passing move other vars too)
nitpick - it's not strictly required for the patch, but let's change
the 'tables' loop to be consistent with the new sequences loop.
~~~
4. AlterSubscription_refresh
My first impression (from the function comment) is that these function
parameters are a bit awkward. For example,
- It says: If 'copy_data' parameter is true, the function will set
the state to "init"; otherwise, it will set the state to "ready".
- It also says: "If 'all_relations' is true, mark all objects with
"init" state..."
Those statements seem to clash. e.g. if copy_data is false but
all_relations is true, then what (???)
~
nitpick - tweak function comment wording.
nitpick - introduce a 'relkind' variable to avoid multiple calls of
get_rel_relkind(relid)
nitpick - use an existing 'relkind' variable instead of calling
get_rel_relkind(relid);
nitpick - add another comment about skipping (for dropping tablesync slots)
~
5.
+ /*
+ * If all the relations should be re-synchronized, then set the
+ * state to init for re-synchronization. This is currently
+ * supported only for sequences.
+ */
+ else if (all_relations)
+ {
+ ereport(DEBUG1,
+ (errmsg_internal("sequence \"%s.%s\" of subscription \"%s\" set to
INIT state",
get_namespace_name(get_rel_namespace(relid)),
get_rel_name(relid),
sub->name)));
+ UpdateSubscriptionRelState(sub->oid, relid, SUBREL_STATE_INIT,
+InvalidXLogRecPtr);
(This is a continuation of my doubts regarding 'all_relations' in the
previous review comment #4 above)
Here are some more questions about it:
~
5a. Why is this an 'else' of the !bsearch? It needs more explanation
what this case means.
~
5b. Along with more description, it might be better to reverse the
!bsearch condition, s
Re: Logical Replication of sequences
On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote: > > On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote: > > > > > > > > On Sat, Jun 8, 2024 at 6:43 PM vignesh C wrote: > >> > >> On Wed, 5 Jun 2024 at 14:11, Amit Kapila wrote: > >> [...] > >> A new catalog table, pg_subscription_seq, has been introduced for > >> mapping subscriptions to sequences. Additionally, the sequence LSN > >> (Log Sequence Number) is stored, facilitating determination of > >> sequence changes occurring before or after the returned sequence > >> state. > > > > > > Can't it be done using pg_depend? It seems a bit excessive unless I'm > > missing > > something. > > We'll require the lsn because the sequence LSN informs the user that > it has been synchronized up to the LSN in pg_subscription_seq. Since > we are not supporting incremental sync, the user will be able to > identify if he should run refresh sequences or not by checking the lsn > of the pg_subscription_seq and the lsn of the sequence(using > pg_sequence_state added) in the publisher. How the user will know from seq's lsn that he needs to run refresh. lsn indicates page_lsn and thus the sequence might advance on pub without changing lsn and thus lsn may look the same on subscriber even though a sequence-refresh is needed. Am I missing something here? thanks Shveta
Re: Logical Replication of sequences
On Sat, 20 Jul 2024 at 20:48, vignesh C wrote:
>
> On Fri, 12 Jul 2024 at 08:22, Peter Smith wrote:
> >
> > Hi Vignesh. Here are the rest of my comments for patch v20240705-0003.
> > ==
> >
> > 8. logicalrep_sequence_sync_worker_find
> >
> > +/*
> > + * Walks the workers array and searches for one that matches given
> > + * subscription id.
> > + *
> > + * We are only interested in the sequence sync worker.
> > + */
> > +LogicalRepWorker *
> > +logicalrep_sequence_sync_worker_find(Oid subid, bool only_running)
> >
> > There are other similar functions for walking the workers array to
> > search for a worker. Instead of having different functions for
> > different cases, wouldn't it be cleaner to combine these into a single
> > function, where you pass a parameter (e.g. a mask of worker types that
> > you are interested in finding)?
This is fixed in the v20240730_2 version attached at [1].
> > 17.
> > Also, where does the number 100 come from? Why not 1000? Why not 10?
> > Why have batching at all? Maybe there should be some comment to
> > describe the reason and the chosen value.
I had run some tests with 10/100 and 1000 sequences per batch for
1 sequences. The results for it:
10 per batch - 4.94 seconds
100 per batch - 4.87 seconds
1000 per batch - 4.53 seconds
There is not much time difference between each of them. Currently, it
is set to 100, which seems fine since it will not generate a lot of
transactions. Additionally, the locks on the sequences will be
periodically released during the commit transaction.
I had used the test from the attached patch by changing
max_sequences_sync_per_batch to 10/100/100 in 035_sequences.pl to
verify this.
[1] -
https://www.postgresql.org/message-id/CALDaNm3%2BXzHAbgyn8gmbBLK5goyv_uyGgHEsTQxRZ8bVk6nAEg%40mail.gmail.com
Regards,
Vignesh
| 1000 seq/batch | 100 seq/batch |10 seq/batch
--||---|-
Exec time | 4.604323149| 4.719184017 |4.945001841
Exec time | 4.518861055| 4.738708973 |4.958534002
Exec time | 4.522281885| 4.873774052 |4.957324982
Exec time | 4.524350882| 4.878502131 |4.943160057
Exec time | 4.531511068| 4.874341965 |4.850522995
Median| 4.524350882| 4.873774052 |4.945001841
From d060bd7903d812904816f0ada004420e2d9f0c21 Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Tue, 30 Jul 2024 11:34:55 +0530
Subject: [PATCH] Performance testing changes.
Performance testing changes.
---
src/backend/replication/logical/launcher.c | 1 +
src/backend/replication/logical/sequencesync.c | 6 +-
src/backend/utils/misc/guc_tables.c| 12 +++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/replication/logicallauncher.h | 2 +
src/test/subscription/t/035_sequences.pl | 142 +
6 files changed, 162 insertions(+), 2 deletions(-)
create mode 100644 src/test/subscription/t/035_sequences.pl
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 04d76e7..2ece21f 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -50,6 +50,7 @@
int max_logical_replication_workers = 4;
int max_sync_workers_per_subscription = 2;
int max_parallel_apply_workers_per_subscription = 2;
+int max_sequences_sync_per_batch = 10;
LogicalRepWorker *MyLogicalRepWorker = NULL;
diff --git a/src/backend/replication/logical/sequencesync.c b/src/backend/replication/logical/sequencesync.c
index fc36bf9..6bcfbdf 100644
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -20,6 +20,7 @@
#include "catalog/pg_subscription_rel.h"
#include "commands/sequence.h"
#include "pgstat.h"
+#include "replication/logicallauncher.h"
#include "replication/logicalworker.h"
#include "replication/worker_internal.h"
#include "utils/acl.h"
@@ -29,6 +30,7 @@
#include "utils/rls.h"
#include "utils/usercontext.h"
+
/*
* fetch_remote_sequence_data
*
@@ -287,11 +289,11 @@ LogicalRepSyncSequences(void)
* Have we reached the end of the current batch of sequences,
* or last remaining sequences to synchronize?
*/
- if (((curr_seq % MAX_SEQUENCES_SYNC_PER_BATCH) == 0) ||
+ if (((curr_seq % max_sequences_sync_per_batch) == 0) ||
curr_seq == seq_count)
{
/* Obtain the starting index of the current batch. */
- int i = (curr_seq - 1) - ((curr_seq - 1) % MAX_SEQUENCES_SYNC_PER_BATCH);
+ int i = (curr_seq - 1) - ((curr_seq - 1) % max_sequences_sync_per_batch);
/* LOG all the sequences synchronized during current batch. */
for (; i < curr_seq; i++)
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 6a623f5..017fb59 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tab
Re: Logical Replication of sequences
Hi Vignesh,
I have a question about the subscriber-side behaviour of currval().
==
AFAIK it is normal for currval() to give error is nextval() has not
yet been called [1]
For example.
test_pub=# create sequence s1;
CREATE SEQUENCE
test_pub=# select * from currval('s1');
2024-08-01 07:42:48.619 AEST [24131] ERROR: currval of sequence "s1"
is not yet defined in this session
2024-08-01 07:42:48.619 AEST [24131] STATEMENT: select * from currval('s1');
ERROR: currval of sequence "s1" is not yet defined in this session
test_pub=# select * from nextval('s1');
nextval
-
1
(1 row)
test_pub=# select * from currval('s1');
currval
-
1
(1 row)
test_pub=#
~~~
OTOH, I was hoping to be able to use currval() at the subscriber=side
to see the current sequence value after issuing ALTER .. REFRESH
PUBLICATION SEQUENCES.
Unfortunately, it has the same behaviour where currval() cannot be
used without nextval(). But, on the subscriber, you probably never
want to do an explicit nextval() independently of the publisher.
Is this currently a bug, or maybe a quirk that should be documented?
For example:
Publisher
==
test_pub=# create sequence s1;
CREATE SEQUENCE
test_pub=# CREATE PUBLICATION pub1 FOR ALL SEQUENCES;
CREATE PUBLICATION
test_pub=# select * from nextval('s1');
nextval
-
1
(1 row)
test_pub=# select * from nextval('s1');
nextval
-
2
(1 row)
test_pub=# select * from nextval('s1');
nextval
-
3
(1 row)
test_pub=#
Subscriber
==
(Notice currval() always gives an error unless nextval() is used prior).
test_sub=# create sequence s1;
CREATE SEQUENCE
test_sub=# CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=test_pub'
PUBLICATION pub1;
2024-08-01 07:51:06.955 AEST [24325] WARNING: subscriptions created
by regression test cases should have names starting with "regress_"
WARNING: subscriptions created by regression test cases should have
names starting with "regress_"
NOTICE: created replication slot "sub1" on publisher
CREATE SUBSCRIPTION
test_sub=# 2024-08-01 07:51:07.023 AEST [4211] LOG: logical
replication apply worker for subscription "sub1" has started
2024-08-01 07:51:07.037 AEST [4213] LOG: logical replication sequence
synchronization worker for subscription "sub1" has started
2024-08-01 07:51:07.063 AEST [4213] LOG: logical replication
synchronization for subscription "sub1", sequence "s1" has finished
2024-08-01 07:51:07.063 AEST [4213] LOG: logical replication sequence
synchronization worker for subscription "sub1" has finished
test_sub=# SELECT * FROM currval('s1');
2024-08-01 07:51:19.688 AEST [24325] ERROR: currval of sequence "s1"
is not yet defined in this session
2024-08-01 07:51:19.688 AEST [24325] STATEMENT: SELECT * FROM currval('s1');
ERROR: currval of sequence "s1" is not yet defined in this session
test_sub=# ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION SEQUENCES;
ALTER SUBSCRIPTION
test_sub=# 2024-08-01 07:51:35.298 AEST [4993] LOG: logical
replication sequence synchronization worker for subscription "sub1"
has started
test_sub=# 2024-08-01 07:51:35.321 AEST [4993] LOG: logical
replication synchronization for subscription "sub1", sequence "s1" has
finished
2024-08-01 07:51:35.321 AEST [4993] LOG: logical replication sequence
synchronization worker for subscription "sub1" has finished
test_sub=#
test_sub=# SELECT * FROM currval('s1');
2024-08-01 07:51:41.438 AEST [24325] ERROR: currval of sequence "s1"
is not yet defined in this session
2024-08-01 07:51:41.438 AEST [24325] STATEMENT: SELECT * FROM currval('s1');
ERROR: currval of sequence "s1" is not yet defined in this session
test_sub=#
test_sub=# SELECT * FROM nextval('s1');
nextval
-
4
(1 row)
test_sub=# SELECT * FROM currval('s1');
currval
-
4
(1 row)
test_sub=#
==
[1] https://www.postgresql.org/docs/current/functions-sequence.html
Kind Regards,
Peter Smith.
Fujitsu Australia.
Re: Logical Replication of sequences
Hi Vignesh,
I noticed that when replicating sequences (using the latest patches
0730_2*) the subscriber-side checks the *existence* of the sequence,
but apparently it is not checking other sequence attributes.
For example, consider:
Publisher: "CREATE SEQUENCE s1 START 1 INCREMENT 2;" should be a
sequence of only odd numbers.
Subscriber: "CREATE SEQUENCE s1 START 2 INCREMENT 2;" should be a
sequence of only even numbers.
Because the names match, currently the patch allows replication of the
s1 sequence. I think that might lead to unexpected results on the
subscriber. IMO it might be safer to report ERROR unless the sequences
match properly (i.e. not just a name check).
Below is a demonstration the problem:
==
Publisher:
==
(publisher sequence is odd numbers)
test_pub=# create sequence s1 start 1 increment 2;
CREATE SEQUENCE
test_pub=# select * from nextval('s1');
nextval
-
1
(1 row)
test_pub=# select * from nextval('s1');
nextval
-
3
(1 row)
test_pub=# select * from nextval('s1');
nextval
-
5
(1 row)
test_pub=# CREATE PUBLICATION pub1 FOR ALL SEQUENCES;
CREATE PUBLICATION
test_pub=#
==
Subscriber:
==
(subscriber sequence is even numbers)
test_sub=# create sequence s1 start 2 increment 2;
CREATE SEQUENCE
test_sub=# SELECT * FROM nextval('s1');
nextval
-
2
(1 row)
test_sub=# SELECT * FROM nextval('s1');
nextval
-
4
(1 row)
test_sub=# SELECT * FROM nextval('s1');
nextval
-
6
(1 row)
test_sub=# CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=test_pub'
PUBLICATION pub1;
2024-08-01 08:43:04.198 AEST [24325] WARNING: subscriptions created
by regression test cases should have names starting with "regress_"
WARNING: subscriptions created by regression test cases should have
names starting with "regress_"
NOTICE: created replication slot "sub1" on publisher
CREATE SUBSCRIPTION
test_sub=# 2024-08-01 08:43:04.294 AEST [26240] LOG: logical
replication apply worker for subscription "sub1" has started
2024-08-01 08:43:04.309 AEST [26244] LOG: logical replication
sequence synchronization worker for subscription "sub1" has started
2024-08-01 08:43:04.323 AEST [26244] LOG: logical replication
synchronization for subscription "sub1", sequence "s1" has finished
2024-08-01 08:43:04.323 AEST [26244] LOG: logical replication
sequence synchronization worker for subscription "sub1" has finished
(after the CREATE SUBSCRIPTION we are getting replicated odd values
from the publisher, even though the subscriber side sequence was
supposed to be even numbers)
test_sub=# SELECT * FROM nextval('s1');
nextval
-
7
(1 row)
test_sub=# SELECT * FROM nextval('s1');
nextval
-
9
(1 row)
test_sub=# SELECT * FROM nextval('s1');
nextval
-
11
(1 row)
(Looking at the description you would expect odd values for this
sequence to be impossible)
test_sub=# \dS+ s1
Sequence "public.s1"
Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+---+-+-+---+-+---
bigint | 2 | 1 | 9223372036854775807 | 2 | no | 1
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Logical Replication of sequences
On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote: > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote: > > > > Thanks for reporting this, these issues are fixed in the attached > > v20240730_2 version patch. > > I was reviewing the design of patch003, and I have a query. Do we need to even start an apply worker and create replication slot when subscription created is for 'sequences only'? IIUC, currently logical replication apply worker is the one launching sequence-sync worker whenever needed. I think it should be the launcher doing this job and thus apply worker may even not be needed for current functionality of sequence sync? Going forward when we implement incremental sync of sequences, then we may have apply worker started but now it is not needed. thanks Shveta
Re: Logical Replication of sequences
On Fri, Aug 2, 2024 at 2:24 PM shveta malik wrote: > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote: > > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote: > > > > > > Thanks for reporting this, these issues are fixed in the attached > > > v20240730_2 version patch. > > > > > I was reviewing the design of patch003, and I have a query. Do we need > to even start an apply worker and create replication slot when > subscription created is for 'sequences only'? IIUC, currently logical > replication apply worker is the one launching sequence-sync worker > whenever needed. I think it should be the launcher doing this job and > thus apply worker may even not be needed for current functionality of > sequence sync? Going forward when we implement incremental sync of > sequences, then we may have apply worker started but now it is not > needed. > Also, can we please mention the state change and 'who does what' atop sequencesync.c file similar to what we have atop tablesync.c file otherwise it is difficult to figure out the flow. thanks Shveta
Re: Logical Replication of sequences
On Thu, 1 Aug 2024 at 03:33, Peter Smith wrote:
>
> Hi Vignesh,
>
> I have a question about the subscriber-side behaviour of currval().
>
> ==
>
> AFAIK it is normal for currval() to give error is nextval() has not
> yet been called [1]
>
> For example.
> test_pub=# create sequence s1;
> CREATE SEQUENCE
> test_pub=# select * from currval('s1');
> 2024-08-01 07:42:48.619 AEST [24131] ERROR: currval of sequence "s1"
> is not yet defined in this session
> 2024-08-01 07:42:48.619 AEST [24131] STATEMENT: select * from currval('s1');
> ERROR: currval of sequence "s1" is not yet defined in this session
> test_pub=# select * from nextval('s1');
> nextval
> -
>1
> (1 row)
>
> test_pub=# select * from currval('s1');
> currval
> -
>1
> (1 row)
>
> test_pub=#
>
> ~~~
>
> OTOH, I was hoping to be able to use currval() at the subscriber=side
> to see the current sequence value after issuing ALTER .. REFRESH
> PUBLICATION SEQUENCES.
>
> Unfortunately, it has the same behaviour where currval() cannot be
> used without nextval(). But, on the subscriber, you probably never
> want to do an explicit nextval() independently of the publisher.
>
> Is this currently a bug, or maybe a quirk that should be documented?
The currval returns the most recent value obtained from the nextval
function for a given sequence within the current session. This
function is specific to the session, meaning it only provides the last
sequence value retrieved during that session. However, if you call
currval before using nextval in the same session, you'll encounter an
error stating "currval of the sequence is not yet defined in this
session." Meaning even in the publisher this value is only visible in
the current session and not in a different session. Alternatively you
can use the following to get the last_value of the sequence: SELECT
last_value FROM sequence_name. I feel this need not be documented as
the similar issue is present in the publisher and there is an "SELECT
last_value FROM sequence_name" to get the last_value.
Regards,
Vignesh
Re: Logical Replication of sequences
On Thu, 1 Aug 2024 at 04:25, Peter Smith wrote:
>
> Hi Vignesh,
>
> I noticed that when replicating sequences (using the latest patches
> 0730_2*) the subscriber-side checks the *existence* of the sequence,
> but apparently it is not checking other sequence attributes.
>
> For example, consider:
>
> Publisher: "CREATE SEQUENCE s1 START 1 INCREMENT 2;" should be a
> sequence of only odd numbers.
> Subscriber: "CREATE SEQUENCE s1 START 2 INCREMENT 2;" should be a
> sequence of only even numbers.
>
> Because the names match, currently the patch allows replication of the
> s1 sequence. I think that might lead to unexpected results on the
> subscriber. IMO it might be safer to report ERROR unless the sequences
> match properly (i.e. not just a name check).
>
> Below is a demonstration the problem:
>
> ==
> Publisher:
> ==
>
> (publisher sequence is odd numbers)
>
> test_pub=# create sequence s1 start 1 increment 2;
> CREATE SEQUENCE
> test_pub=# select * from nextval('s1');
> nextval
> -
>1
> (1 row)
>
> test_pub=# select * from nextval('s1');
> nextval
> -
>3
> (1 row)
>
> test_pub=# select * from nextval('s1');
> nextval
> -
>5
> (1 row)
>
> test_pub=# CREATE PUBLICATION pub1 FOR ALL SEQUENCES;
> CREATE PUBLICATION
> test_pub=#
>
> ==
> Subscriber:
> ==
>
> (subscriber sequence is even numbers)
>
> test_sub=# create sequence s1 start 2 increment 2;
> CREATE SEQUENCE
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
>2
> (1 row)
>
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
>4
> (1 row)
>
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
>6
> (1 row)
>
> test_sub=# CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=test_pub'
> PUBLICATION pub1;
> 2024-08-01 08:43:04.198 AEST [24325] WARNING: subscriptions created
> by regression test cases should have names starting with "regress_"
> WARNING: subscriptions created by regression test cases should have
> names starting with "regress_"
> NOTICE: created replication slot "sub1" on publisher
> CREATE SUBSCRIPTION
> test_sub=# 2024-08-01 08:43:04.294 AEST [26240] LOG: logical
> replication apply worker for subscription "sub1" has started
> 2024-08-01 08:43:04.309 AEST [26244] LOG: logical replication
> sequence synchronization worker for subscription "sub1" has started
> 2024-08-01 08:43:04.323 AEST [26244] LOG: logical replication
> synchronization for subscription "sub1", sequence "s1" has finished
> 2024-08-01 08:43:04.323 AEST [26244] LOG: logical replication
> sequence synchronization worker for subscription "sub1" has finished
>
> (after the CREATE SUBSCRIPTION we are getting replicated odd values
> from the publisher, even though the subscriber side sequence was
> supposed to be even numbers)
>
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
>7
> (1 row)
>
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
>9
> (1 row)
>
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
> 11
> (1 row)
>
> (Looking at the description you would expect odd values for this
> sequence to be impossible)
>
> test_sub=# \dS+ s1
> Sequence "public.s1"
> Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
> +---+-+-+---+-+---
> bigint | 2 | 1 | 9223372036854775807 | 2 | no | 1
Even if we check the sequence definition during the CREATE
SUBSCRIPTION/ALTER SUBSCRIPTION ... REFRESH PUBLICATION or ALTER
SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES commands, there's still
a chance that the sequence definition might change after the command
has been executed. Currently, there's no mechanism to lock a sequence,
and we also permit replication of table data even if the table
structures differ, such as mismatched data types like int and
smallint. I have modified it to log a warning to inform users that the
sequence options on the publisher and subscriber are not the same and
advise them to ensure that the sequence definitions are consistent
between both.
The v20240805 version patch attached at [1] has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CALDaNm1Y_ot-jFRfmtwDuwmFrgSSYHjVuy28RspSopTtwzXy8w%40mail.gmail.com
Regards,
Vignesh
Re: Logical Replication of sequences
On Wed, 31 Jul 2024 at 14:39, shveta malik wrote:
>
> On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote:
> >
> > On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote:
> > >
> > >
> > >
> > > On Sat, Jun 8, 2024 at 6:43 PM vignesh C wrote:
> > >>
> > >> On Wed, 5 Jun 2024 at 14:11, Amit Kapila wrote:
> > >> [...]
> > >> A new catalog table, pg_subscription_seq, has been introduced for
> > >> mapping subscriptions to sequences. Additionally, the sequence LSN
> > >> (Log Sequence Number) is stored, facilitating determination of
> > >> sequence changes occurring before or after the returned sequence
> > >> state.
> > >
> > >
> > > Can't it be done using pg_depend? It seems a bit excessive unless I'm
> > > missing
> > > something.
> >
> > We'll require the lsn because the sequence LSN informs the user that
> > it has been synchronized up to the LSN in pg_subscription_seq. Since
> > we are not supporting incremental sync, the user will be able to
> > identify if he should run refresh sequences or not by checking the lsn
> > of the pg_subscription_seq and the lsn of the sequence(using
> > pg_sequence_state added) in the publisher.
>
> How the user will know from seq's lsn that he needs to run refresh.
> lsn indicates page_lsn and thus the sequence might advance on pub
> without changing lsn and thus lsn may look the same on subscriber even
> though a sequence-refresh is needed. Am I missing something here?
When a sequence is synchronized to the subscriber, the page LSN of the
sequence from the publisher is also retrieved and stored in
pg_subscriber_rel as shown below:
--- Publisher page lsn
publisher=# select pg_sequence_state('seq1');
pg_sequence_state
(0/1510E38,65,1,t)
(1 row)
--- Subscriber stores the publisher's page lsn for the sequence
subscriber=# select * from pg_subscription_rel where srrelid = 16384;
srsubid | srrelid | srsubstate | srsublsn
-+-++---
16389 | 16384 | r | 0/1510E38
(1 row)
If changes are made to the sequence, such as performing many nextvals,
the page LSN will be updated. Currently the sequence values are
prefetched for SEQ_LOG_VALS 32, so the lsn will not get updated for
the prefetched values, once the prefetched values are consumed the lsn
will get updated.
For example:
--- Updated LSN on the publisher (old lsn - 0/1510E38, new lsn - 0/1558CA8)
publisher=# select pg_sequence_state('seq1');
pg_sequence_state
--
(0/1558CA8,143,22,t)
(1 row)
The user can then compare this updated value with the sequence's LSN
in pg_subscription_rel to determine when to re-synchronize the
sequence.
Regards,
Vignesh
Re: Logical Replication of sequences
On Fri, 2 Aug 2024 at 14:24, shveta malik wrote: > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote: > > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote: > > > > > > Thanks for reporting this, these issues are fixed in the attached > > > v20240730_2 version patch. > > > > > I was reviewing the design of patch003, and I have a query. Do we need > to even start an apply worker and create replication slot when > subscription created is for 'sequences only'? IIUC, currently logical > replication apply worker is the one launching sequence-sync worker > whenever needed. I think it should be the launcher doing this job and > thus apply worker may even not be needed for current functionality of > sequence sync? Going forward when we implement incremental sync of > sequences, then we may have apply worker started but now it is not > needed. I believe the current method of having the apply worker initiate the sequence sync worker is advantageous for several reasons: a) Reduces Launcher Load: This approach prevents overloading the launcher, which must handle various other subscription requests. b) Facilitates Incremental Sync: It provides a more straightforward path to extend support for incremental sequence synchronization. c) Reuses Existing Code: It leverages the existing tablesync worker code for starting the tablesync process, avoiding the need to duplicate code in the launcher. d) Simplified Code Maintenance: Centralizing sequence synchronization logic within the apply worker can simplify code maintenance and updates, as changes will only need to be made in one place rather than across multiple components. e) Better Monitoring and Debugging: With sequence synchronization being handled by the apply worker, you can more effectively monitor and debug synchronization processes since all related operations are managed by a single component. Also, I noticed that even when a publication has no tables, we create replication slot and start apply worker. Regards, Vignesh
Re: Logical Replication of sequences
On Fri, 2 Aug 2024 at 14:33, shveta malik wrote: > > On Fri, Aug 2, 2024 at 2:24 PM shveta malik wrote: > > > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote: > > > > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote: > > > > > > > > Thanks for reporting this, these issues are fixed in the attached > > > > v20240730_2 version patch. > > > > > > > > I was reviewing the design of patch003, and I have a query. Do we need > > to even start an apply worker and create replication slot when > > subscription created is for 'sequences only'? IIUC, currently logical > > replication apply worker is the one launching sequence-sync worker > > whenever needed. I think it should be the launcher doing this job and > > thus apply worker may even not be needed for current functionality of > > sequence sync? Going forward when we implement incremental sync of > > sequences, then we may have apply worker started but now it is not > > needed. > > > > Also, can we please mention the state change and 'who does what' atop > sequencesync.c file similar to what we have atop tablesync.c file > otherwise it is difficult to figure out the flow. I have added this in sequencesync.c file, the changes for the same are available at v20240805_2 version patch at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm1kk1MHGk3BU_XTxay%3DdR6sMHnm4TT5cmVz2f_JXkWENQ%40mail.gmail.com Regards, Vignesh
Re: Logical Replication of sequences
On Mon, Aug 5, 2024 at 2:36 PM vignesh C wrote: > > On Fri, 2 Aug 2024 at 14:24, shveta malik wrote: > > > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote: > > > > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote: > > > > > > > > Thanks for reporting this, these issues are fixed in the attached > > > > v20240730_2 version patch. > > > > > > > > I was reviewing the design of patch003, and I have a query. Do we need > > to even start an apply worker and create replication slot when > > subscription created is for 'sequences only'? IIUC, currently logical > > replication apply worker is the one launching sequence-sync worker > > whenever needed. I think it should be the launcher doing this job and > > thus apply worker may even not be needed for current functionality of > > sequence sync? > But that would lead to maintaining all sequence-sync of each subscription by launcher. Say there are 100 sequences per subscription and some of them from each subscription are failing due to some reasons then the launcher will be responsible for ensuring all the sequences are synced. I think it would be better to handle per-subscription work by the apply worker. > > Going forward when we implement incremental sync of > > sequences, then we may have apply worker started but now it is not > > needed. > > I believe the current method of having the apply worker initiate the > sequence sync worker is advantageous for several reasons: > a) Reduces Launcher Load: This approach prevents overloading the > launcher, which must handle various other subscription requests. > b) Facilitates Incremental Sync: It provides a more straightforward > path to extend support for incremental sequence synchronization. > c) Reuses Existing Code: It leverages the existing tablesync worker > code for starting the tablesync process, avoiding the need to > duplicate code in the launcher. > d) Simplified Code Maintenance: Centralizing sequence synchronization > logic within the apply worker can simplify code maintenance and > updates, as changes will only need to be made in one place rather than > across multiple components. > e) Better Monitoring and Debugging: With sequence synchronization > being handled by the apply worker, you can more effectively monitor > and debug synchronization processes since all related operations are > managed by a single component. > > Also, I noticed that even when a publication has no tables, we create > replication slot and start apply worker. > As far as I understand slots and origins are primarily required for incremental sync. Would it be used only for sequence-sync cases? If not then we can avoid creating those. I agree that it would add some complexity to the code with sequence-specific checks, so we can create a top-up patch for this if required and evaluate its complexity versus the benefit it produces. -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Mon, Aug 5, 2024 at 11:04 AM vignesh C wrote:
>
> On Wed, 31 Jul 2024 at 14:39, shveta malik wrote:
> >
> > On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote:
> > >
> > > On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote:
> > > >
> > > >
> > > >
> > > > On Sat, Jun 8, 2024 at 6:43 PM vignesh C wrote:
> > > >>
> > > >> On Wed, 5 Jun 2024 at 14:11, Amit Kapila
> > > >> wrote:
> > > >> [...]
> > > >> A new catalog table, pg_subscription_seq, has been introduced for
> > > >> mapping subscriptions to sequences. Additionally, the sequence LSN
> > > >> (Log Sequence Number) is stored, facilitating determination of
> > > >> sequence changes occurring before or after the returned sequence
> > > >> state.
> > > >
> > > >
> > > > Can't it be done using pg_depend? It seems a bit excessive unless I'm
> > > > missing
> > > > something.
> > >
> > > We'll require the lsn because the sequence LSN informs the user that
> > > it has been synchronized up to the LSN in pg_subscription_seq. Since
> > > we are not supporting incremental sync, the user will be able to
> > > identify if he should run refresh sequences or not by checking the lsn
> > > of the pg_subscription_seq and the lsn of the sequence(using
> > > pg_sequence_state added) in the publisher.
> >
> > How the user will know from seq's lsn that he needs to run refresh.
> > lsn indicates page_lsn and thus the sequence might advance on pub
> > without changing lsn and thus lsn may look the same on subscriber even
> > though a sequence-refresh is needed. Am I missing something here?
>
> When a sequence is synchronized to the subscriber, the page LSN of the
> sequence from the publisher is also retrieved and stored in
> pg_subscriber_rel as shown below:
> --- Publisher page lsn
> publisher=# select pg_sequence_state('seq1');
> pg_sequence_state
>
> (0/1510E38,65,1,t)
> (1 row)
>
> --- Subscriber stores the publisher's page lsn for the sequence
> subscriber=# select * from pg_subscription_rel where srrelid = 16384;
> srsubid | srrelid | srsubstate | srsublsn
> -+-++---
>16389 | 16384 | r | 0/1510E38
> (1 row)
>
> If changes are made to the sequence, such as performing many nextvals,
> the page LSN will be updated. Currently the sequence values are
> prefetched for SEQ_LOG_VALS 32, so the lsn will not get updated for
> the prefetched values, once the prefetched values are consumed the lsn
> will get updated.
> For example:
> --- Updated LSN on the publisher (old lsn - 0/1510E38, new lsn - 0/1558CA8)
> publisher=# select pg_sequence_state('seq1');
> pg_sequence_state
> --
> (0/1558CA8,143,22,t)
> (1 row)
>
> The user can then compare this updated value with the sequence's LSN
> in pg_subscription_rel to determine when to re-synchronize the
> sequence.
Thanks for the details. But I was referring to the case where we are
in between pre-fetched values on publisher (say at 25th value), while
on subscriber we are slightly behind (say at 15th value), but page-lsn
will be the same on both. Since the subscriber is behind, a
sequence-refresh is needed on sub, but by looking at lsn (which is
same), one can not say that for sure. Let me know if I have
misunderstood it.
thanks
Shveta
Re: Logical Replication of sequences
On Mon, Aug 5, 2024 at 5:28 PM Amit Kapila wrote: > > On Mon, Aug 5, 2024 at 2:36 PM vignesh C wrote: > > > > On Fri, 2 Aug 2024 at 14:24, shveta malik wrote: > > > > > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik > > > wrote: > > > > > > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote: > > > > > > > > > > Thanks for reporting this, these issues are fixed in the attached > > > > > v20240730_2 version patch. > > > > > > > > > > > I was reviewing the design of patch003, and I have a query. Do we need > > > to even start an apply worker and create replication slot when > > > subscription created is for 'sequences only'? IIUC, currently logical > > > replication apply worker is the one launching sequence-sync worker > > > whenever needed. I think it should be the launcher doing this job and > > > thus apply worker may even not be needed for current functionality of > > > sequence sync? > > > > But that would lead to maintaining all sequence-sync of each > subscription by launcher. Say there are 100 sequences per subscription > and some of them from each subscription are failing due to some > reasons then the launcher will be responsible for ensuring all the > sequences are synced. I think it would be better to handle > per-subscription work by the apply worker. I thought we can give that task to sequence-sync worker. Once sequence sync worker is started by launcher, it keeps on syncing until all the sequences are synced (even failed ones) and then exits only after all are synced; instead of apply worker starting it multiple times for failed sequences. Launcher to start sequence sync worker when signaled by 'alter-sub refresh seq'. But after going through details given by Vignesh in [1], I also see the benefits of using apply worker for this task. Since apply worker is already looping and doing that for table-sync, we can reuse the same code for sequence sync and maintenance will be easy. So looks okay if we go with existing apply worker design. [1]: https://www.postgresql.org/message-id/CALDaNm1KO8f3Fj%2BRHHXM%3DUSGwOcW242M1jHee%3DX_chn2ToiCpw%40mail.gmail.com > > > > > Going forward when we implement incremental sync of > > > sequences, then we may have apply worker started but now it is not > > > needed. > > > > I believe the current method of having the apply worker initiate the > > sequence sync worker is advantageous for several reasons: > > a) Reduces Launcher Load: This approach prevents overloading the > > launcher, which must handle various other subscription requests. > > b) Facilitates Incremental Sync: It provides a more straightforward > > path to extend support for incremental sequence synchronization. > > c) Reuses Existing Code: It leverages the existing tablesync worker > > code for starting the tablesync process, avoiding the need to > > duplicate code in the launcher. > > d) Simplified Code Maintenance: Centralizing sequence synchronization > > logic within the apply worker can simplify code maintenance and > > updates, as changes will only need to be made in one place rather than > > across multiple components. > > e) Better Monitoring and Debugging: With sequence synchronization > > being handled by the apply worker, you can more effectively monitor > > and debug synchronization processes since all related operations are > > managed by a single component. > > > > Also, I noticed that even when a publication has no tables, we create > > replication slot and start apply worker. > > > > As far as I understand slots and origins are primarily required for > incremental sync. Would it be used only for sequence-sync cases? If > not then we can avoid creating those. I agree that it would add some > complexity to the code with sequence-specific checks, so we can create > a top-up patch for this if required and evaluate its complexity versus > the benefit it produces. > > -- > With Regards, > Amit Kapila.
Re: Logical Replication of sequences
On Tue, Aug 6, 2024 at 8:49 AM shveta malik wrote:
>
Do we need some kind of coordination between table sync and sequence
sync for internally generated sequences? Lets say we have an identity
column with a 'GENERATED ALWAYS' sequence. When the sequence is synced
to subscriber, subscriber can also do an insert to table (extra one)
incrementing the sequence and then when publisher performs an insert,
apply worker will blindly copy that row to sub's table making identity
column's duplicate entries.
CREATE TABLE color ( color_id INT GENERATED ALWAYS AS
IDENTITY,color_name VARCHAR NOT NULL);
Pub: insert into color(color_name) values('red');
Sub: perform sequence refresh and check 'r' state is reached, then do insert:
insert into color(color_name) values('yellow');
Pub: insert into color(color_name) values('blue');
After above, data on Pub: (1, 'red') ;(2, 'blue'),
After above, data on Sub: (1, 'red') ;(2, 'yellow'); (2, 'blue'),
Identity column has duplicate values. Should the apply worker error
out while inserting such a row to the table? Or it is not in the
scope of this project?
thanks
Shveta
Re: Logical Replication of sequences
On Tue, Aug 6, 2024 at 8:49 AM shveta malik wrote: > > On Mon, Aug 5, 2024 at 5:28 PM Amit Kapila wrote: > > > > On Mon, Aug 5, 2024 at 2:36 PM vignesh C wrote: > > > > > > On Fri, 2 Aug 2024 at 14:24, shveta malik wrote: > > > > > > > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik > > > > wrote: > > > > > > > > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote: > > > > > > > > > > > > Thanks for reporting this, these issues are fixed in the attached > > > > > > v20240730_2 version patch. > > > > > > > > > > > > > > I was reviewing the design of patch003, and I have a query. Do we need > > > > to even start an apply worker and create replication slot when > > > > subscription created is for 'sequences only'? IIUC, currently logical > > > > replication apply worker is the one launching sequence-sync worker > > > > whenever needed. I think it should be the launcher doing this job and > > > > thus apply worker may even not be needed for current functionality of > > > > sequence sync? > > > > > > > But that would lead to maintaining all sequence-sync of each > > subscription by launcher. Say there are 100 sequences per subscription > > and some of them from each subscription are failing due to some > > reasons then the launcher will be responsible for ensuring all the > > sequences are synced. I think it would be better to handle > > per-subscription work by the apply worker. > > I thought we can give that task to sequence-sync worker. Once sequence > sync worker is started by launcher, it keeps on syncing until all the > sequences are synced (even failed ones) and then exits only after all > are synced; instead of apply worker starting it multiple times for > failed sequences. Launcher to start sequence sync worker when signaled > by 'alter-sub refresh seq'. > But after going through details given by Vignesh in [1], I also see > the benefits of using apply worker for this task. Since apply worker > is already looping and doing that for table-sync, we can reuse the > same code for sequence sync and maintenance will be easy. So looks > okay if we go with existing apply worker design. > Fair enough. However, I was wondering whether apply_worker should exit after syncing all sequences for a sequence-only subscription or should it be there for future commands that can refresh the subscription and add additional tables or sequences? -- With Regards, Amit Kapila.
Re: Logical Replication of sequences
On Tue, Aug 6, 2024 at 9:54 AM Amit Kapila wrote:
>
> On Tue, Aug 6, 2024 at 8:49 AM shveta malik wrote:
> >
> > > > > I was reviewing the design of patch003, and I have a query. Do we need
> > > > > to even start an apply worker and create replication slot when
> > > > > subscription created is for 'sequences only'? IIUC, currently logical
> > > > > replication apply worker is the one launching sequence-sync worker
> > > > > whenever needed. I think it should be the launcher doing this job and
> > > > > thus apply worker may even not be needed for current functionality of
> > > > > sequence sync?
> > > >
> > >
> > > But that would lead to maintaining all sequence-sync of each
> > > subscription by launcher. Say there are 100 sequences per subscription
> > > and some of them from each subscription are failing due to some
> > > reasons then the launcher will be responsible for ensuring all the
> > > sequences are synced. I think it would be better to handle
> > > per-subscription work by the apply worker.
> >
> > I thought we can give that task to sequence-sync worker. Once sequence
> > sync worker is started by launcher, it keeps on syncing until all the
> > sequences are synced (even failed ones) and then exits only after all
> > are synced; instead of apply worker starting it multiple times for
> > failed sequences. Launcher to start sequence sync worker when signaled
> > by 'alter-sub refresh seq'.
> > But after going through details given by Vignesh in [1], I also see
> > the benefits of using apply worker for this task. Since apply worker
> > is already looping and doing that for table-sync, we can reuse the
> > same code for sequence sync and maintenance will be easy. So looks
> > okay if we go with existing apply worker design.
> >
>
> Fair enough. However, I was wondering whether apply_worker should exit
> after syncing all sequences for a sequence-only subscription
If apply worker exits, then on next sequence-refresh, we need a way to
wake-up launcher to start apply worker which then will start
table-sync worker. Instead, won't it be better if the launcher starts
table-sync worker directly without the need of apply worker being
present (which I stated earlier).
> or should
> it be there for future commands that can refresh the subscription and
> add additional tables or sequences?
If we stick with apply worker starting table sync worker when needed
by continuously checking seq-sync states ('i'/'r'), then IMO, it is
better that apply-worker stays. But if we want apply-worker to exit
and start only when needed, then why not to start sequence-sync worker
directly for seq-only subscriptions?
thanks
Shveta
