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