On Thu, 20 Mar 2025 at 18:09, Ajin Cherian <itsa...@gmail.com> wrote: > > On Thu, Mar 13, 2025 at 5:49 PM Ajin Cherian <itsa...@gmail.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