Thank you, Masahiko-san, for your review comments. Please find my responses inline below:
On Tue, Jun 30, 2026 at 4:32 AM Masahiko Sawada <[email protected]> wrote: > > On Mon, Jun 29, 2026 at 2:08 AM Ashutosh Sharma <[email protected]> wrote: > > > Thank you for updating the patch! The new column name output_bytes > looks good to me. > > Here are some comments: > > memset(nulls, 0, sizeof(nulls)); > values[0] = LSNGetDatum(lsn); > + outputBytes += sizeof(XLogRecPtr); > values[1] = TransactionIdGetDatum(xid); > + outputBytes += sizeof(TransactionId); > : > : > /* ick, but cstring_to_text_with_len works for bytea perfectly > fine */ values[2] = > PointerGetDatum(cstring_to_text_with_len(ctx->out->data, > ctx->out->len)); > + outputBytes += ctx->out->len; > > I'm not sure that we should include these values to the new statistics > since they are not the data produced by output plugins. I think it's > better to collect the amount of data output plugins produced and get > it in OutputPluginWrite(). What do you think? > Agreed. The values added by the SQL interface, such as the LSN and XID columns, are not produced by the output plugin, so they should not be counted as output bytes. I also agree that output_bytes should be incremented in OutputPluginWrite(), since that is the common path for both the SQL and walsender consumers and ctx->out contains the data produced by the output plugin there. I have implemented these changes in the attached patch, please have a look and let me know your thoughts. > --- > - elog(DEBUG2, "UpdateDecodingStats: updating stats %p %" PRId64 " > %" PRId64 " %" PRId64 " %" PRId64 " %" PRId64 " %" PRId64 " %" PRId64 > " %" PRId64 " %" PRId64, > + elog(DEBUG2, "UpdateDecodingStats: updating stats %p %" PRId64 " > %" PRId64 " %" PRId64 " %" PRId64 " %" PRId64 " %" PRId64 " %" PRId64 > " %" PRId64 " %" PRId64 " %" PRId64, > > This log message is already quite hard to read, and I'm concerned that > simply dumping ten unlabeled numbers is no longer helpful for > debugging. If all of these metrics are indeed still useful, I would > propose adding explicit labels for each one. Otherwise, we might as > well remove this elog() entirely. > I have added labels for each field emitted by this debug message, as I am not quite sure that removing it completely would be a good idea. > --- > @@ -1979,6 +1980,7 @@ UpdateDecodingStats(LogicalDecodingContext *ctx) > repSlotStat.mem_exceeded_count = rb->memExceededCount; > repSlotStat.total_txns = rb->totalTxns; > repSlotStat.total_bytes = rb->totalBytes; > + repSlotStat.output_bytes = rb->outputBytes; > > Since output_bytes is now the tenth field we accumulate for > replication slot statistics, it seems like a good opportunity to > improve its maintainability. Instead of adding fields individually, we > could define a common structure for these shared metrics so that both > PgStat_StatReplSlotEntry and ReorderBuffer can embed it. This way, > UpdateDecodingStats() could simply assign the collected struct and > clear it using MemSet(). > I agree that this would improve maintainability. PgStat_StatReplSlotEntry now has 13 fields, and 10 of them are the decoding counters that are also tracked in ReorderBuffer, so a common counter-only structure sounds like the right direction. That said, this is not specific to the output_bytes field being added here. It is more of a refactoring of the boundary between logical decoding and pgstat statistics. To keep this patch focused, I think it would be better to handle that as a separate follow-up patch, possibly with a new thread once this work is done. Please let me know your thoughts on this? -- With Regards, Ashutosh Sharma.
v20260630-0001-Report-output-bytes-in-pg_stat_replication_slots.patch
Description: Binary data
