Re: Failed transaction statistics to measure the logical replication progress

On Wed, Nov 10, 2021 at 3:43 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, Nov 9, 2021 at 5:05 PM osumi(dot)takamichi(at)fujitsu(dot)com
> <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> > Yes. I've rebased and updated the patch, paying attention to this point.
> > Attached the updated version.
>
> Thanks for the updated patch, few comments:
> 6) Few places you have used strlcpy and few places you have used
> memcpy, you can keep it consistent:
> + msg.m_command = command;
> + strlcpy(msg.m_gid, gid, sizeof(msg.m_gid));
> + msg.m_xact_bytes = xact_size;
>
> + key.subid = subid;
> + memcpy(key.gid, gid, sizeof(key.gid));
> + action = (create ? HASH_ENTER : HASH_FIND);

Few more comments:
1) Here the tuple length is not considered in the calculation, else it
will always show the fixed size for any size tuple. Ex varchar insert
with 1 byte or varchar insert with 100's of bytes. So I feel we should
include the tuple length in the calculation.
+ case LOGICAL_REP_MSG_INSERT:
+ case LOGICAL_REP_MSG_UPDATE:
+ case LOGICAL_REP_MSG_DELETE:
+ Assert(extra_data != NULL);
+
+ /*
+ * Compute size based on ApplyExecutionData.
+ * The size of LogicalRepRelMapEntry can be
skipped because
+ * it is obtained from hash_search in
logicalrep_rel_open.
+ */
+ size += sizeof(ApplyExecutionData) + sizeof(EState) +
+ sizeof(ResultRelInfo) + sizeof(ResultRelInfo);
+
+ /*
+ * Add some extra size if the target relation
is partitioned.
+ * PartitionTupleRouting isn't exported.
Therefore, call the
+ * function that returns its size instead.
+ */
+ if
(extra_data->relmapentry->localrel->rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE)
+ size += sizeof(ModifyTableState) +
PartitionTupleRoutingSize();
+ break;

2) Can this be part of PgStat_StatDBEntry, similar to tables,
functions and subworkers. It might be more appropriate to have it
there instead of having another global variable.
+ * Stats of prepared transactions should be displayed
+ * at either commit prepared or rollback prepared time, even when it's
+ * after the server restart. We have the apply worker send those statistics
+ * to the stats collector at prepare time and the startup process restore
+ * those at restart if necessary.
+ */
+static HTAB *subWorkerPreparedXactSizeHash = NULL;
+
+/*

Regards,
Vignesh