On Thu, Feb 20, 2025 at 1:30 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Feb 14, 2025 at 6:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Feb 12, 2025 at 10:41 AM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > On Wed, Jan 29, 2025 at 9:31 AM Peter Smith <smithpb2250@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=dQNx4uCSqDEOHA@mail.gmail.com
regards,
Ajin Cherian
Fujitsu Australia
