Re: Skipping logical replication transactions on subscriber side - Mailing list pgsql-hackers

On Tue, Sep 21, 2021 at 2:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached the updated version patches. Please review them.
>

Some comments on the v14-0001 patch:

(1)
Patch comment

The existing patch comment doesn't read well. I suggest the following updates:

BEFORE:
Add pg_stat_subscription_errors statistics view.

This commits adds new system view pg_stat_logical_replication_error,
showing errors happening during applying logical replication changes
as well as during performing initial table synchronization.

The subscription error entries are removed by autovacuum workers when
the table synchronization competed in table sync worker cases and when
dropping the subscription in apply worker cases.

It also adds SQL function pg_stat_reset_subscription_error() to
reset the single subscription error.

AFTER:
Add a subscription errors statistics view "pg_stat_subscription_errors".

This commit adds a new system view pg_stat_logical_replication_errors,
that shows information about any errors which occur during application
of logical replication changes as well as during performing initial table
synchronization.

The subscription error entries are removed by autovacuum workers after
table synchronization completes in table sync worker cases and after
dropping the subscription in apply worker cases.

It also adds an SQL function pg_stat_reset_subscription_error() to
reset a single subscription error.


src/backend/postmaster/pgstat.c
(2)
In pgstat_read_db_statsfile_timestamp(), you've added the following
code for case 'S':

+ case 'S':
+ {
+ PgStat_StatSubEntry subbuf;
+ PgStat_StatSubErrEntry errbuf;
+ int32 nerrors;
+
+ if (fread(&subbuf, 1, sizeof(PgStat_StatSubEntry), fpin)
+ != sizeof(PgStat_StatSubEntry))
+ {
+ ereport(pgStatRunningInCollector ? LOG : WARNING,
+ (errmsg("corrupted statistics file \"%s\"",
+ statfile)));
+ FreeFile(fpin);
+ return false;
+ }
+
+ if (fread(&nerrors, 1, sizeof(nerrors), fpin) != sizeof(nerrors))
+ {
+ ereport(pgStatRunningInCollector ? LOG : WARNING,
+ (errmsg("corrupted statistics file \"%s\"",
+ statfile)));
+ goto done;
+ }
+
+ for (int i = 0; i < nerrors; i++)
+ {
+ if (fread(&errbuf, 1, sizeof(PgStat_StatSubErrEntry), fpin) !=
+ sizeof(PgStat_StatSubErrEntry))
+ {
+ ereport(pgStatRunningInCollector ? LOG : WARNING,
+ (errmsg("corrupted statistics file \"%s\"",
+ statfile)));
+ goto done;
+ }
+ }
+ }
+
+ break;
+

Why in the 2nd and 3rd instances of calling fread() and detecting a
corrupted statistics file, does it:
    goto done;
instead of:
    FreeFile(fpin);
    return false;

?
(so ends up returning true for these instances)

It looks like a mistake, but if it's intentional then comments need to
be added to explain it.

(3)
In pgstat_get_subscription_error_entry(), there seems to be a bad comment.

Shouldn't:

+ /* Return the apply error worker */
+ return &(subent->apply_error);

be:

+ /* Return the apply worker error */
+ return &(subent->apply_error);


src/tools/pgindent/typedefs.list
(4)

"PgStat_MsgSubscriptionErrReset" is missing from the list.


Regards,
Greg Nancarrow
Fujitsu Australia