On Wed, Oct 29, 2025 at 8:25 PM Ashutosh Bapat <[email protected]> wrote: > > On Wed, Oct 29, 2025 at 9:14 AM shveta malik <[email protected]> wrote: > > > > On Tue, Oct 28, 2025 at 12:46 PM Ashutosh Bapat > > <[email protected]> wrote: > > > > > > On Mon, Oct 27, 2025 at 4:47 PM shveta malik <[email protected]> > > > 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. > > I noticed that we do something similar while filtering columns. I > think we need to add a comment in that code as well. How about > something like below? > > diff --git a/src/backend/replication/pgoutput/pgoutput.c > b/src/backend/replication/pgoutput/pgoutput.c > index 4b35f2de6aa..f2d6e20a702 100644 > --- a/src/backend/replication/pgoutput/pgoutput.c > +++ b/src/backend/replication/pgoutput/pgoutput.c > @@ -1621,7 +1621,12 @@ pgoutput_change(LogicalDecodingContext *ctx, > ReorderBufferTXN *txn, > > OutputPluginPrepareWrite(ctx, true); > > - /* Send the data */ > + /* > + * Send the data. Even if we end up filtering some columns > while sending the > + * message, we won't consider the change, as a whole, to be > filtered out. > + * Instead the filtered columns will be reflected as a smaller > sentBytes > + * count. > + */ > switch (action) > { > case REORDER_BUFFER_CHANGE_INSERT: > @@ -1728,6 +1733,13 @@ pgoutput_truncate(LogicalDecodingContext *ctx, > ReorderBufferTXN *txn, > > change->data.truncate.cascade, > > change->data.truncate.restart_seqs); > OutputPluginWrite(ctx, true); > + > + /* > + * Even if we filtered out some relations, we still > send a TRUNCATE > + * message for the remaining relations. Since the > change, as a whole, is > + * not filtered out, we don't count modify > filteredBytes. The filtered > + * out relations will be reflected as a smaller sentBytes > count. > + */ > } > else > ctx->stats->filteredBytes += ReorderBufferChangeSize(change); >
> + * not filtered out, we don't count modify filteredBytes. The > filtered Something is wrong in this sentence. Also, regarding "The filtered out relations will be reflected as a smaller sentBytes count." Can you please point me to the code where it happens? From what I have understood, pgoutput_truncate() completely skips the relations which do not support publishing truncate. Then it sends 'BEGIN', then schema info of non-filtered relations and then TRUNCATE for non-filtered relations (based on nrelids). thanks Shveta
