On Fri, Jan 21, 2022 at 10:00 PM David G. Johnston <david.g.johns...@gmail.com> wrote: > > On Fri, Jan 21, 2022 at 4:55 AM Amit Kapila <amit.kapil...@gmail.com> wrote: >> >> Apart from this, I have changed a few comments and ran pgindent. Do >> let me know what you think of the changes? > > > The paragraph describing ALTER SUBSCRIPTION SKIP seems unnecessarily > repetitive. Consider: > """ > Skips applying all changes of the specified remote transaction, whose value > should be obtained from pg_stat_subscription_workers.last_error_xid. >
Here, you can also say that the value can be found from server logs as well. > While this will result in avoiding the last error on the subscription, thus allowing it to resume working. See "link to a more holistic description in the Logical Replication chapter" for alternative means of resolving subscription errors. Removing an entire transaction from the history of a table should be considered a last resort as it can leave the system in a very inconsistent state. > > Note, this feature will not accept transactions prepared under two-phase > commit. > > This command sets pg_subscription.subskipxid field upon issuance and the > system clears the same field upon seeing and successfully skipped the > identified transaction. Issuing this command again while a skipped > transaction is pending replaces the existing transaction with the new one. > """ > The proposed text sounds better to me except for a minor change as suggested above. > Then change the subskipxid column description to be: > """ > ID of the transaction whose changes are to be skipped. It is 0 when there > are no pending skips. This is set by issuing ALTER SUBSCRIPTION SKIP and > resets back to 0 when the identified transactions passes through the > subscription stream and is successfully ignored. > """ > Users can manually reset it by specifying NONE, so that should be covered in the above text, otherwise, looks good. > I don't understand why/how ", if a valid transaction ID;" comes into play > (how would we know whether it is valid, or if we do ALTER SUBSCRIPTION SKIP > should prohibit the invalid value from being chosen). > What do you mean by invalid value here? Is it the value lesser than FirstNormalTransactionId or a value that is of the non-error transaction? For the former, we already have a check in the patch and for later we can't identify it with any certainty because the error stats are collected by the stats collector. > I'm against mentioning subtransactions in the skip_option description. > We have mentioned that because currently, we don't support it but in the future one can come up with an idea to support it. What problem do you see with it? > The Logical Replication page changes provide good content overall but I > dislike going into detail about how to perform conflict resolution in the > third paragraph and then summarize the various forms of conflict resolution > in the newly added forth. Maybe re-work things like: > > 1. Logical replication behaves... > 2. A conflict will produce...details can be found in places... > 3. Resolving conflicts can be done by... > 4. (split and reworded) If choosing to simply skip the offending transaction > you take the pg_stat_subscription_worker.last_error_xid value (716 in the > example above) and provide it while executing ALTER SUBSCRIPTION SKIP... > 5. (split and reworded) Prior to v15 ALTER SUBSCRIPTION SKIP was not > available and instead you had to use the pg_replication_origin_advance() > function... > > Don't just list out two options for the user to perform the same action. > Tell a story about why we felt compelled to add ALTER SYSTEM SKIP and why > either the function is now deprecated or is useful given different > circumstances (the former seems likely). > Personally, I don't see much value in the split (especially giving context like "Prior to v15 ..) but specifying the circumstances where each of the options could be useful. -- With Regards, Amit Kapila.
