Re: Report bytes and transactions actually sent downtream

On 7/3/26 1:44 AM, Ashutosh Sharma wrote:

> Thank you for your review comments.
>
> I have swapped the patch order - please see attached.
>
> In the refactoring patch, I also ensured that reorderbuffer.h does not
> depend on pgstat.h. To make that possible, I introduced a shared
> header, src/include/replication/replslot_stats.h, where
> PgStat_ReplSlotStats is defined. Both pgstat.h and reorderbuffer.h
> include that header. Hope that helps!
>
> --
> With Regards,
> Ashutosh Sharma.

Hi Ashutosh!

Thanks for the updated patches. I reviewed the latest two as
part of the July patch review workshop:

First, I wasn't able to cleanly apply the refactoring patch against the
current tree. It could be an issue on my end, but it may need to be
checked again.

Secondly, this was mentioned earlier in the thread and handled for
the SQL API path but I believe an excess 25 bytes is still being
counted by output_bytes in the WALsender path because of an
unaccounted for header.

In WalSndPrepareWrite(), we call pq_sendbyte() and pq_sendint64()
twice in order to send some header data. In order to be consistent
with the renaming to "output_bytes" I think that should not
be included. One way this could be fixed is through an offset that
simply accounts for this header data, and subtracts it from output_bytes:

/* OutputPluginPrepareWrite */
ctx->prepare_write(ctx, ctx->write_location, ctx->write_xid, last_write);
ctx->prepared_write_offset = ctx->out->len;

/* OutputPluginWrite */
ctx->reorder->stats.output_bytes += ctx->out->len -
ctx->prepared_write_offset;

Finally, I wonder if the test coverage is sufficient. We only
assert that output_bytes is > 0 and that it is equal to 0 after a reset
happens. Should we also check that it is equal to the expected
number of bytes for the output_plugin?

From what I can tell, in the SQL API path, we know the data comes
from the buffer contents. So maybe additional verification
could do something along the lines of:

length output plugin produced data == output_bytes delta

I'm fairly new to patch reviews and the postgres community
so I'm curious what your thoughts are!

Is the new code I suggested to remove the 25 bytes and potentially
more tests worth it?

Best,
Adi Gollamudi