MIME-Version: 1.0
References: <202201120041.p24wvsfcsope@alvherre.pgsql> <ca91dc91-80ba-e954-213e-b4170a6160f5@enterprisedb.com>
 <d37d6709-7e9c-f422-f925-fa66ce1fc0e8@enterprisedb.com> <CAA4eK1KONbrck=hQTFSFgwio5rqu-82KZba2HLE_AcVa_RWYcQ@mail.gmail.com>
 <cac80284-7196-6c77-adb9-ef0976a3e21f@enterprisedb.com> <CAA4eK1JyoVty_BZOMJdx7VMkBTadvv8oOYrh9xTQ5BY+k-3+OA@mail.gmail.com>
 <d26d24dd-2fab-3c48-0162-2b7f84a9c893@enterprisedb.com> <CAA4eK1JJ5qgqbjfYQU9F4R1+OABxec-DQechQXus0fanDjaAVg@mail.gmail.com>
 <OS0PR01MB571614E4A39DD33669D5AFD6940A9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <e4b0fb99-3d74-7333-1e71-74f990b641ee@enterprisedb.com> <78b8d385-57cb-9a3f-cfe5-9f300f15eced@enterprisedb.com>
 <822a8e40-287c-59ff-0ea9-35eb759f4fe6@enterprisedb.com>
In-Reply-To: <822a8e40-287c-59ff-0ea9-35eb759f4fe6@enterprisedb.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Fri, 11 Mar 2022 15:22:19 +0530
Message-ID: <CAA4eK1J9UDnM-xN7viu4eO_DDL_PqMG9F6ZsskqEU7snujXi+A@mail.gmail.com>
Subject: Re: Column Filtering in Logical Replication
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, 
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>, 
	Alvaro Herrera <alvherre@alvh.no-ip.org>, Justin Pryzby <pryzby@telsasoft.com>, 
	Rahila Syed <rahilasyed90@gmail.com>, Peter Smith <smithpb2250@gmail.com>, 
	pgsql-hackers <pgsql-hackers@postgresql.org>, 
	"shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1J9UDnM-xN7viu4eO_DDL_PqMG9F6ZsskqEU7snujXi%2BA%40mail.gmail.com>
Precedence: bulk

On Fri, Mar 11, 2022 at 7:26 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> On 3/10/22 20:10, Tomas Vondra wrote:
> >
> >
> > FWIW I think the reason is pretty simple - pgoutput_row_filter_init is
> > broken. It assumes you can just do this
> >
> > rftuple = SearchSysCache2(PUBLICATIONRELMAP,
> >                           ObjectIdGetDatum(entry->publish_as_relid),
> >                           ObjectIdGetDatum(pub->oid));
> >
> > if (HeapTupleIsValid(rftuple))
> > {
> >     /* Null indicates no filter. */
> >     rfdatum = SysCacheGetAttr(PUBLICATIONRELMAP, rftuple,
> >                               Anum_pg_publication_rel_prqual,
> >                               &pub_no_filter);
> > }
> > else
> > {
> >     pub_no_filter = true;
> > }
> >
> >
> > and pub_no_filter=true means there's no filter at all. Which is
> > nonsense, because we're using publish_as_relid here - the publication
> > may not include this particular ancestor, in which case we need to just
> > ignore this publication.
> >
> > So yeah, this needs to be reworked.
> >
>
> I spent a bit of time looking at this, and I think a minor change in
> get_rel_sync_entry() fixes this - it's enough to ensure rel_publications
> only includes publications that actually include publish_as_relid.
>

Thanks for looking into this. I think in the first patch before
calling get_partition_ancestors() we need to ensure it is a partition
(the call expects that) and pubviaroot is true. I think it would be
good if we can avoid an additional call to get_partition_ancestors()
as it could be costly. I wonder why it is not sufficient to ensure
that publish_as_relid exists after ancestor in ancestors list before
assigning the ancestor to publish_as_relid? This only needs to be done
in case of (if (!publish)). I have not tried this so I could be wrong.

-- 
With Regards,
Amit Kapila.