RE: Failed transaction statistics to measure the logical replication progress

On Wednesday, November 10, 2021 3:43 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> 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
I really appreciate your attention and review.

> 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?
I thought those were same and currently it gets incremented when an error of apply occurs.
Then, it equals to the number of total error. May I have the case when we
get different values between those two ? I can be missing something.

> 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?
By the error_bytes, you can see the consumed resources that
were acquired during apply but the applying processing stopped by some error.
On the other hand, abort_bytes displays bytes used for ROLLBACK PREPARED
and stream_abort processing. That's what I intended.


> 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. 
I'm so sorry to make you confused.

I'll just delete the top part that handles streaming
bytes calculation in the update_apply_change_size().
It's because now that there is a specific structure to recognize each streaming xid
and save transaction size there, which makes the top part in question useless.

> 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.
As described above, I'll fix this part and
related codes mainly streaming related codes in the next version.


Best Regards,
    Takamichi Osumi