Re: Failed transaction statistics to measure the logical replication progress

On Tue, Nov 9, 2021 at 5:05 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Tuesday, November 9, 2021 12:08 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> > On Fri, Nov 5, 2021 at 7:11 PM osumi(dot)takamichi(at)fujitsu(dot)com
> > <osumi(dot)takamichi(at)fujitsu(dot)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