On Fri, Sep 10, 2021 at 12:33 AM Masahiko Sawada <sawada.m...@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