On Thu, Feb 20, 2025 at 1:30 PM Ajin Cherian <itsa...@gmail.com> wrote: > > On Fri, Feb 14, 2025 at 6:18 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Wed, Feb 12, 2025 at 10:41 AM Ajin Cherian <itsa...@gmail.com> wrote: > > > > > > On Wed, Jan 29, 2025 at 9:31 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > > > > Hi Ajin, > > > > > > Some review comments for patch v12-0001. > > > > > > ====== > > > Commit message > > > > > > 1. > > > Track transactions which have snapshot changes with a new flag > > > RBTXN_HAS_SNAPSHOT_CHANGES > > > > > > ~ > > > > > > The commit message only says *what* it does, but not *why* this patch > > > even exists. TBH, I don't understand why this patch needs to be > > > separated from your patch 0002, because 0001 makes no independent use > > > of the flag, nor is it separately tested. > > > > > > Anyway, if it is going to remain separated then IMO at least the the > > > message should explain the intended purpose e.g. why the subsequent > > > patches require this flagged info and how they will use it. > > > > > > > > > Fixed. > > > > > > > I still can't get from 0001's commit message the reason for tracking > > the snapshot changes separately. Also, please find my comments for > > 0002's commit message. > > > > > When most changes in a transaction are unfilterable, the overhead of > > starting a transaction for each record is significant. > > > > > > > Can you tell what is the exact overhead by testing it? IIRC, that was > > the initial approach. It is better if you can mention in the commit > > message what was overhead. It will be easier for reviewers. > > > > > > > To reduce this overhead a hash cache of relation file locators is > > created. Even then a hash search for every record before recording has > > considerable overhead especially for use cases where most tables in an > > instance are published. > > > > > > > Again, can you share the link of performance data for this overhead > > and if you have not published then please share it and also mention it > > in commit message? > > > > I compared the patch 1 which does not employ a hash cache and has the > overhead of starting a transaction every time the filter is checked. >
Just to clarify, by patch 1, I mean the first patch in this thread (v1) posted by Lie Jie here - (1) 1 - https://www.postgresql.org/message-id/CAGfChW5Qo2SrjJ7rU9YYtZbRaWv6v-Z8MJn=dqnx4ucsqde...@mail.gmail.com regards, Ajin Cherian Fujitsu Australia
