public inbox for [email protected]
help / color / mirror / Atom feedFrom: [email protected] <[email protected]>
To: 'vignesh C' <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Greg Nancarrow <[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, 16 Dec 2021 11:41:27 +0000
Message-ID: <TYCPR01MB8373985EF7977DDEB5BB385BED779@TYCPR01MB8373.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CALDaNm0=J+xhZ=qJ+Otc36O1E-EfO_9_--1Ed__zYCv+a58Uig@mail.gmail.com>
References: <TYCPR01MB8373DC922E549FF3F031E9DAED8E9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAJcOf-cMU_1VJ1c+9fwEBgimkj+=hHcCjj_SUfbHvKTuN=1tuw@mail.gmail.com>
<TYCPR01MB8373B1614522B90BA96883E0ED929@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm3-QTZnd4rxbrUfKMM4u_vgb23LO+KW3O9LX7X6b8mr8A@mail.gmail.com>
<CALDaNm130T4jr2RvpgY19SCY87UQ75wVbjrhjGj43bHV=9-LpQ@mail.gmail.com>
<TYCPR01MB8373FEB287F733C81C1E4D42ED989@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<TYCPR01MB8373D7AE0B6C62E5CDB03C6CED989@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<TYCPR01MB8373CFDCD60CCDB1C35CFA45ED999@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAD21AoBAREVDtAM+JGR3DcnwnOi1Nw7E_5m68ZwFUGkOkfDn1Q@mail.gmail.com>
<TYCPR01MB8373271BE5D4619302C05B00ED9A9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAA4eK1JCbhxuO8fEaAaw8e30=5kEDaFRXpV2+opAdMoEOH5LEg@mail.gmail.com>
<TYWPR01MB8362FCFAAD8D90424BEA2300ED9A9@TYWPR01MB8362.jpnprd01.prod.outlook.com>
<CAA4eK1KR41bRUuPeNBSGv2+q7ROKukS3myeAUqrZMD8MEwR0DQ@mail.gmail.com>
<CAD21AoC_p=gQKhhGwhDLzEU9rGwCWSzTk+a89XGG1JQKn-PiKw@mail.gmail.com>
<TYCPR01MB8373380ED159E8BD52F2D9A5ED689@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm32HHjdwmuoF+Nw5CU70r819Kq+Tmat3bzkkvSv_2u=gA@mail.gmail.com>
<TYWPR01MB8362480F9148248F3D1B1213ED6B9@TYWPR01MB8362.jpnprd01.prod.outlook.com>
<CALDaNm1T3qnUnCiTytACUFiw=vktBPi9dvy8iNXCNAyPALOP_g@mail.gmail.com>
<TYCPR01MB8373C2D4D105B83C378511CAED6E9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm0=J+xhZ=qJ+Otc36O1E-EfO_9_--1Ed__zYCv+a58Uig@mail.gmail.com>
On Tuesday, December 14, 2021 12:45 AM vignesh C <[email protected]> wrote:
> Thanks for the updated patch, few comments:
> 1) Can we change this:
> /*
> + * Report the success of table sync as one commit to consolidate all
> + * transaction stats into one record.
> + */
> + pgstat_report_subworker_xact_end(MyLogicalRepWorker->subid,
> +
> LOGICAL_REP_MSG_COMMIT);
> +
> To:
> /* Report the success of table sync */
> + pgstat_report_subworker_xact_end(MyLogicalRepWorker->subid,
> +
> LOGICAL_REP_MSG_COMMIT);
> +
This function call that the table sync worker reports
an update of stats has been removed according to the recent discussion.
> 2) Typo: ealier should be earlier
> + /*
> + * Report ealier than the call of process_syncing_tables() not
> to miss an
> + * increment of commit_count in case it leads to the process exit. See
> + * process_syncing_tables_for_apply().
> + */
> + pgstat_report_subworker_xact_end(MyLogicalRepWorker->subid,
> +
> LOGICAL_REP_MSG_COMMIT);
> +
Thanks ! Fixed.
> 3) Should we add an Assert for subwentry:
> + /*
> + * If this is a new error reported by table sync worker,
> consolidate this
> + * error count into the entry of apply worker, by swapping the stats
> + * entries.
> + */
> + if (OidIsValid(msg->m_subrelid))
> + subwentry = pgstat_get_subworker_entry(dbentry,
> + msg->m_subid,
> +
> InvalidOid, true);
> + subwentry->error_count++;
The latest implementation doesn't require
the call of pgstat_get_subworker_entry().
So, I skipped.
> 4) Can we slightly change it to :We can change it:
> +# Check the update of stats counters.
> +confirm_transaction_stats_update(
> + $node_subscriber,
> + 'commit_count = 1',
> + 'the commit_count increment by table sync');
> +
> +confirm_transaction_stats_update(
> + $node_subscriber,
> + 'error_count = 1',
> + 'the error_count increment by table sync');
> to:
> +# Check updation of subscription worker transaction count statistics.
> +confirm_transaction_stats_update(
> + $node_subscriber,
> + 'commit_count = 1',
> + 'check table sync worker commit count is updated');
> +
> +confirm_transaction_stats_update(
> + $node_subscriber,
> + 'error_count = 1',
> + 'check table sync worker error count is updated');
I've removed the corresponding tests for table sync workers in the patch.
But, I adopted the comment suggestion partly for the tests of the apply worker.
On the other hand, I didn't fix the 3rd arguments of confirm_transaction_stats_update().
It needs to be a noun, because it's connected to another string
"Timed out while waiting for ". in the function.
See the definition of the function.
The new patch v17 is shared in [1].
[1] - https://www.postgresql.org/message-id/TYCPR01MB83734A7A0596AC7ADB0DCB51ED779%40TYCPR01MB8373.jpnprd0...
Best Regards,
Takamichi Osumi
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], [email protected]
Subject: RE: Failed transaction statistics to measure the logical replication progress
In-Reply-To: <TYCPR01MB8373985EF7977DDEB5BB385BED779@TYCPR01MB8373.jpnprd01.prod.outlook.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