Re: Skipping logical replication transactions on subscriber side

On Fri, Sep 10, 2021 at 12:33 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Sorry for the late response. I've attached the updated patches that
> incorporate all comments unless I missed something. Please review
> them.
>

Here's some review comments for the v13-0001 patch:

doc/src/sgml/monitoring.sgml

(1)
There's an extra space in the following line, before "processing":

+       OID of the relation that the worker was  processing when the

(2) Suggested wording update:
BEFORE:
+        field is always NULL if the error is reported by
AFTER:
+        field is always NULL if the error is reported by the

(3) Suggested wording update:
BEFORE:
+        by <literal>tablesync</literal> worker.
AFTER:
+        by the <literal>tablesync</literal> worker.

(4)
Missing "." at end of following description (inconsistent with other doc):

+       Time at which these statistics were last reset

(5) Suggested wording update:
BEFORE:
+         can be granted EXECUTE to run the function.
AFTER:
+         can be granted EXECUTE privilege to run the function.


src/backend/postmaster/pgstat.c

(6) Suggested wording update:
BEFORE:
+ * for this relation already completes or the table is no
AFTER:
+ * for this relation already completed or the table is no


(7)
In the code below, since errmsg.m_nentries only ever gets incremented
by the 1st IF condition, it's probably best to include the 2nd IF
block within the 1st IF condition. Then can avoid checking
"errmsg.m_nentries" each loop iteration.

+ if (hash_search(not_ready_rels_htab, (void *) &(errent->relid),
+ HASH_FIND, NULL) == NULL)
+ errmsg.m_relids[errmsg.m_nentries++] = errent->relid;
+
+ /*
+ * If the message is full, send it out and reinitialize to
+ * empty
+ */
+ if (errmsg.m_nentries >= PGSTAT_NUM_SUBSCRIPTIONERRPURGE)
+ {
+ len = offsetof(PgStat_MsgSubscriptionErrPurge, m_relids[0])
+ + errmsg.m_nentries * sizeof(Oid);
+
+ pgstat_setheader(&errmsg.m_hdr, PGSTAT_MTYPE_SUBSCRIPTIONERRPURGE);
+ pgstat_send(&errmsg, len);
+ errmsg.m_nentries = 0;
+ }


(8)
+ * Tell the collector about reset the subscription error.

Is this meant to say "Tell the collector to reset the subscription error." ?


(9)
I think the following:

+ len = offsetof(PgStat_MsgSubscriptionErr, m_errmsg[0]) + strlen(errmsg);

should be:

+ len = offsetof(PgStat_MsgSubscriptionErr, m_errmsg[0]) + strlen(errmsg) + 1;

to account for the \0 terminator.

(10)
I don't think that using the following Assert is really correct here,
because PgStat_MsgSubscriptionErr is not setup to have the maximum
number of m_errmsg[] entries to fill up to PGSTAT_MAX_MSG_SIZE (as are
some of the other pgstat structs):

+ Assert(len < PGSTAT_MAX_MSG_SIZE);

(the max size of all of the pgstat structs is statically asserted anyway)

It would be correct to do the following instead:

+ Assert(strlen(errmsg) < PGSTAT_SUBSCRIPTIONERR_MSGLEN);

The overflow is guarded by the strlcpy() in any case.

(11)
Would be better to write:

+ rc = fwrite(&nerrors, sizeof(nerrors), 1, fpout);

instead of:

+ rc = fwrite(&nerrors, sizeof(int32), 1, fpout);


(12)
Would be better to write:

+ if (fread(&nerrors, 1, sizeof(nerrors), fpin) != sizeof(nerrors))

instead of:

+ if (fread(&nerrors, 1, sizeof(int32), fpin) != sizeof(int32))


src/include/pgstat.h

(13)
BEFORE:
+ * update/reset the error happening during logical
AFTER:
+ * update/reset the error occurring during logical

(14)
Typo:  replicatoin -> replication

+ * an error that occurred during application of logical replicatoin or


(15) Suggested wording update:
BEFORE:
+ * there is no table sync error, where is the common case in practice.
AFTER:
+ * there is no table sync error, which is the common case in practice.


Regards,
Greg Nancarrow
Fujitsu Australia