Re: Report bytes and transactions actually sent downtream - Mailing list pgsql-hackers

On Tue, Oct 28, 2025 at 12:46 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Mon, Oct 27, 2025 at 4:47 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > Few comments:
> >
> > 1)
> > pgoutput_truncate:
> >
> > if (nrelids > 0)
> > {
> > OutputPluginPrepareWrite(ctx, true);
> > logicalrep_write_truncate(ctx->out,
> >   xid,
> >   nrelids,
> >   relids,
> >   change->data.truncate.cascade,
> >   change->data.truncate.restart_seqs);
> > OutputPluginWrite(ctx, true);
> > }
> > + else
> > + ctx->stats->filteredBytes += ReorderBufferChangeSize(change);
> > +
> >
> > It seems that filteredBytes are only counted for TRUNCATE when nrelids
> > is 0. Can nrelids only be 0 or same as nrelations?
> >
> > The below code makes me think that nrelids can be any number between 0
> > and nrelations, depending on which relations are publishable and which
> > supports publishing TRUNCATE. If that’s true, shouldn’t we count
> > filteredBytes in each such skipped case?
>
> IIIUC, you are suggesting that we should add
> ReorderBufferChangeSize(change) for every relation which is not part
> of the publication or whose truncate is not published.

No, that will be wrong.

> I think that
> won't be correct since it can lead to a situation where filtered bytes
> > total bytes which should never happen. Even if there is a single
> publishable relation whose truncate is published, the change should
> not be considered as filtered since something would be output
> downstream.

Yes, the entire change should not be treated as filtered. The idea is
that, for example, if there are 20 relations belonging to different
publications and only one of them supports publishing TRUNCATE, then
when a TRUNCATE is triggered on all, the data for that one relation
should be counted as sent (which is currently happening based on
nrelids), while the data for the remaining 19 should be considered
filtered — which is not happening right now.

> Otherwise filtered bytes as well as sent bytes both will
> be incremented causing an inconsistency (which would be hard to notice
> since total bytes - filtered bytes has something to do with the sent
> bytes but the exact correlation is hard to grasp in a formula).
>
> We may increment filteredBytes by sizeof(OID) for every relation we
> skip here OR by ReoderBufferChangeSize(change) if all the relations
> are filtered, but that's too much dependent on how the WAL record is
> encoded; and adding that dependency in an output plugin code seems
> hard to manage.
>

Yes, that was the idea, to increment filteredBytes in this way. But I
see your point. I can’t think of a better solution at the moment. If
you also don’t have any better ideas, then at least adding a comment
in this function would be helpful. Right now, it looks like we
overlooked the fact that some relationships should contribute to
filteredBytes while others should go to sentBytes.

> If you are suggesting something else, maybe sharing actual code
> changes would help.
>
> >
> >
> > 2)
> > + int64 filteredBytes; /* amount of data from reoder buffer that was
> >
> > reoder --> reorder
>
> Done.
>
> >
> > 3)
> > One small nitpick:
> >
> > + /*
> > + * If output plugin has chosen to maintain its stats, update the amount of
> > + * data sent downstream.
> > + */
> > + if (ctx->stats)
> > + ctx->stats->sentBytes += ctx->out->len + sizeof(XLogRecPtr) +
> > sizeof(TransactionId);
> >
> > The way sentBytes is updated here feels a bit unnatural; we’re adding
> > the lengths for values[2], then [0], and then [1]. Would it be cleaner
> > to introduce a len[3] array similar to the existing values[3] and
> > nulls[3] arrays? We could initialize len[i] alongside values[i], and
> > later just sum up all three elements when updating
> > ctx->stats->sentBytes. It would be easier to understand as well.
>
> Instead of an array of length 3, we could keep a counter sentBytes to
> accumulate all lengths. It will be assigned to ctx->stats->sentBytes
> at the end if ctx->stats != NULL. But that might appear as if we are
> performing additions even if it won't be used ultimately. That's not
> true, since this plugin will always maintain stats. Changed that way.
>

Looks good.

Apart from the above discussion, I have no more comments on this patch.

thanks
Shveta