Subject: Re: row filtering for logical replication
To: Stephen Frost <sfrost@snowman.net>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 =?UTF-8?Q?Fabr=c3=adzio_de_Royes_Mello?= <fabriziomello@gmail.com>,
 Euler Taveira de Oliveira <euler@timbira.com.br>,
 Pgsql Hackers <pgsql-hackers@postgresql.org>, hironobu@interdb.jp
References: <7106a0fc-8017-c0fe-a407-9466c9407ff8@2ndquadrant.com>
 <CAHE3wggWAtbT+Yt_MDy0uQwZcc6x820OwHp3_TBtAqtBeW4+Ug@mail.gmail.com>
 <92e5587d-28b8-5849-2374-5ca3863256f1@2ndquadrant.com>
 <CAFcNs+q88Z--uHbcii80r-pBWByEVNbc1nPyN8iHHKhCHCtzrA@mail.gmail.com>
 <466cdec7-92a0-41c0-b74a-ec0be94d4451@2ndquadrant.com>
 <CAFcNs+pA9ywYJbf940UFXW5B5i+An3=0yzxC8_bXubu+CsOQ=A@mail.gmail.com>
 <20181123190330.GI3415@tamriel.snowman.net>
 <b2f86a5b-06fa-d1a9-1295-db673ebed550@2ndquadrant.com>
 <20181214155645.GV3415@tamriel.snowman.net>
 <fdfd8677-120a-3dbe-2078-4187b3c62939@2ndquadrant.com>
 <20181227191925.GJ2528@tamriel.snowman.net>
From: Petr Jelinek <petr.jelinek@2ndquadrant.com>
Openpgp: preference=signencrypt
Autocrypt: addr=petr.jelinek@2ndquadrant.com; prefer-encrypt=mutual; keydata=
 mQINBFawwSABEAC45Vua8rSggpM2kkMMmoOHz51a+fi3OXFfnrStVh80pM2/J9nSzjrIKQKf
 H5PtRnoYzSKnoUDoLOkLwDYitK3HzUUWXma+AWAqef2Y0GSZ7A3jJkD+JHOfkpS1CIpkEB/e
 WjA8Wqegwzc/OEE5111/l0Q1e6jxgZI86h8MMnm35Of1Y6Lz4kqWCyxHhaTJKTCkq66ICQ2C
 VJkbvuUoEE+Hr/I38dPHTAqIlCnycVnJPLHBQ/UYGHR/USF8qI/qJJx6tJcNAEqz69QZ/VlY
 31UyJHdYe5NtBVz+rYZCOVWfpe3crbHuq9FjAPT+xcZefdxridmMKuW8eBmmXI9iZ3EG+X8J
 IsdLWL8QoqW0lnO1otYz1QDjdOvbEpq+u5/TAU8pgtaYH2J+6/GaQ6hMLyLBkaIAtnqlLCzq
 pxXcxMsFT14P5/rZA7EbBI8RS1ZIWJBlS2oO+dtVuENLJUC0SpptaRyn/uyOIopt7hSmBekL
 SzFnet7P9qofFJY+fQEz84v0G1gvwonHGHiCGgLpbMdPtnfPzda1aiTURvvOD7w/RmX1/ogk
 TqSGO4ejVNO/28QYouxmxzy8jbbIEiLCbHpLR/M/TvYHHPURxcx+MxVl3G+d/4Ihy4abxOdW
 pe9zLIcVMvqiUFQO28/79Gfhb7G4K+L21ORB82QYkzm3M7ebXQARAQABtCNQZXRyIEplbMOt
 bmVrIDxwam1vZG9zQHBqbW9kb3MubmV0PokCQAQTAQoAKgIbAwUJBaOagAULCQgHAwUVCgkI
 CwUWAgMBAAIeAQIXgAUCVrDBqwIZAQAKCRBci2l0giH4y/QJEACfiLHyvxlwMvo3Y88Qa9QI
 i7Hu3ZW+xxojojdRd7do0nBnf2a4YstD9u6ZOLJx4f5Ijb/8LxY10NgLugmfQ7sw2W8ui6wT
 NiAeNHpr4wpT3IV+V53psOMRYPdq2zNTLO4iNxqPFPgNufR/m4FqcGe0Wo7U0wd1p0MErXTg
 EXJDVmjPDkYnzE9ejSCOxF57+o3xs6+eW9Al2m3nqP8JCzpauCr0gqpQzCaw0VZ05aLL0t9E
 B0fdm90+ydVe10Wg+I348005smuSSckBCHZqUfjjWvjwE6v/6dZ3/50ycAbi6cTe7Fo4kOSO
 sXBFXkzYGWYrEFPkzK0zDH65D98gL1+l9uokCR2t/OYVwudbUCV3cS3xRd0eFgHbXuzV+aZc
 jRjqkPfpCA2loKtrX2U739mpVSHnpIfSBf48fLg7O5dVn5ZCrydGkEzgvyynPo7IikQrIA02
 91IoSp4RisKsJpaXHwHu6DH2rDCbYFumAMgXk2osLFX6VMPoSWKVRkr87A0VLafCtwaqr6Hy
 sFE6Lmu7jP4/BDr+9A2MXK+v0yYrXqqsZN6V9856ywlZrWK3MB6jc2bKpKS2YshneKxpitNV
 0ITs2Li0OlAXOEHE0eXwI93POgLoyH+8JiNbem3orw3Mnc+7XpIY86AejwUiYl8T7EO/2tIx
 HDaXLK3BPNOdH7kCDQRWsMEgARAA6Z3NrDLdcFF/P3TD/F7WQtMALEEKn5TXSF/K2VHt21GD
 8tGY1DZhbxjLuQJeheY/2L4F4Y5WCkAoUdBXQs7nIauN+63EL88/qPdH6f3+/HZ+cmPG1E5o
 dBABCkC42eGcI5O8/9TAaFQcSzg86pdAE8v927tnc6aH4W4/Mj7RJNlQiZaz2ngCC9mr5uu+
 CysOTfKCS0llNUkExZOJzZsQU3zDkcg3+xmgtHkCjglJ006KuytHj55HR6TzaKaAW8coHGoS
 OI/BQ9KAzvlj+32/zS8FSISZSTY3spMpraADsYklejCeJOmqQ3nTrCtwlX3hTSggfr4eelKA
 hdZKs1rIimNBtX90jJc0u2A/GmC959aTSrCUcfaPRweKUuexg7jxuhXyg9br2GscD2G6W7EV
 c+3s4dOHgoJFVMBBIqTtZviaNJ63IsZIFTYJzGKKMHOioZ3naoBVav0w63waDzj82nPYMIPj
 TxnClKiecuBQQPzMaG0jTmK0lis6rO0nIXrCwca8e/3Xzvi7sykOdcz9t4ef8HrDCCkG0yDO
 o8qHy1e7EtmJD7kWffIXZMt4TZVxpiOqgWtv+heJAfmqzr8pkNLUubmgvNdnakXJOJT41Lui
 50C5OhpU9mfFCIDOeZXTQLsHA4KYaL8W/qcjv/YDmvpJtnK24y6tByWuTu7r/SMAEQEAAYkC
 JQQYAQoADwUCVrDBIAIbDAUJBaOagAAKCRBci2l0giH4yyWpD/91jd4BMN2CDYqLtU7kaoAa
 aXnRVpf0z+R8rJ4LZJX1UNbHjsdClMQLfdBuZeDsdYar4iMfwScz6fiCiRwaRpr3XT6znaM0
 wba6fjzEqVSHr3WB1jrb/TMiNaNgQJaRPhI5C7VsLuPrv5/L8gFmyjl712q7F3busGFBKrjK
 qTESMu+sHNvVeC3QyBFCe2fudbAJdaaRsUoTbAZYiowVoQHOow8eBoWY+RzQoYGuw1fls5Ck
 85N73pYHXzjfk1s49sDKTYI263h9wbGGA234biHRI3NQN6sSYWRr/u5GOwTm/tzyadRj0pnZ
 S6KyLSEL4qx8jiyniD/XLoftlvR4YWQ0BXveEElx2n3TThLL1cVtOI4qE9BZKm32euBqwLta
 k9Jc659hmZB9E9aWLo8KcLjngwqSZupoHRPZ3yAlgZkpDoTIZpMCGxGEuuxgnosRbNpOJ1fE
 nNXenYYfi87tHRUCRKeHhoKkfGZEgC9zv9Y97j75npqV2tQyh4fNQT1tGdBigtvzI4nHM1/1
 PLJtJBdRHucP76vUH9RlP9EE/Sg5DFQqsfSYPqxLW9r+ZfSEeT0APdz7TQC9MzNzUhyXvA5a
 RQQCRMXKbGaYhKkYEYiYOGqtTxN+PmyvNUBmFEcagBX2TXqEkgnI6zOnN13mB4SihJ2lne2r
 9RNiPYs/hmRxYg==
Message-ID: <abd77986-fb20-9fe1-4447-3e11c5039fe9@2ndquadrant.com>
Date: Fri, 28 Dec 2018 00:36:25 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <20181227191925.GJ2528@tamriel.snowman.net>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 27/12/2018 20:19, Stephen Frost wrote:
> Greetings,
> 
> * Petr Jelinek (petr.jelinek@2ndquadrant.com) wrote:
>> On 14/12/2018 16:56, Stephen Frost wrote:
>>> * Tomas Vondra (tomas.vondra@2ndquadrant.com) wrote:
>>>> On 11/23/18 8:03 PM, Stephen Frost wrote:
>>>>> * Fabrízio de Royes Mello (fabriziomello@gmail.com) wrote:
>>>>>> On Fri, Nov 23, 2018 at 4:13 PM Petr Jelinek <petr.jelinek@2ndquadrant.com>
>>>>>> wrote:
>>>>>>>> If carefully documented I see no problem with it... we already have an
>>>>>>>> analogous problem with functional indexes.
>>>>>>>
>>>>>>> The difference is that with functional indexes you can recreate the
>>>>>>> missing object and everything is okay again. With logical replication
>>>>>>> recreating the object will not help.
>>>>>>>
>>>>>>
>>>>>> In this case with logical replication you should rsync the object. That is
>>>>>> the price of misunderstanding / bad use of the new feature.
>>>>>>
>>>>>> As usual, there are no free beer ;-)
>>>>>
>>>>> There's also certainly no shortage of other ways to break logical
>>>>> replication, including ways that would also be hard to recover from
>>>>> today other than doing a full resync.
>>>>
>>>> Sure, but that seems more like an argument against creating additional
>>>> ones (and for preventing those that already exist). I'm not sure this
>>>> particular feature is where we should draw the line, though.
>>>
>>> I was actually going in the other direction- we should allow it because
>>> advanced users may know what they're doing better than we do and we
>>> shouldn't prevent things just because they might be misused or
>>> misunderstood by a user.
>>
>> That's all good, but we need good escape hatch for when things go south
>> and we don't have it and IMHO it's not as easy to have one as you might
>> think.
> 
> We don't have a great solution but we should be able to at least drop
> and recreate the publication or subscription, even today, can't we?

Well we can drop thing always, yes, not having ability to drop things
when they break would be bad design. I am debating ability to recover
without rebuilding everything a there are cases where you simply can't
rebuild everything (ie we allow filtering out deletes). I don't like
disabling UDFs either as that means that user created types are unusable
in filters, I just wonder if saying "sorry your replica is gone" is any
better.

> Sure, that means having to recopy everything, but that's what you get if
> you break your publication/subscription.

This is but off-topic here, but I really wonder how are you currently
breaking your publications/subscriptions.

>>>>> What that seems to indicate, to me at least, is that it'd be awful
>>>>> nice to have a way to resync the data which doesn't necessairly
>>>>> involve transferring all of it over again.
>>>>>
>>>>> Of course, it'd be nice if we could track those dependencies too,
>>>>> but that's yet another thing.
>>>>
>>>> Yep, that seems like a good idea in general. Both here and for
>>>> functional indexes (although I suppose sure is a technical reason why it
>>>> wasn't implemented right away for them).
>>>
>>> We don't track function dependencies in general and I could certainly
>>> see cases where you really wouldn't want to do so, at least not in the
>>> same way that we track FKs or similar.  I do wonder if maybe we didn't
>>> track function dependencies because we didn't (yet) have create or
>>> replace function and that now we should.  We don't track dependencies
>>> inside a function either though.
>>
>> Yeah we can't always have dependencies, it would break some perfectly
>> valid usage scenarios. Also it's not exactly clear to me how we'd track
>> dependencies of say plpython function...
> 
> Well, we could at leasts depend on the functions explicitly listed at
> the top level and I don't believe we even do that today.  I can't think
> of any downside off-hand to that, given that we have create-or-replace
> function.
> 

I dunno how much is that worth it TBH, the situations where I've seen
this issue (pglogical has this feature for long time and suffers from
the same lack of dependency tracking) is that somebody drops table/type
used in a function that is used as filter.

>>>>> In short, I'm not sure that I agree with the idea that we shouldn't
>>>>> allow this and instead I'd rather we realize it and put the logical
>>>>> replication into some kind of an error state that requires a resync.
>>>>
>>>> That would still mean a need to resync the data to recover, so I'm not
>>>> sure it's really an improvement. And I suppose it'd require tracking the
>>>> dependencies, because how else would you mark the subscription as
>>>> requiring a resync? At which point we could decline the DROP without a
>>>> CASCADE, just like we do elsewhere, no?
>>>
>>> I was actually thinking more along the lines of just simply marking the
>>> publication/subscription as being in a 'failed' state when a failure
>>> actually happens, and maybe even at that point basically throwing away
>>> everything except the shell of the publication/subscription (so the user
>>> can see that it failed and come in and properly drop it); I'm thinking
>>> about this as perhaps similar to a transaction being aborted.
>>
>> There are several problems with that. First this happens in historic
>> snapshot which can't write and on top of that we are in the middle of
>> error processing so we have our hands tied a bit, it's definitely going
>> to need bit of creative thinking to do this.
> 
> We can't write to things inside the database in a historic snapshot and
> we do have to deal with the fact that we're in error processing.  What
> about writing somewhere that's outside of the regular database system?
> Maybe a pg_logical/failed directory?  There's all the usual
> complications from that around dealing with durable writes (if we need
> to worry about that and I'm not sure that we do...  if we fail to
> persist a write saying "X failed" and we restart.. well, it's gonna fail
> again and we write it then), and cleaning things up as needed (but maybe
> this is handled as part of the DROP, and we WAL that, so we can re-do
> the removal of the failed marker file...), and if we need to think about
> what should happen on replicas (is there anything?).

That sounds pretty reasonable. Given that this is corner-case user error
we could perhaps do extra work to ensure things are fsynced even if it's
all not too fast...

> 
>> Second, and that's more soft issue (which is probably harder to solve)
>> what do we do with the slot and subscription. There is one failed
>> publication, but the subscription may be subscribed to 20 of them, do we
>> kill the whole subscription because of single failed publication? If we
>> don't do we continue replicating like nothing has happened but with data
>> in the failed publication missing (which can be considered data
>> loss/corruption from the view of user). If we stop replication, do we
>> clean the slot so that we don't keep back wal/catalog xmin forever
>> (which could lead to server stopping) or do we keep the slot so that
>> user can somehow fix the issue (reconfigure subscription to not care
>> about that publication for example) and continue replication without
>> further loss?
> 
> I would think we'd have to fail the whole publication if there's a
> failure for any part of it.  Replicating a partial set definitely sounds
> wrong to me.  Once we stop replication, yes, we should clean the slot
> and mark it failed so that we don't keep back WAL and so that we allow
> the catalog xmin to move forward so that the failed publication doesn't
> run the server out of disk space.
> 

I agree that continuing replication where some part of publication is
broken seems wrong and that we should stop replication at that point.

> If we really think there's a use-case for keeping the replication slot

It's not so much about use-case as it is about complete change of
behavior - there is no current error where we remove existing slot.
The use case for keeping slot is a) investigation of the issue, b) just
skipping the broken part of stream by advancing origin on subscription
and continuing replication, with some luck that can mean only single
table needs resyncing, which is better than rebuilding everything.

I think some kind of automated slot cleanup is desirable, but likely
separate feature that should be designed based on amount of outstanding
wal or something.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services