On Mon, Mar 21, 2022 at 7:09 AM Euler Taveira <eu...@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.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.
>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.
v17-0001-Add-ALTER-SUBSCRIPTION-.-SKIP.patch
Description: Binary data
