Subject: Re: row filtering for logical replication
To: Euler Taveira <euler@eulerto.com>, Greg Nancarrow <gregn4422@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.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>
References: 
 <CA+HiwqH+9NK5m-=BOYpzjH5UEpeWjT_3HEm4XBYNrda88naGMQ@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>
 <CAJcOf-dApxx3mqeHZ_20O-6hGOXmpMuqPpNb+3ZF3ENfb0oHoA@mail.gmail.com>
 <34b61db2-3e26-42ed-bb84-b14a9fe0a60d@www.fastmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <849ee491-bba3-c0ae-cc25-4fce1c03f105@enterprisedb.com>
Date: Mon, 12 Jul 2021 01:09:21 +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: <34b61db2-3e26-42ed-bb84-b14a9fe0a60d@www.fastmail.com>
Content-Type: multipart/mixed;
 boundary="------------EA50EA0D7CFD2CC326916F04"
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/849ee491-bba3-c0ae-cc25-4fce1c03f105%40enterprisedb.com>
Precedence: bulk

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

Hi,

I took a look at this patch, which seems to be in CF since 2018. I have 
only some basic comments and observations at this point:

1) alter_publication.sgml

I think "expression is executed" sounds a bit strange, perhaps 
"evaluated" would be better?

2) create_publication.sgml

Why is the patch changing publish_via_partition_root docs? That seems 
like a rather unrelated bit.

    The <literal>WHERE</literal> clause should probably contain only
    columns that are part of the primary key or be covered by
    <literal>REPLICA ...

I'm not sure what exactly is this trying to say. What does "should 
probably ..." mean in practice for the users? Does that mean something 
bad will happen for other columns, or what? I'm sure this wording will 
be quite confusing for users.

It may also be unclear whether the condition is evaluated on the old or 
new row, so perhaps add an example illustrating that & more detailed 
comment, or something. E.g. what will happen with

	UPDATE departments SET active = false WHERE active;


3) publication_add_relation

Does this need to build the parse state even for whereClause == NULL?


4) AlterPublicationTables

I wonder if this new reworked code might have issues with subscriptions 
containing many tables, but I haven't tried.


5) OpenTableList

I really dislike that the list can have two different node types 
(Relation and PublicationTable). In principle we don't actually need the 
extra flag, we can simply check the node type directly by IsA() and act 
based on that. However, I think it'd be better to just use a single node 
type from all places.

I don't see why not to set whereClause every time, I don't think the 
extra if saves anything, it's just a bit more complex.


5) CloseTableList

The comment about node types seems pointless, this function has no flag 
and the element type does not matter.


6) parse_agg.c

    ... are not allowed in publication WHERE expressions

I think all similar cases use "WHERE conditions" instead.


7) transformExprRecurse

The check at the beginning seems rather awkward / misplaced - it's way 
too specific for this location (there are no other p_expr_kind 
references in this function). Wouldn't transformFuncCall (or maybe 
ParseFuncOrColumn) be a more appropriate place?

Initially I was wondering why not to allow function calls in WHERE 
conditions, but I see that was discussed in the past as problematic. But 
that reminds me that I don't see any docs describing what expressions 
are allowed in WHERE conditions - maybe we should explicitly list what 
expressions are allowed?


8) pgoutput.c

I have not reviewed this in detail yet, but there seems to be something 
wrong because `make check-world` fails in subscription/010_truncate.pl 
after hitting an assert  (backtrace attached) during "START_REPLICATION 
SLOT" in get_rel_sync_entry in this code:

     /* Release tuple table slot */
     if (entry->scantuple != NULL)
     {
         ExecDropSingleTupleTableSlot(entry->scantuple);
         entry->scantuple = NULL;
     }

So there seems to be something wrong with how the slot is created.


regards

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

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

UHJvZ3JhbSB0ZXJtaW5hdGVkIHdpdGggc2lnbmFsIFNJR0FCUlQsIEFib3J0ZWQuCiMwICAw
eDAwMDA3ZThlZDJlYzI5ZTUgaW4gPz8gKCkgZnJvbSAvbGliNjQvbGliYy5zby42Ck1pc3Np
bmcgc2VwYXJhdGUgZGVidWdpbmZvcywgdXNlOiBkbmYgZGVidWdpbmZvLWluc3RhbGwgZ2xp
YmMtMi4zMi04LmZjMzMueDg2XzY0IGxpYmdjYy0xMC4zLjEtMS5mYzMzLng4Nl82NAooZ2Ri
KSBidAojMCAgMHgwMDAwN2U4ZWQyZWMyOWU1IGluID8/ICgpIGZyb20gL2xpYjY0L2xpYmMu
c28uNgojMSAgMHgwMDAwN2U4ZWQyZWFiOGE0IGluID8/ICgpIGZyb20gL2xpYjY0L2xpYmMu
c28uNgojMiAgMHgwMDAwMDAwMDAwYWZmZmUxIGluIEV4Y2VwdGlvbmFsQ29uZGl0aW9uIChj
b25kaXRpb25OYW1lPTB4YjdmOGU1ICJ0dXBkZXNjLT50ZHJlZmNvdW50ID4gMCIsIGVycm9y
VHlwZT0weGI3ZjhhMyAiRmFpbGVkQXNzZXJ0aW9uIiwgZmlsZU5hbWU9MHhiN2Y4MTAgInR1
cGRlc2MuYyIsIGxpbmVOdW1iZXI9Mzg2KSBhdCBhc3NlcnQuYzo2OQojMyAgMHgwMDAwMDAw
MDAwNGE1NWNiIGluIERlY3JUdXBsZURlc2NSZWZDb3VudCAodHVwZGVzYz0weDdlOGVkMjNi
NDU3OCkgYXQgdHVwZGVzYy5jOjM4NgojNCAgMHgwMDAwMDAwMDAwNzNmZDg0IGluIEV4ZWNE
cm9wU2luZ2xlVHVwbGVUYWJsZVNsb3QgKHNsb3Q9MHg3ZThlZDIzYjg5MTApIGF0IGV4ZWNU
dXBsZXMuYzoxMjYxCiM1ICAweDAwMDA3ZThlZDIzZjJhZWQgaW4gZ2V0X3JlbF9zeW5jX2Vu
dHJ5IChkYXRhPTB4Mjg3NTQ0MCwgcmVsYXRpb249MHg3ZThlZDIzYjg2YzApIGF0IHBnb3V0
cHV0LmM6MTI1OAojNiAgMHgwMDAwN2U4ZWQyM2YyMGY4IGluIHBnb3V0cHV0X3RydW5jYXRl
IChjdHg9MHgyODZjYzcwLCB0eG49MHgyODg4YzQ4LCBucmVsYXRpb25zPTEsIHJlbGF0aW9u
cz0weDI4OTJkZDgsIGNoYW5nZT0weDI4OGIxMzgpIGF0IHBnb3V0cHV0LmM6ODkwCiM3ICAw
eDAwMDAwMDAwMDA4Y2YwM2IgaW4gdHJ1bmNhdGVfY2Jfd3JhcHBlciAoY2FjaGU9MHgyODZl
YzgwLCB0eG49MHgyODg4YzQ4LCBucmVsYXRpb25zPTEsIHJlbGF0aW9ucz0weDI4OTJkZDgs
IGNoYW5nZT0weDI4OGIxMzgpIGF0IGxvZ2ljYWwuYzoxMDgzCiM4ICAweDAwMDAwMDAwMDA4
ZDk2MDUgaW4gUmVvcmRlckJ1ZmZlckFwcGx5VHJ1bmNhdGUgKHJiPTB4Mjg2ZWM4MCwgdHhu
PTB4Mjg4OGM0OCwgbnJlbGF0aW9ucz0xLCByZWxhdGlvbnM9MHgyODkyZGQ4LCBjaGFuZ2U9
MHgyODhiMTM4LCBzdHJlYW1pbmc9ZmFsc2UpIGF0IHJlb3JkZXJidWZmZXIuYzoxOTI4CiM5
ICAweDAwMDAwMDAwMDA4ZGEwOGMgaW4gUmVvcmRlckJ1ZmZlclByb2Nlc3NUWE4gKHJiPTB4
Mjg2ZWM4MCwgdHhuPTB4Mjg4OGM0OCwgY29tbWl0X2xzbj0yMzkwMzc1Miwgc25hcHNob3Rf
bm93PTB4Mjg2ZjE2OCwgY29tbWFuZF9pZD0yLCBzdHJlYW1pbmc9ZmFsc2UpIGF0IHJlb3Jk
ZXJidWZmZXIuYzoyMzE0CiMxMCAweDAwMDAwMDAwMDA4ZGE4NDUgaW4gUmVvcmRlckJ1ZmZl
clJlcGxheSAodHhuPTB4Mjg4OGM0OCwgcmI9MHgyODZlYzgwLCB4aWQ9NzQ0LCBjb21taXRf
bHNuPTIzOTAzNzUyLCBlbmRfbHNuPTIzOTAzOTYwLCBjb21taXRfdGltZT02NzkzNTgzNDY4
ODMxMjEsIG9yaWdpbl9pZD0wLCBvcmlnaW5fbHNuPTApIGF0IHJlb3JkZXJidWZmZXIuYzoy
NjE5CiMxMSAweDAwMDAwMDAwMDA4ZGE4YzMgaW4gUmVvcmRlckJ1ZmZlckNvbW1pdCAocmI9
MHgyODZlYzgwLCB4aWQ9NzQ0LCBjb21taXRfbHNuPTIzOTAzNzUyLCBlbmRfbHNuPTIzOTAz
OTYwLCBjb21taXRfdGltZT02NzkzNTgzNDY4ODMxMjEsIG9yaWdpbl9pZD0wLCBvcmlnaW5f
bHNuPTApIGF0IHJlb3JkZXJidWZmZXIuYzoyNjQzCiMxMiAweDAwMDAwMDAwMDA4Y2E3MmEg
aW4gRGVjb2RlQ29tbWl0IChjdHg9MHgyODZjYzcwLCBidWY9MHg3ZmZmZDQ1Y2RlMjAsIHBh
cnNlZD0weDdmZmZkNDVjZGM4MCwgeGlkPTc0NCwgdHdvX3BoYXNlPWZhbHNlKSBhdCBkZWNv
ZGUuYzo3NDQKIzEzIDB4MDAwMDAwMDAwMDhjOWE5NyBpbiBEZWNvZGVYYWN0T3AgKGN0eD0w
eDI4NmNjNzAsIGJ1Zj0weDdmZmZkNDVjZGUyMCkgYXQgZGVjb2RlLmM6Mjc4CiMxNCAweDAw
MDAwMDAwMDA4Yzk2YzMgaW4gTG9naWNhbERlY29kaW5nUHJvY2Vzc1JlY29yZCAoY3R4PTB4
Mjg2Y2M3MCwgcmVjb3JkPTB4Mjg2ZDA3MCkgYXQgZGVjb2RlLmM6MTQyCiMxNSAweDAwMDAw
MDAwMDA5MDI0ZjcgaW4gWExvZ1NlbmRMb2dpY2FsICgpIGF0IHdhbHNlbmRlci5jOjI4ODQK
IzE2IDB4MDAwMDAwMDAwMDkwMTg3MiBpbiBXYWxTbmRMb29wIChzZW5kX2RhdGE9MHg5MDI0
NWIgPFhMb2dTZW5kTG9naWNhbD4pIGF0IHdhbHNlbmRlci5jOjIzMTQKIzE3IDB4MDAwMDAw
MDAwMDkwMDJiMCBpbiBTdGFydExvZ2ljYWxSZXBsaWNhdGlvbiAoY21kPTB4MjgzODhjOCkg
YXQgd2Fsc2VuZGVyLmM6MTIxNAojMTggMHgwMDAwMDAwMDAwOTAwYmViIGluIGV4ZWNfcmVw
bGljYXRpb25fY29tbWFuZCAoY21kX3N0cmluZz0weDI3OWVkYjAgIlNUQVJUX1JFUExJQ0FU
SU9OIFNMT1QgXCJzdWIxXCIgTE9HSUNBTCAwLzAgKHByb3RvX3ZlcnNpb24gJzInLCBwdWJs
aWNhdGlvbl9uYW1lcyAnXCJwdWIxXCInKSIpIGF0IHdhbHNlbmRlci5jOjE2NTQKIzE5IDB4
MDAwMDAwMDAwMDk3MDZiMiBpbiBQb3N0Z3Jlc01haW4gKGFyZ2M9MSwgYXJndj0weDdmZmZk
NDVjZTE0MCwgZGJuYW1lPTB4MjdjYjgxOCAicG9zdGdyZXMiLCB1c2VybmFtZT0weDI3Y2I3
ZjggInVzZXIiKSBhdCBwb3N0Z3Jlcy5jOjQ0ODIKIzIwIDB4MDAwMDAwMDAwMDhhZDIxOSBp
biBCYWNrZW5kUnVuIChwb3J0PTB4MjdjNDhkMCkgYXQgcG9zdG1hc3Rlci5jOjQ1MDcKIzIx
IDB4MDAwMDAwMDAwMDhhY2I5OCBpbiBCYWNrZW5kU3RhcnR1cCAocG9ydD0weDI3YzQ4ZDAp
IGF0IHBvc3RtYXN0ZXIuYzo0MjI5CiMyMiAweDAwMDAwMDAwMDA4YTkxNGMgaW4gU2VydmVy
TG9vcCAoKSBhdCBwb3N0bWFzdGVyLmM6MTc0NQojMjMgMHgwMDAwMDAwMDAwOGE4YTE3IGlu
IFBvc3RtYXN0ZXJNYWluIChhcmdjPTQsIGFyZ3Y9MHgyNzk4ODAwKSBhdCBwb3N0bWFzdGVy
LmM6MTQxNwojMjQgMHgwMDAwMDAwMDAwN2FiNDNjIGluIG1haW4gKGFyZ2M9NCwgYXJndj0w
eDI3OTg4MDApIGF0IG1haW4uYzoyMDkK
--------------EA50EA0D7CFD2CC326916F04--