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

From vignesh C
Subject Re: Failed transaction statistics to measure the logical replication progress
Date
Msg-id CALDaNm130T4jr2RvpgY19SCY87UQ75wVbjrhjGj43bHV=9-LpQ@mail.gmail.com
Whole thread Raw
In response to Re: Failed transaction statistics to measure the logical replication progress  (vignesh C <vignesh21@gmail.com>)
Responses RE: Failed transaction statistics to measure the logical replication progress
List pgsql-hackers
On Wed, Nov 10, 2021 at 3:43 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Nov 9, 2021 at 5:05 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.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



pgsql-hackers by date:

Previous
From: Alvaro Herrera
Date:
Subject: Re: remove spurious CREATE INDEX CONCURRENTLY wait
Next
From: Anton Voloshin
Date:
Subject: Re: fix warnings in 9.6, 10, 11's contrib when compiling without openssl