On Tuesday, December 14, 2021 12:45 AM vignesh C <vignes...@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
