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. 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. 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.
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.
--
Best Wishes,
Ashutosh Bapat
