public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Nancarrow <[email protected]>
To: [email protected] <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Failed transaction statistics to measure the logical replication progress
Date: Thu, 4 Nov 2021 11:53:47 +1100
Message-ID: <CAJcOf-dqYomG3h0HP0zkecjNCMFS85eYaubLiPPScibyrP_7Qg@mail.gmail.com> (raw)
In-Reply-To: <OSBPR01MB48880D8D2966FE8711F5BBB0ED8A9@OSBPR01MB4888.jpnprd01.prod.outlook.com>
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>
On Tue, Nov 2, 2021 at 12:18 AM [email protected]
<[email protected]> 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
view thread (113+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Failed transaction statistics to measure the logical replication progress
In-Reply-To: <CAJcOf-dqYomG3h0HP0zkecjNCMFS85eYaubLiPPScibyrP_7Qg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox