Re: Proposal: Filter irrelevant change before reassemble transactions during logical decoding

On Thu, 20 Mar 2025 at 18:09, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Thu, Mar 13, 2025 at 5:49 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > Moving this patch to the next CF as this patch needs more design level
> > inputs which may not be feasible in this CF but do continue to review
> > the patch.
> >
> > regards,
> > Ajin Cherian
> > Fujitsu Australia
>
> Rebased the patch as it no longer applied.
>
Hi Ajin,

I have reviewed patch v16-0001, here are my comments:

1. There are some places where comments are more than 80 columns
window. I think it should be <=80 as per [1].
a. initial comment in reorderbuffer.c
+ * Filtering is temporarily suspended for a sequence of changes
(set to 100
+ * changes) when an unfilterable change is encountered. This
strategy minimizes
+ * the overhead of hash searching for every record, which is
beneficial when the
+ * majority of tables in an instance are published and thus
unfilterable. The
+ * threshold of 100 was determined to be the optimal balance
based on performance
+ * tests.
+ *
+ * Additionally, filtering is paused for transactions
containing WAL records
+ * (INTERNAL_SNAPSHOT, COMMAND_ID, or INVALIDATION) that modify
the historical
+ * snapshot constructed during logical decoding. This pause is
necessary because
+ * constructing a correct historical snapshot for searching publication
+ * information requires processing these WAL records.

b.
+ if (!has_tuple)
+ {
+ /*
+ * Mapped catalog tuple without data, emitted while catalog table was in
+ * the process of being rewritten. We can fail to look up the
+ * relfilenumber, because the relmapper has no "historic" view, in
+ * contrast to the normal catalog during decoding. Thus repeated rewrites
+ * can cause a lookup failure. That's OK because we do not decode catalog
+ * changes anyway. Normally such tuples would be skipped over below, but
+ * we can't identify whether the table should be logically logged without
+ * mapping the relfilenumber to the oid.
+ */
+ return NULL;
+ }

2. I think, 'rb->unfiltered_changes_count' should be initialised in
the function 'ReorderBufferAllocate'. While debugging I found that the
initial value of rb->unfiltered_changes_count is 127. I think it
should be set to '0' inside 'ReorderBufferAllocate'. Am I missing
something here?
I have also added the same comment in point 1. in [2].

Also, please ignore point 2. in email [2] a crash happened because I
was testing it without doing a clean build. Sorry for the
inconvenience.

[1]: https://www.postgresql.org/docs/devel/source-format.html
[2]: https://www.postgresql.org/message-id/CANhcyEUF1HzDRj_fJAGCqBqNg7xGVoATR7XgR311xq8UvBg9tg%40mail.gmail.com

Thanks and Regards,
Shlok Kyal