Re: Skipping logical replication transactions on subscriber side

On Thu, Mar 17, 2022, at 3:03 AM, Amit Kapila wrote:
> On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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/