On Thu, Mar 17, 2022, at 3:03 AM, Amit Kapila wrote: > On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > I've attached an updated version patch. > > > > The patch LGTM. I have made minor changes in comments and docs in the > attached patch. Kindly let me know what you think of the attached? > > I am planning to commit this early next week (on Monday) unless there > are more comments/suggestions. I reviewed this last version and I have a few comments.
+ * If the user set subskiplsn, we do a sanity check to make + * sure that the specified LSN is a probable value. ... user *sets*... + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("skip WAL location (LSN) must be greater than origin LSN %X/%X", + LSN_FORMAT_ARGS(remote_lsn)))); Shouldn't we add the LSN to be skipped in the "(LSN)"? + * Start a new transaction to clear the subskipxid, if not started + * yet. It seems it means subskiplsn. + * subskipxid in order to inform users for cases e.g., where the user mistakenly + * specified the wrong subskiplsn. It seems it means subskiplsn. +sub test_skip_xact +{ It seems this function should be named test_skip_lsn. Unless the intention is to cover other skip options in the future. src/test/subscription/t/029_disable_on_error.pl | 94 ---------- src/test/subscription/t/029_on_error.pl | 183 +++++++++++++++++++ It seems you are removing a test for 705e20f8550c0e8e47c0b6b20b5f5ffd6ffd9e33. I should also name 029_on_error.pl to something else such as 030_skip_lsn.pl or a generic name 030_skip_option.pl. -- Euler Taveira EDB https://www.enterprisedb.com/