MIME-Version: 1.0
References: 
 <OSBPR01MB48887CA8F40C8D984A6DC00CED199@OSBPR01MB4888.jpnprd01.prod.outlook.com>
 <CAD21AoAixHnpjG-TtnSejJ2Dv1VsrzGr3oVPSRFhYjz3Z8_XZA@mail.gmail.com>
 <OSBPR01MB4888D9048D3D7E7C3D035D54EDEF9@OSBPR01MB4888.jpnprd01.prod.outlook.com>
 <CAD21AoCbj6YBNmdG9gAXf=dPE+bdY95ENwkh4q5wPL0q=qMzVw@mail.gmail.com>
 <CAA4eK1+tOV-+ssGjj1zq+nAL8a9LfPsxbtyupZGvZ0U7nV0A7g@mail.gmail.com>
 <CAD21AoAO0004L1kv9+uArrhT7d=vFj=2ZUKFbUfpZnHJku-37g@mail.gmail.com>
 <CAA4eK1+CfMrJeA8TR__uFMQVZk50L+OGUEW-8h5G0hD_erfaSw@mail.gmail.com>
 <CAD21AoDtn1bY4CYkBDXMgxpyBQZTAh45HMTXfAjQ3nY_G_=7Mg@mail.gmail.com>
 <CAA4eK1KNqNnWq=wep5OALjqRRKbqp3pRjnqBJi2CPfMYJMUjrQ@mail.gmail.com>
 <CAD21AoA6X7UtbewgRVetcwh2tuAp=umR5n_bE69ZBj3mN6R31Q@mail.gmail.com>
 <CAA4eK1KMT8biciVqTBoZ9gYV-Gf297JFeNhJaxZNmFrZL8m2jA@mail.gmail.com>
 <TY2PR01MB48905729E8CB81805CCF64C0EDCE9@TY2PR01MB4890.jpnprd01.prod.outlook.com>
 <TY2PR01MB489065DB7B8066936087C5E4EDD59@TY2PR01MB4890.jpnprd01.prod.outlook.com>
 <OSBPR01MB4888507DD52192F0CDF9A49DEDDA9@OSBPR01MB4888.jpnprd01.prod.outlook.com>
 <TY2PR01MB489042618A073A2FC7BF62C0EDA29@TY2PR01MB4890.jpnprd01.prod.outlook.com>
 <CAA4eK1JEVoTJNMu94vVdUoak-uefo9V=XJ5bQQ+o47uawDFftg@mail.gmail.com>
 <TY2PR01MB48902C1278A49542EDBB377FEDA89@TY2PR01MB4890.jpnprd01.prod.outlook.com>
 <CAA4eK1+NT3u-hcYzgrOn2JaEQ=V3EvRxOwvY4Vs5GxR6TgQpcA@mail.gmail.com>
 <CAD21AoCnuZu1NA+xF3ZEHikuEKKzVm-NdPM=MZFJ3KZ3jvN+uw@mail.gmail.com>
 <CAA4eK1L3dPeUNu740mu-5znsvXpx2PkRRZ=w1Msf58o=EWSnnQ@mail.gmail.com>
 <OS0PR01MB5716E503D54B23F21A9733B194AA9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1JqwpsvjhLxV8CMYQ3NrhimZ8AFhWHh0Qn1FrL=LXfY6Q@mail.gmail.com>
 <OS0PR01MB5716D863C8800028DA65612494B89@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1+1n3upCMB-Y_k9b1wPNCtNE7MEHan9kA1s6GNsZGB0Og@mail.gmail.com>
 <CAD21AoDF7LmSALzMfmPshRw_xFcRz3WvB-me8T2gO6Ht=3zL2w@mail.gmail.com>
 <OSBPR01MB4888125C82102E3343D85BC7ED869@OSBPR01MB4888.jpnprd01.prod.outlook.com>
 <OSBPR01MB48880D8D2966FE8711F5BBB0ED8A9@OSBPR01MB4888.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OSBPR01MB48880D8D2966FE8711F5BBB0ED8A9@OSBPR01MB4888.jpnprd01.prod.outlook.com>
From: Greg Nancarrow <gregn4422@gmail.com>
Date: Thu, 4 Nov 2021 11:53:47 +1100
Message-ID: 
 <CAJcOf-dqYomG3h0HP0zkecjNCMFS85eYaubLiPPScibyrP_7Qg@mail.gmail.com>
Subject: Re: Failed transaction statistics to measure the logical replication
 progress
To: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	"houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAJcOf-dqYomG3h0HP0zkecjNCMFS85eYaubLiPPScibyrP_7Qg%40mail.gmail.com>
Precedence: bulk

On Tue, Nov 2, 2021 at 12:18 AM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Thursday, October 28, 2021 11:19 PM I wrote:
> > I've created a new patch that extends pg_stat_subscription_workers to include
> > other transaction statistics.
> >
> > Note that this patch depends on v18 patch-set in [1]...
> Rebased based on the v19 in [1].
> Also, fixed documentation a little and made tests tidy.
> FYI, the newly included TAP test(027_worker_xact_stats.pl) is stable
> because I checked that 100 times of its execution in a tight loop all passed.
>

I have done some basic testing of the patch and have some initial
review comments:

(1) I think this patch needs to be in "git format-patch" format, with
a proper commit message that describes the purpose of the patch and
the functionality it adds, and any high-level design points (something
like the overview given in the initial post, and accounting for the
subsequent discussion points and updated functionality).

(2) doc/src/sgml/monitoring.sgml
There are some grammatical issues in the current description. I
suggest changing it to something like:

BEFORE:
+      <entry>At least one row per subscription, showing about
transaction statistics and error summary that
AFTER:
+      <entry>At least one row per subscription, showing transaction
statistics and information about errors that

(2) doc/src/sgml/monitoring.sgml
The current description seems a little confusing.
Per subscription, it shows the transaction statistics and any last
error info from tablesync/apply workers? If this is the case, I'd
suggest the following change:

BEFORE:
+   one row per subscription for transaction statistics and summary of the last
+   error reported by workers applying logical replication changes and workers
+   handling the initial data copy of the subscribed tables.
AFTER:
+   one row per subscription, showing corresponding transaction statistics and
+   information about the last error reported by workers applying
logical replication
+   changes or by workers handling the initial data copy of the
subscribed tables.

(3) xact_commit
I think that the "xact_commit" column should be named
"xact_commit_count" or "xact_commits".
Similarly, I think "xact_error" should be named "xact_error_count" or
"xact_errors", and "xact_aborts" should be named "xact_abort_count" or
"xact_aborts".

(4) xact_commit_bytes

+       Amount of transactions data successfully applied in this subscription.
+       Consumed memory for xact_commit is displayed.

I find this description a bit confusing. "Consumed memory for
xact_commit" seems different to "transactions data".
Could the description be something like: Amount of data (in bytes)
successfully applied in this subscription, across "xact_commit_count"
transactions.

(5)
I'd suggest some minor rewording for the following:

BEFORE:
+       Number of transactions failed to be applied and caught by table
+       sync worker or main apply worker in this subscription.
AFTER:
+       Number of transactions that failed to be applied by the table
+       sync worker or main apply worker in this subscription.

(6) xact_error_bytes
Again, it's a little confusing referring to "consumed memory" here.
How about rewording this, something like:

BEFORE:
+       Amount of transactions data unsuccessfully applied in this subscription.
+       Consumed memory that past failed transaction used is displayed.
AFTER:
+       Amount of data (in bytes) unsuccessfully applied in this
subscription by the last failed transaction.

(7)
The additional information provided for "xact_abort_bytes" needs some
rewording, something like:

BEFORE:
+       Increase <literal>logical_decoding_work_mem</literal> on the publisher
+       so that it exceeds the size of whole streamed transaction
+       to suppress unnecessary consumed network bandwidth in addition to change
+       in memory of the subscriber, if unexpected amount of streamed
transactions
+       are aborted.
AFTER:
+       In order to suppress unnecessary consumed network bandwidth, increase
+       <literal>logical_decoding_work_mem</literal> on the publisher so that it
+       exceeds the size of the whole streamed transaction, and
additionally increase
+       the available subscriber memory, if an unexpected amount of
streamed transactions
+       are aborted.

(8)
Suggested update:

BEFORE:
+ *  Tell the collector that worker transaction has finished without problem.
AFTER:
+ *  Tell the collector that the worker transaction has successfully completed.

(9) src/backend/postmaster/pgstat.c
I think that the GID copying is unnecessarily copying the whole GID
buffer or using an additional strlen().
It should be changed to use strlcpy() to match other code:

BEFORE:
+ /* get the gid for this two phase operation */
+ if (command == LOGICAL_REP_MSG_PREPARE ||
+   command == LOGICAL_REP_MSG_STREAM_PREPARE)
+   memcpy(msg.m_gid, prepare_data->gid, GIDSIZE);
+ else if (command == LOGICAL_REP_MSG_COMMIT_PREPARED)
+   memcpy(msg.m_gid, commit_data->gid, GIDSIZE);
+ else /* rollback prepared */
+   memcpy(msg.m_gid, rollback_data->gid, GIDSIZE);
AFTER:
+ /* get the gid for this two phase operation */
+ if (command == LOGICAL_REP_MSG_PREPARE ||
+   command == LOGICAL_REP_MSG_STREAM_PREPARE)
+   strlcpy(msg.m_gid, prepare_data->gid, sizeof(msg.m_gid));
+ else if (command == LOGICAL_REP_MSG_COMMIT_PREPARED)
+   strlcpy(msg.m_gid, commit_data->gid, sizeof(msg.m_gid));
+ else /* rollback prepared */
+   strlcpy(msg.m_gid, rollback_data->gid, sizeof(msg.m_gid));


BEFORE:
+ strlcpy(prepared_txn->gid, msg->m_gid, strlen(msg->m_gid) + 1);
AFTER:
+ strlcpy(prepared_txn->gid, msg->m_gid, sizeof(prepared_txn->gid));

BEFORE:
+ memcpy(key.gid, msg->m_gid, strlen(msg->m_gid));
AFTER:
+ strlcpy(key.gid, msg->m_gid, sizeof(key.gid));

BEFORE:
+ memcpy(key.gid, gid, strlen(gid));
AFTER:
+ strlcpy(key.gid, gid, sizeof(key.gid));

(10) src/backend/replication/logical/worker.c
Some suggested rewording:

BEFORE:
+ * size of streaming transaction resources because it have used the
AFTER:
+ * size of streaming transaction resources because it has used the


BEFORE:
+ * tradeoff should not be good. Also, add multiple values
+ * at once in order to reduce the number of this function call.
AFTER:
+ * tradeoff would not be good. Also, add multiple values
+ * at once in order to reduce the number of calls to this function.

(11) update_apply_change_size()
Shouldn't this function be declared static?

(12) stream_write_change()

+ streamed_entry->xact_size = streamed_entry->xact_size + total_len;
/* update */

could be simply written as:

+ streamed_entry->xact_size += total_len; /* update */


Regards,
Greg Nancarrow
Fujitsu Australia