Message-ID: <2b8471f9-3849-c91f-ef82-9181d4bca0a7@enterprisedb.com>
Date: Fri, 11 Mar 2022 13:50:29 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: Column Filtering in Logical Replication
Content-Language: en-US
To: Amit Kapila <amit.kapila16@gmail.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>
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>
 <CAA4eK1J9UDnM-xN7viu4eO_DDL_PqMG9F6ZsskqEU7snujXi+A@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <CAA4eK1J9UDnM-xN7viu4eO_DDL_PqMG9F6ZsskqEU7snujXi+A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/2b8471f9-3849-c91f-ef82-9181d4bca0a7%40enterprisedb.com>
Precedence: bulk


On 3/11/22 10:52, Amit Kapila wrote:
> 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.

Does the call really require that? Also, I'm not sure why we'd need to
look at pubviaroot - that's already considered earlier when calculating
publish_as_relid, here we just need to know the relationship of the two
OIDs (if one is ancestor/child of the other).

> I think it would be
> good if we can avoid an additional call to get_partition_ancestors()
> as it could be costly.

Maybe. OTOH we only should do this only very rarely anyway.

> 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.
> 

I'm not sure what exactly are you proposing. Maybe try coding it? That's
probably faster than trying to describe what the code might do ...


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company