RE: Failed transaction statistics to measure the logical replication progress - Mailing list pgsql-hackers

From osumi.takamichi@fujitsu.com
Subject RE: Failed transaction statistics to measure the logical replication progress
Date
Msg-id TYCPR01MB8373985EF7977DDEB5BB385BED779@TYCPR01MB8373.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Re: Failed transaction statistics to measure the logical replication progress  (vignesh C <vignesh21@gmail.com>)
List pgsql-hackers
On Tuesday, December 14, 2021 12:45 AM vignesh C <vignesh21@gmail.com> 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.jpnprd01.prod.outlook.com


Best Regards,
    Takamichi Osumi


pgsql-hackers by date:

Previous
From: "osumi.takamichi@fujitsu.com"
Date:
Subject: RE: Failed transaction statistics to measure the logical replication progress
Next
From: Amit Kapila
Date:
Subject: Re: logical decoding and replication of sequences