Re: clarifying trigger/rule behavior on logical replication subscribers
On Wed, Jun 7, 2023 at 4:09 PM vignesh C wrote: > > On Wed, 7 Jun 2023 at 10:17, Jonathan S. Katz wrote: > > > > Hi, > > > > While answering a question on "do triggers fire on a logical replication > > subscriber by default?" I tried to look up a reference to this behavior > > in the docs. There wasn't a clear reference point, but on the > > architecture page[1], I found this line that was closest to the answer: > > > > "The apply process on the subscriber database always runs with > > session_replication_role set to replica, which produces the usual > > effects on triggers and constraints." > > > > which assumes that the reader knows what the "usual effects" are. > > > > Attached is a patch that disambiguates this. > > I agree that it is a good idea to be explicit about this behavior. As mentioned by you, it would be better to backpatch this. Also, I thought about moving this to some other section like [1] but I think we need to explain a bit about apply worker before providing this specific information. So, the current place seems to be the best bet considering the way current logical replication is documented. [1] - https://www.postgresql.org/docs/devel/logical-replication-subscription.html -- With Regards, Amit Kapila.
Re: Ambiguous description on new columns
On Fri, May 31, 2024 at 10:54 PM Peter Smith wrote: > > On Wed, May 29, 2024 at 8:04 PM vignesh C wrote: > > > > > > > > > > The following documentation comment has been logged on the website: > > > > > > > > Page: > > > > https://www.postgresql.org/docs/16/logical-replication-col-lists.html > > > > Description: > > > > > > > > The documentation on this page mentions: > > > > > > > > "If no column list is specified, any columns added later are > > > > automatically > > > > replicated." > > > > > > > > It feels ambiguous what this could mean. Does it mean: > > > > > > > > 1/ That if you alter the table on the publisher and add a new column, it > > > > will be replicated > > > > > > > > 2/ If you add a column list later and add a column to it, it will be > > > > replicated > > > > > > > > In both cases, does the subscriber automatically create this column if > > > > it > > > > wasn't there before? > > > > > > > > > > > > I think the following small change will remove any ambiguity: > > > > > > BEFORE > > > If no column list is specified, any columns added later are > > > automatically replicated. > > > > > > SUGGESTION > > > If no column list is specified, any columns added to the table later > > > are automatically replicated. > > > > > > ~~ > > > > > > I attached a small patch to make the above change. > > > > A small recommendation: > > It would enhance clarity to include a line break following "If no > > column list is specified, any columns added to the table later are": > > - If no column list is specified, any columns added later are > > automatically > > + If no column list is specified, any columns added to the table > > later are automatically > > replicated. This means that having a column list which names all columns > > Hi Vignesh, > > IIUC you're saying my v1 patch *content* and rendering is OK, but you > only wanted the SGML text to have better wrapping for < 80 chars > lines. So I have attached a patch v2 with improved wrapping. If you > meant something different then please explain. > Your patch is an improvement. Koen, does the proposed change make things clear to you? -- With Regards, Amit Kapila.
Re: Ambiguous description on new columns
On Tue, Jun 4, 2024 at 11:26 AM Amit Kapila wrote: > > > > > IIUC you're saying my v1 patch *content* and rendering is OK, but you > > only wanted the SGML text to have better wrapping for < 80 chars > > lines. So I have attached a patch v2 with improved wrapping. If you > > meant something different then please explain. > > > > Your patch is an improvement. Koen, does the proposed change make > things clear to you? > I am planning to push and backpatch the latest patch by Peter Smith unless there are any further comments or suggestions. -- With Regards, Amit Kapila.
Re: Ambiguous description on new columns
On Fri, Jun 7, 2024 at 3:23 PM Koen De Groote wrote: > > Yes, this change is clear to me that the "columns added" applies to the table > on the publisher. > Thanks for the confirmation. I have pushed and backpatched the fix. -- With Regards, Amit Kapila.
Re: Fix an incorrect statement for failover option in alter_subscription.sgml
On Mon, Jul 22, 2024 at 10:45 AM shveta malik wrote: > > On Fri, Jul 19, 2024 at 10:43 AM Zhijie Hou (Fujitsu) > wrote: > > > > Hi, > > > > The documentation incorrectly stated that users cannot alter subscription's > > failover option when two-phase commit is enabled. > > > > When writing this doc, I only wanted to mention we cannot execute ALTER SUB > > SET > > (failover) in a transaction block, but I missed the following statement for > > two-phase commit which causes the incorrect understanding. Here is a small > > patch to fix it. > > The patch looks good to me except there is repetition of commands in > both the lines now. But I could not think of a better way. So I think > it is okay to have this way. > Thanks for the review. I also couldn't think of a better way to document this information. The patch as proposed looks good to me. -- With Regards, Amit Kapila.
Re: Correction in doc of failover ready steps
On Mon, Jul 22, 2024 at 10:59 AM shveta malik wrote: > > On Mon, Jul 22, 2024 at 10:46 AM shveta malik wrote: > > > > Hi, > > > > We have a query in failover-ready doc referring to > > pg_subscription_rel. Unlike pg_subscription, pg_subscription_rel gives > > results only when connected to the database having the > > subscription(s). If we run the concerned query on any other database, > > it will give incomplete results i.e. it will give info on main slots > > leaving table sync slots (if any). > > Thus the failover-ready steps which queries pg_subscription_rel need > > to mention that the concerned query needs to be run on the database(s) > > that includes the failover enabled subscription(s). Corrected the doc > > for the same. > > On rethinking, since pg_subscription query needs to be run only once > on *any* database to get combined results of all main slots while > pg_subscription_rel query needs to be run on each database having > concerned subscription (and table), does it makes sense to separate > the 2 queries instead of having UNION ? Thoughts? > I think so. Let's see if Hou-San or anyone else has better ideas to fetch this information. -- With Regards, Amit Kapila.
Re: Correction in doc of failover ready steps
On Tue, Jul 23, 2024 at 9:40 AM shveta malik wrote: > > On Tue, Jul 23, 2024 at 8:14 AM Zhijie Hou (Fujitsu) > wrote: > > > > I also agree that separating the 2 queries makes sense. > > Please find the patch with the suggested change. > One minor comment: - if the table copy is finished (See ). + On the subscriber node, use the following SQL to identify which main + slots should be synced to the standby that we plan to promote. This query Shall we refer to these slots as replication slots instead of main slots in the above sentence? We don't have a main slot terminology at other places, so it would be better not to introduce a new one. I know that it was introduced in the original commit but it seems better to change if we agree. -- With Regards, Amit Kapila.
Re: PostgreSQL 15 minor fixes in protocol.sgml
On Tue, Aug 2, 2022 at 4:28 PM Michael Paquier wrote: > > On Mon, Aug 01, 2022 at 11:00:20PM +0300, Ekaterina Kiryanova wrote: > > > Another point worth mentioning is that only this file contains the phrase > > "two-phase transaction". I believe that "two-phase commit transaction" or > > "transaction prepared for two-phase commit" depending on the situation would > > be better wording. > > "Prepare for two-phase commit" may be clearer? > I think we can use just "Prepared transaction" instead. So, the message "The user defined GID of the two-phase transaction." can be changed to "The user defined GID of the prepared transaction.". Similarly, the message "Identifies the message as a two-phase prepared transaction message." could be changed to: "Identifies the message as a prepared transaction message." > > And finally, could you please clarify this part? > > -- The end LSN of the prepare transaction. > > Is it a typo of "prepared transaction"? I think in this case it should be a "prepared transaction". Thanks for the report and Thanks Michael for including me. I am just redirecting it to -hackers so that others involved in this feature also can share their views. -- With Regards, Amit Kapila.
Re: PostgreSQL 15 minor fixes in protocol.sgml
On Wed, Aug 3, 2022 at 10:56 AM Peter Smith wrote: > > PSA a patch to modify the descriptions as suggested by Amit. > * - The end LSN of the commit prepared transaction. + The end LSN of the commit of the prepared transaction. ... ... - Identifies the message as the commit of a two-phase transaction message. + Identifies the message as the commit of a prepared transaction message. In the above messages, we can even directly say "commit prepared transaction" but as you have written appears clear to me. * For timestamp, related messages, we have three different messages: Commit timestamp of the transaction. The value is in number of microseconds since PostgreSQL epoch (2000-01-01). Prepare timestamp of the transaction. The value is in number of microseconds since PostgreSQL epoch (2000-01-01). Rollback timestamp of the transaction. The value is in number of microseconds since PostgreSQL epoch (2000-01-01). We can improve by saying "Timestamp of prepared transaction" for the second one but it will make it bit inconsistent with others, so not sure if changing it makes sense or if there is a better way to change all the three messages. Thoughts? -- With Regards, Amit Kapila.
Re: pg_copy_logical_replication_slot doesn't copy the failover property
On Sat, Feb 22, 2025 at 11:26 AM Shlok Kyal wrote: > > Adding Amit to the thread. > Thanks. Looking at the original complaint: > It says > * To avoid potential issues with the slot synchronization > where the > * restart_lsn of a replication slot can go backward, we set > the > * failover option to false here. This situation occurs when > a slot > * on the primary server is dropped and immediately replaced > with a > * new slot of the same name, created by copying from another > existing > * slot. However, the slot synchronization will only observe > the > * restart_lsn of the same slot going backward. It would be better to update the comments as well to make the potential issues with slot synchronization clear or mention the reference of other place where we have comments related to this race condition. Also, I think it is better to write about two_phase in the comments as well as in docs unless already mentioned. If you agree with updating the comments as well, shall we redirect this to hackers? > > > > IIUC the two_phase field is also not copied from the source slot. I > > think we can clarify in the doc that these two fields are not copied. > > +1. -- With Regards, Amit Kapila.
