MIME-Version: 1.0
References: 
 <CA+HiwqH+9NK5m-=BOYpzjH5UEpeWjT_3HEm4XBYNrda88naGMQ@mail.gmail.com>
 <8cf1ea71-b609-00e8-b8f0-c85ba61b7312@pgmasters.net>
 <4dbad191-9b40-4bb6-8940-d18f2e93c3ce@www.fastmail.com>
 <ME3P282MB16678160F5EA09DCB962474EB6B69@ME3P282MB1667.AUSP282.PROD.OUTLOOK.COM>
 <5b9fceed-8f91-467f-a64e-2134c5c3f496@www.fastmail.com>
 <YBjc/SVkHTcCpxbR@paquier.xyz>
 <MEYP282MB16699D041638CE2F8284701FB6B59@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
 <MEYP282MB1669F5063EC084C0C5854EB1B6B59@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
 <52edc9d2-8e52-4cde-9a15-991961b78387@www.fastmail.com>
 <CACawEhW0NUMHHAQjVP_xM8gWXKvy9haK9nKhC-H=Qn2yTCv-=g@mail.gmail.com>
 <9d5ef845-391a-4c66-b298-0f594d9e6828@www.fastmail.com>
 <f6566ad9-8979-4056-a4ce-24b09f389ede@www.fastmail.com>
 <CAH2L28tjTYctr+eu6Vg5U6iihVhe6P+qKnB_diMGy4=CfwXzdg@mail.gmail.com>
 <CAH2L28sS8=jY6Lj2-xVu6exxQ9gK8uGZr9jf64zDw9hacMrkyA@mail.gmail.com>
 <7e4b5c48-a3d3-4b7e-b455-2d50c60a0ab7@www.fastmail.com>
 <57373e8b-1264-cd37-404e-8edbcf7884cc@enterprisedb.com>
 <d7483e04-7c5f-4881-a9a0-2363b2e52f78@www.fastmail.com>
 <CAH2L28tvKGLO9ZRFsaA5msXqZzF6wHgxgXNvnCQ2dRQfs1T8XQ@mail.gmail.com>
 <b02b43c7-3b84-4ca4-8218-94ea486c2fce@www.fastmail.com>
 <CAA4eK1Lu7oPHm2j=nLeqZLVoro76E0EWvH+5wmGG39iJNBzUog@mail.gmail.com>
 <86d76e89-9a96-411a-8077-f8918cbae2e1@www.fastmail.com>
 <CAHut+PvrJHz_9HG5jvqHWe=FZLz9A+CxyJDyZKsHmY8THU+pjg@mail.gmail.com>
 <3dbf7241-a417-45c9-9a0e-04a079de9b9f@www.fastmail.com>
 <CAHut+PvCWX8WauZJkXfgL08nn8+rDsTdxQinhyL1z2+E7r06fw@mail.gmail.com>
 <CAA4eK1LPh+KCALU1Eo4itWq+bYU6NzUGr4dwypMynM3Skjvv5A@mail.gmail.com>
 <0ca5b27f-edf8-4ef9-a9e2-533216890f62@www.fastmail.com>
 <CAHut+Ps3GgPKUJ2npfY4bQdxAmYW+yQin+hQuBsMYvX=kBqEpA@mail.gmail.com>
 <532a18d8-ce90-4444-8570-8a9fcf09f329@www.fastmail.com>
 <CAA4eK1+xQb06NGs6Y7OzwMtKYYixEqR8tdWV5THAVE4SAqNrDg@mail.gmail.com>
 <CAHut+Ps5j7mkO0xLmNW=kXh0eezGoKyzBCiQc9bfkCiM_MVDrg@mail.gmail.com>
 <d65b0801-34bb-4db2-b0d4-8e39852c9f9d@www.fastmail.com>
 <CAA4eK1JXYQ3B7kLXwi2buDP-LZ_qnqZDf=nDTPt_c8hHkyztww@mail.gmail.com>
 <CAA4eK1+qd1OATDSZCrjebZx+3qm1=RmF-fDU_1YsT__XYnxvVA@mail.gmail.com>
 <CAHut+Pvn+S8upyrkBi3m=rc4b5Ttw3xGsg09rNec=qJK0pD5mQ@mail.gmail.com>
 <CAA4eK1J1FNhDhj85Am1X_5VTxKOYo_nSBMuuMteB3n5YVPMdpw@mail.gmail.com>
 <CAHut+PtbKUmWNjTEDMVhP9abjgph6JB7vUFYF7ioa_4uRPZb=A@mail.gmail.com>
 <CAA4eK1LPhubcm4o1Xg0RwZdatqwfz3KKYcdun8QvRyqFthyJ1g@mail.gmail.com>
 <CAHut+PtFxoP477E8odkxkpyDoH_tiNBSiJH-j88NR728nxPErQ@mail.gmail.com>
 <CAA4eK1+tio46goUKBUfAKFsFVxtgk8nOty=TxKoKH-gdLzHD2g@mail.gmail.com>
 <CAHut+PtdRU74kUZd2RugdCqB0Ya1L+BbZk45czM5ufQs5E8jmw@mail.gmail.com>
 <CAHut+PsgRHymwLhJ9t3By6+KNaVDzfjf6Y4Aq=JRD-y8t1mEFg@mail.gmail.com>
 <0c2464d4-65f4-4d91-aeb2-c5584c1350f5@www.fastmail.com>
 <CAFPTHDb7bpkuc4SxaL9B5vEvF2aEi0EOERdrG+xgVeAyMJsF=Q@mail.gmail.com>
 <CAFPTHDYZk4P1kD=y6qMBnzh66+CgnfV3VTK7yS-Tk1eGrFOVZg@mail.gmail.com>
 <CAA4eK1KNPWVuV-bwH8J8LtZWzh+PpQ+Rz0Lab2mpL=BA97hi3A@mail.gmail.com>
 <CAFiTN-sGR_MbOVhcdYgNhWe-GRzPg12q=j7S00zXfW_OzQDPuQ@mail.gmail.com>
 <CAFPTHDZmRdRZ3hz4YTQ8XqeSzzdsHKqwYQb+6Q5MG-bRX0nc-g@mail.gmail.com>
 <CAFiTN-uGr4jG2LTOz_nU2tKoPvgQSHoScMTsFaKVxbFeNAkvYA@mail.gmail.com>
 <CAA4eK1LjrEFOrpZ7t_uCp4-qGA3VyNaL2h_+wY26=3uEyE66vg@mail.gmail.com>
 <CAFiTN-vauBL5fWZRSO0XPcO4ATT2z4epemEDKmeAHXdTorUMtg@mail.gmail.com>
 <CAA4eK1KRZGrCnhRCusH-k0K8RZ2qL=dp2eXLSDJCMtWHBcRygg@mail.gmail.com>
 <CAFiTN-vwBjy+eR+iodkO5UVN5cPv_xx1=s8ehzgCRJZA+AztAA@mail.gmail.com>
 <CAFiTN-ug0kSDJyYxkz9L5UMLjf1vA+7cnY8Co7zU=Vom1MfG7A@mail.gmail.com>
 <CAFPTHDbVR+097SSvSM8KCjyvqrKq13NoSgCqQj06d3BYVZREVA@mail.gmail.com>
 <CAA4eK1JgBRRZ9LnSzgHYJ0UM4n1guMfvkmRj8xm9iaZW8Wdj=w@mail.gmail.com>
 <CAFPTHDaUQHdZ0RQYM8dVeBHAnvBZRYqDCBBK9r6GxBX0HL1xGQ@mail.gmail.com>
 <CAFPTHDaCeMuHnz7SonsvqDxHvcF7XD-LRLjVCqEBcyk5QgyMVg@mail.gmail.com>
 <CAFPTHDZH+uuKU0uRXFZQri1v96-YnPAu6SorhZycM+=GCKvYQA@mail.gmail.com>
 <CAFiTN-uXTHcFkODayUsSMFp-7XLXQk=sOV2mJi6tDv-TjQAEBw@mail.gmail.com>
In-Reply-To: 
 <CAFiTN-uXTHcFkODayUsSMFp-7XLXQk=sOV2mJi6tDv-TjQAEBw@mail.gmail.com>
From: Ajin Cherian <itsajin@gmail.com>
Date: Tue, 12 Oct 2021 13:33:12 +1100
Message-ID: 
 <CAFPTHDYfipttorJ-SkWerWHDP_GmtAipW5LLLeEt5Y3+PptVPw@mail.gmail.com>
Subject: Re: row filtering for logical replication
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Euler Taveira <euler@eulerto.com>,
	Peter Smith <smithpb2250@gmail.com>, Rahila Syed <rahilasyed90@gmail.com>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	=?UTF-8?B?w5ZuZGVyIEthbGFjxLE=?= <onderkalaci@gmail.com>,
	japin <japinli@hotmail.com>, Michael Paquier <michael@paquier.xyz>,
	David Steele <david@pgmasters.net>, Craig Ringer <craig@2ndquadrant.com>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 Amit Langote <amitlangote09@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAFPTHDYfipttorJ-SkWerWHDP_GmtAipW5LLLeEt5Y3%2BPptVPw%40mail.gmail.com>
Precedence: bulk

On Tue, Oct 12, 2021 at 1:37 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Oct 6, 2021 at 2:33 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Sat, Oct 2, 2021 at 5:44 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > I have for now also rebased the patch and merged the first 5 patches
> > > into 1, and added my changes for the above into the second patch.
> >
> > I have split the patches back again, just to be consistent with the
> > original state of the patches. Sorry for the inconvenience.
>
> Thanks for the updated version of the patch, I was looking into the
> latest version and I have a few comments.
>
>
> +        if ((att->attlen == -1 &&
> VARATT_IS_EXTERNAL_ONDISK(tmp_new_slot->tts_values[i])) &&
> +                (!old_slot->tts_isnull[i] &&
> +                    !(VARATT_IS_EXTERNAL_ONDISK(old_slot->tts_values[i]))))
> +        {
> +            tmp_new_slot->tts_values[i] = old_slot->tts_values[i];
> +            newtup_changed = true;
> +        }
>
> If the attribute is stored EXTERNAL_ONDIS on the new tuple and it is
> not null in the old tuple then it must be logged completely in the old
> tuple, so instead of checking
> !(VARATT_IS_EXTERNAL_ONDISK(old_slot->tts_values[i]), it should be
> asserted,
>
>
> +    heap_deform_tuple(newtuple, desc, new_slot->tts_values,
> new_slot->tts_isnull);
> +    heap_deform_tuple(oldtuple, desc, old_slot->tts_values,
> old_slot->tts_isnull);
> +
> +    if (newtup_changed)
> +        tmpnewtuple = heap_form_tuple(desc, tmp_new_slot->tts_values,
> new_slot->tts_isnull);
> +
> +    old_matched = pgoutput_row_filter(relation, NULL, oldtuple, entry);
> +    new_matched = pgoutput_row_filter(relation, NULL,
> +                                      newtup_changed ? tmpnewtuple :
> newtuple, entry);
>
> I do not like the fact that, first we have deformed the tuples and we
> are again using the HeapTuple
> for expression evaluation machinery and later the expression
> evaluation we do the deform again.
>
> So why don't you use the deformed tuple as it is to store as a virtual tuple?
>
> Infact, if newtup_changed is true then you are forming back the tuple
> just to get it deformed again
> in the expression evaluation.
>
> I think I have already given this comment on the last version.

Right, I only used the deformed tuple later when it was written to the
stream. I will modify this as well.

regards,
Ajin Cherian
Fujitsu Australia