Message-ID: <78b8d385-57cb-9a3f-cfe5-9f300f15eced@enterprisedb.com>
Date: Thu, 10 Mar 2022 20:10:01 +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
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
 Amit Kapila <amit.kapila16@gmail.com>
Cc: 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>
In-Reply-To: <e4b0fb99-3d74-7333-1e71-74f990b641ee@enterprisedb.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/78b8d385-57cb-9a3f-cfe5-9f300f15eced%40enterprisedb.com>
Precedence: bulk


On 3/10/22 19:17, Tomas Vondra wrote:
> On 3/9/22 11:12, houzj.fnst@fujitsu.com wrote:
>> Hi,
>>
>> Here are some tests and results about the table sync query of
>> column filter patch and row filter.
>>
>> 1) multiple publications which publish schema of parent table and partition.
>> ----pub
>> create schema s1;
>> create table s1.t (a int, b int, c int) partition by range (a);
>> create table t_1 partition of s1.t for values from (1) to (10);
>> create publication pub1 for all tables in schema s1;
>> create publication pub2 for table t_1(b);
>>
>> ----sub
>> - prepare tables
>> CREATE SUBSCRIPTION sub CONNECTION 'port=10000 dbname=postgres' PUBLICATION pub1, pub2;
>>
>> When doing table sync for 't_1', the column list will be (b). I think it should
>> be no filter because table t_1 is also published via ALL TABLES IN SCHMEA
>> publication.
>>
>> For Row Filter, it will use no filter for this case.
>>
>>
>> 2) one publication publishes both parent and child
>> ----pub
>> create table t (a int, b int, c int) partition by range (a);
>> create table t_1 partition of t for values from (1) to (10)
>>        partition by range (a);
>> create table t_2 partition of t_1 for values from (1) to (10);
>>
>> create publication pub2 for table t_1(a), t_2
>>   with (PUBLISH_VIA_PARTITION_ROOT);
>>
>> ----sub
>> - prepare tables
>> CREATE SUBSCRIPTION sub CONNECTION 'port=10000 dbname=postgres' PUBLICATION pub2;
>>
>> When doing table sync for table 't_1', it has no column list. I think the
>> expected column list is (a).
>>
>> For Row Filter, it will use the row filter of the top most parent table(t_1) in
>> this case.
>>
>>
>> 3) one publication publishes both parent and child
>> ----pub
>> create table t (a int, b int, c int) partition by range (a);
>> create table t_1 partition of t for values from (1) to (10)
>>        partition by range (a);
>> create table t_2 partition of t_1 for values from (1) to (10);
>>
>> create publication pub2 for table t_1(a), t_2(b)
>>   with (PUBLISH_VIA_PARTITION_ROOT);
>>
>> ----sub
>> - prepare tables
>> CREATE SUBSCRIPTION sub CONNECTION 'port=10000 dbname=postgres' PUBLICATION pub2;
>>
>> When doing table sync for table 't_1', the column list would be (a, b). I think
>> the expected column list is (a).
>>
>> For Row Filter, it will use the row filter of the top most parent table(t_1) in
>> this case.
>>
> 
> Attached is an updated patch version, addressing all of those issues.
> 
> 0001 is a bugfix, reworking how we calculate publish_as_relid. The old
> approach was unstable with multiple publications, giving different
> results depending on order of the publications. This should be
> backpatched into PG13 where publish_via_partition_root was introduced, I
> think.
> 
> 0002 is the main patch, merging the changes proposed by Peter and fixing
> the issues reported here. In most cases this means adopting the code
> used for row filters, and perhaps simplifying it a bit.
> 
> 
> But I also tried to implement a row-filter test for 0001, and I'm not
> sure I understand the behavior I observe. Consider this:
> 
> -- a chain of 3 partitions (on both publisher and subscriber)
> CREATE TABLE test_part_rf (a int primary key, b int, c int)
>        PARTITION BY LIST (a);
> 
> CREATE TABLE test_part_rf_1
>        PARTITION OF test_part_rf FOR VALUES IN (1,2,3,4,5)
>        PARTITION BY LIST (a);
> 
> CREATE TABLE test_part_rf_2
>        PARTITION OF test_part_rf_1 FOR VALUES IN (1,2,3,4,5);
> 
> -- initial data
> INSERT INTO test_part_rf VALUES (1, 5, 100);
> INSERT INTO test_part_rf VALUES (2, 15, 200);
> 
> -- two publications, each adding a different partition
> CREATE PUBLICATION test_pub_part_1 FOR TABLE test_part_rf_1
>  WHERE (b < 10) WITH (publish_via_partition_root);
> 
> CREATE PUBLICATION test_pub_part_2 FOR TABLE test_part_rf_2
>  WHERE (b > 10) WITH (publish_via_partition_root);
> 
> -- now create the subscription (also try opposite ordering)
> CREATE SUBSCRIPTION test_part_sub CONNECTION '...'
>        PUBLICATION test_pub_part_1, test_pub_part_2;
> 
> -- wait for sync
> 
> -- inert some more data
> INSERT INTO test_part_rf VALUES (3, 6, 300);
> INSERT INTO test_part_rf VALUES (4, 16, 400);
> 
> -- wait for catchup
> 
> Now, based on the discussion here, my expectation is that we'll use the
> row filter from the top-most ancestor in any publication, which in this
> case is test_part_rf_1. Hence the filter should be (b < 10).
> 
> So I'd expect these rows to be replicated:
> 
> 1,5,100
> 3,6,300
> 
> But that's not what I get, unfortunately. I get different results,
> depending on the order of publications:
> 
> 1) test_pub_part_1, test_pub_part_2
> 
> 1|5|100
> 2|15|200
> 3|6|300
> 4|16|400
> 
> 2) test_pub_part_2, test_pub_part_1
> 
> 3|6|300
> 4|16|400
> 
> That seems pretty bizarre, because it either means we're not enforcing
> any filter or some strange combination of filters (notice that for (2)
> we skip/replicate rows matching either filter).
> 
> I have to be missing something important, but this seems confusing.
> There's a patch adding a simple test case to 028_row_filter.sql (named
> .txt, so as not to confuse cfbot).
> 

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.


regards

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