Subject: Re: Column Filtering in Logical Replication
To: Rahila Syed <rahilasyed90@gmail.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
References: 
 <CAH2L28vddB_NFdRVpuyRBJEBWjz4BSyTB=_ektNRH8NJ1jf95g@mail.gmail.com>
 <202107062342.eq6htmp2wgp2@alvherre.pgsql>
 <CAH2L28sZj2Bj=73qZrXy69y6w5BRKKpunV3P7tnXYQKPbA11gw@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <fe1e52c0-ae22-119f-c382-9579033b83a6@enterprisedb.com>
Date: Mon, 12 Jul 2021 14:54:26 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: 
 <CAH2L28sZj2Bj=73qZrXy69y6w5BRKKpunV3P7tnXYQKPbA11gw@mail.gmail.com>
Content-Type: multipart/mixed;
 boundary="------------5C65C717A67C48BD588AE593"
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/fe1e52c0-ae22-119f-c382-9579033b83a6%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------5C65C717A67C48BD588AE593
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit


On 7/12/21 11:38 AM, Rahila Syed wrote:
> Hi Alvaro,
> 
> Thank you for comments.
> 
>     The patch adds a function get_att_num_by_name; but we have a lsyscache.c
>     function for that purpose, get_attnum.  Maybe that one should be used
>     instead?
> 
> Thank you for pointing that out, I agree it makes sense to reuse the
> existing function.
> Changed it accordingly in the attached patch.
>  
> 
>     get_tuple_columns_map() returns a bitmapset of the attnos of the columns
>     in the given list, so its name feels wrong.  I propose
>     get_table_columnset().  However, this function is invoked for every
>     insert/update change, so it's going to be far too slow to be usable.  I
>     think you need to cache the bitmapset somewhere, so that the function is
>     only called on first use.  I didn't look very closely, but it seems that
>     struct RelationSyncEntry may be a good place to cache it.
> 
> Makes sense, changed accordingly.
>  

To nitpick, I find "Bitmapset *att_list" a bit annoying, because it's
not really a list ;-)


FWIW "make check" fails for me with this version, due to segfault in
OpenTableLists. Apparenly there's some confusion - the code expects the
list to contain PublicationTable nodes, and tries to extract the
RangeVar from the elements. But the list actually contains RangeVar, so
this crashes and burns. See the attached backtrace.

I'd bet this is because the patch uses list of RangeVar in some cases
and list of PublicationTable in some cases, similarly to the "row
filtering" patch nearby. IMHO this is just confusing and we should
always pass list of PublicationTable nodes.


regards

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

--------------5C65C717A67C48BD588AE593
Content-Type: text/plain; charset=UTF-8;
 name="crash.txt"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="crash.txt"

Q29yZSB3YXMgZ2VuZXJhdGVkIGJ5IGBwb3N0Z3JlczogdXNlciByZWdyZXNzaW9uIFtsb2Nh
bF0gQUxURVIgUFVCTElDQVRJT04gICAgICAgICAgICAgICAgICAgICAgICAgICAnLgpQcm9n
cmFtIHRlcm1pbmF0ZWQgd2l0aCBzaWduYWwgU0lHU0VHViwgU2VnbWVudGF0aW9uIGZhdWx0
LgojMCAgMHgwMDAwMDAwMDAwNmJlNWI1IGluIE9wZW5UYWJsZUxpc3QgKHRhYmxlcz0weDI3
YjM1YTApIGF0IHB1YmxpY2F0aW9uY21kcy5jOjUyMAo1MjAJCQlib29sCQlyZWN1cnNlID0g
cnYtPmluaDsKTWlzc2luZyBzZXBhcmF0ZSBkZWJ1Z2luZm9zLCB1c2U6IGRuZiBkZWJ1Z2lu
Zm8taW5zdGFsbCBnbGliYy0yLjMyLTYuZmMzMy54ODZfNjQKKGdkYikgYnQKIzAgIDB4MDAw
MDAwMDAwMDZiZTViNSBpbiBPcGVuVGFibGVMaXN0ICh0YWJsZXM9MHgyN2IzNWEwKSBhdCBw
dWJsaWNhdGlvbmNtZHMuYzo1MjAKIzEgIDB4MDAwMDAwMDAwMDZiZTEzNyBpbiBBbHRlclB1
YmxpY2F0aW9uVGFibGVzIChzdG10PTB4MjdiMzVmOCwgcmVsPTB4NzNlMjY0ODVlYzgwLCB0
dXA9MHgyODc3Yzk4KSBhdCBwdWJsaWNhdGlvbmNtZHMuYzozNzUKIzIgIDB4MDAwMDAwMDAw
MDZiZTQ1OCBpbiBBbHRlclB1YmxpY2F0aW9uIChzdG10PTB4MjdiMzVmOCkgYXQgcHVibGlj
YXRpb25jbWRzLmM6NDY0CiMzICAweDAwMDAwMDAwMDA5NzYyYjQgaW4gUHJvY2Vzc1V0aWxp
dHlTbG93IChwc3RhdGU9MHgyODc3YjgwLCBwc3RtdD0weDI3YjM5NjgsIHF1ZXJ5U3RyaW5n
PTB4MjdiMmFlMCAiQUxURVIgUFVCTElDQVRJT04gdGVzdHB1Yl9kZWZhdWx0IFNFVCBUQUJM
RSB0ZXN0cHViX3RibDE7IiwgY29udGV4dD1QUk9DRVNTX1VUSUxJVFlfVE9QTEVWRUwsIHBh
cmFtcz0weDAsIHF1ZXJ5RW52PTB4MCwgZGVzdD0weDI3YjNhNTgsIHFjPTB4N2ZmZWMyMTFi
ZTcwKSBhdCB1dGlsaXR5LmM6MTgwOQojNCAgMHgwMDAwMDAwMDAwOTc0OTY4IGluIHN0YW5k
YXJkX1Byb2Nlc3NVdGlsaXR5IChwc3RtdD0weDI3YjM5NjgsIHF1ZXJ5U3RyaW5nPTB4Mjdi
MmFlMCAiQUxURVIgUFVCTElDQVRJT04gdGVzdHB1Yl9kZWZhdWx0IFNFVCBUQUJMRSB0ZXN0
cHViX3RibDE7IiwgcmVhZE9ubHlUcmVlPWZhbHNlLCBjb250ZXh0PVBST0NFU1NfVVRJTElU
WV9UT1BMRVZFTCwgcGFyYW1zPTB4MCwgcXVlcnlFbnY9MHgwLCBkZXN0PTB4MjdiM2E1OCwg
cWM9MHg3ZmZlYzIxMWJlNzApIGF0IHV0aWxpdHkuYzoxMDQ5CiM1ICAweDAwMDAwMDAwMDA5
NzNiYjggaW4gUHJvY2Vzc1V0aWxpdHkgKHBzdG10PTB4MjdiMzk2OCwgcXVlcnlTdHJpbmc9
MHgyN2IyYWUwICJBTFRFUiBQVUJMSUNBVElPTiB0ZXN0cHViX2RlZmF1bHQgU0VUIFRBQkxF
IHRlc3RwdWJfdGJsMTsiLCByZWFkT25seVRyZWU9ZmFsc2UsIGNvbnRleHQ9UFJPQ0VTU19V
VElMSVRZX1RPUExFVkVMLCBwYXJhbXM9MHgwLCBxdWVyeUVudj0weDAsIGRlc3Q9MHgyN2Iz
YTU4LCBxYz0weDdmZmVjMjExYmU3MCkgYXQgdXRpbGl0eS5jOjUyNwojNiAgMHgwMDAwMDAw
MDAwOTcyODkwIGluIFBvcnRhbFJ1blV0aWxpdHkgKHBvcnRhbD0weDI4MTZmZjAsIHBzdG10
PTB4MjdiMzk2OCwgaXNUb3BMZXZlbD10cnVlLCBzZXRIb2xkU25hcHNob3Q9ZmFsc2UsIGRl
c3Q9MHgyN2IzYTU4LCBxYz0weDdmZmVjMjExYmU3MCkgYXQgcHF1ZXJ5LmM6MTE0NwojNyAg
MHgwMDAwMDAwMDAwOTcyYWY0IGluIFBvcnRhbFJ1bk11bHRpIChwb3J0YWw9MHgyODE2ZmYw
LCBpc1RvcExldmVsPXRydWUsIHNldEhvbGRTbmFwc2hvdD1mYWxzZSwgZGVzdD0weDI3YjNh
NTgsIGFsdGRlc3Q9MHgyN2IzYTU4LCBxYz0weDdmZmVjMjExYmU3MCkgYXQgcHF1ZXJ5LmM6
MTMwNAojOCAgMHgwMDAwMDAwMDAwOTcyMDJkIGluIFBvcnRhbFJ1biAocG9ydGFsPTB4Mjgx
NmZmMCwgY291bnQ9OTIyMzM3MjAzNjg1NDc3NTgwNywgaXNUb3BMZXZlbD10cnVlLCBydW5f
b25jZT10cnVlLCBkZXN0PTB4MjdiM2E1OCwgYWx0ZGVzdD0weDI3YjNhNTgsIHFjPTB4N2Zm
ZWMyMTFiZTcwKSBhdCBwcXVlcnkuYzo3ODYKIzkgIDB4MDAwMDAwMDAwMDk2YmM0ZiBpbiBl
eGVjX3NpbXBsZV9xdWVyeSAocXVlcnlfc3RyaW5nPTB4MjdiMmFlMCAiQUxURVIgUFVCTElD
QVRJT04gdGVzdHB1Yl9kZWZhdWx0IFNFVCBUQUJMRSB0ZXN0cHViX3RibDE7IikgYXQgcG9z
dGdyZXMuYzoxMjE0CiMxMCAweDAwMDAwMDAwMDA5NzAxNWEgaW4gUG9zdGdyZXNNYWluIChh
cmdjPTEsIGFyZ3Y9MHg3ZmZlYzIxMWMxMDAsIGRibmFtZT0weDI3ZGVlZDggInJlZ3Jlc3Np
b24iLCB1c2VybmFtZT0weDI3ZGVlYjggInVzZXIiKSBhdCBwb3N0Z3Jlcy5jOjQ0ODYKIzEx
IDB4MDAwMDAwMDAwMDhhZDA4ZCBpbiBCYWNrZW5kUnVuIChwb3J0PTB4MjdkN2ZjMCkgYXQg
cG9zdG1hc3Rlci5jOjQ1MDcKIzEyIDB4MDAwMDAwMDAwMDhhY2EwYyBpbiBCYWNrZW5kU3Rh
cnR1cCAocG9ydD0weDI3ZDdmYzApIGF0IHBvc3RtYXN0ZXIuYzo0MjI5CiMxMyAweDAwMDAw
MDAwMDA4YThmYzAgaW4gU2VydmVyTG9vcCAoKSBhdCBwb3N0bWFzdGVyLmM6MTc0NQojMTQg
MHgwMDAwMDAwMDAwOGE4ODhiIGluIFBvc3RtYXN0ZXJNYWluIChhcmdjPTgsIGFyZ3Y9MHgy
N2FjNTkwKSBhdCBwb3N0bWFzdGVyLmM6MTQxNwojMTUgMHgwMDAwMDAwMDAwN2FiMmIwIGlu
IG1haW4gKGFyZ2M9OCwgYXJndj0weDI3YWM1OTApIGF0IG1haW4uYzoyMDkK
--------------5C65C717A67C48BD588AE593--