On Mon, Mar 21, 2022 at 7:09 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Mar 17, 2022, at 3:03 AM, Amit Kapila wrote:
>
> On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada <sawada.mshk@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.
>
I have fixed all the above comments as per your suggestion in the
attached. Do let me know if something is missed?
> 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.
>
As explained in my previous email, I don't think any change is
required for this comment but do let me know if you still think so?
--
With Regards,
Amit Kapila.
