Re: Failed transaction statistics to measure the logical replication progress

On Tue, Nov 9, 2021 at 5:05 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Tuesday, November 9, 2021 12:08 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
> > On Fri, Nov 5, 2021 at 7:11 PM osumi.takamichi@fujitsu.com
> > <osumi.takamichi@fujitsu.com> wrote:
> > >
> >
> > I did a quick scan through the latest v8 patch and noticed the following things:
> I appreciate your review !
>

I have reviewed some part of the patch and I have a few comments

1.
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>error_count</structfield> <type>bigint</type>
+      </para>
+      <para>
+       Number of transactions that failed to be applied by the table
+       sync worker or main apply worker in this subscription.
+       </para></entry>
+     </row>

The error_count, should be number of transaction failed to applied? or
it should be number of error?

2.
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>error_bytes</structfield> <type>bigint</type>
+      </para>

How different is error_bytes from the abort_bytes?

3.
+    {
+        size += *extra_data->stream_write_len;
+        add_apply_error_context_xact_size(size);
+        return;
+    }

From apply_handle_insert(), we are calling update_apply_change_size(),
and inside this function we are dereferencing
*extra_data->stream_write_len.  Basically, stream_write_len is in
integer pointer and the caller hasn't allocated memory for that and
inside update_apply_change_size, we are directly dereferencing the
pointer, how this can be correct.  And I also see that in the whole
patch stream_write_len, is never used as lvalue so without storing
anything into this why are we trying to use this as rvalue here?  This
is clearly an issue.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com