On Thu, Mar 17, 2022, at 3:03 AM, Amit Kapila 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.
