Message-ID: <f54fc646-6bc1-b218-9831-8159ce145234@enterprisedb.com>
Date: Mon, 18 Apr 2022 14:36:00 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:100.0) Gecko/20100101
 Thunderbird/100.0
Subject: Re: Column Filtering in Logical Replication
Content-Language: en-US
To: Amit Kapila <amit.kapila16@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
 Petr Jelinek <petr.jelinek@enterprisedb.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>
 <CAA4eK1KvFAygPbFu=cUvnqhmTLPpjzqp8p4UNSsLkiZmvFp8oQ@mail.gmail.com>
 <bc6918cc-2025-6c91-2ca4-c237af67e97d@enterprisedb.com>
 <f217341d-6841-390d-3dd2-89f9d1de17fe@enterprisedb.com>
 <f90d2b03-4462-ce95-a524-d91464e797c8@enterprisedb.com>
 <CAA4eK1LpBFU49Ohbnk=dv_v9YP+Kqh1+Sf8i++_s-QhD1Gy4Qw@mail.gmail.com>
 <3162f7f0-86ac-e3a9-dc55-0a660cb9bab1@enterprisedb.com>
 <369ae611-8822-f499-87cd-58ad0d60c60c@enterprisedb.com>
 <CAA4eK1JzzoE61CY1qi9Vcdi742JFwG4YA3XpoMHwfKNhbFic6g@mail.gmail.com>
 <CAA4eK1JcQRQw0G-U4A+vaGaBWSvggYMMDJH4eDtJ0Yf2eUYXyA@mail.gmail.com>
 <CAD21AoAD6qhu1gK=vnzxvNiQpFx1m_wqXOvWqy1g4iNLJWCbnw@mail.gmail.com>
 <CAA4eK1J03HJC-6bUsdHnO8O6W89T-CJ1NaqA5HuxTYikwA-HOQ@mail.gmail.com>
 <CAD21AoAS67ppEQ_RPggNBHbfrah2NiO59NPUpomPYRSF_voMJA@mail.gmail.com>
 <CAA4eK1LHevgWSsHHD2VHCMNnqqopdyY9fSZFjKPoTwA3vPy81g@mail.gmail.com>
 <CAA4eK1+fbCPjoFUO-FYDkosfzs0he4=AJtJNcQkOxV53=39jJg@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <CAA4eK1+fbCPjoFUO-FYDkosfzs0he4=AJtJNcQkOxV53=39jJg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/f54fc646-6bc1-b218-9831-8159ce145234%40enterprisedb.com>
Precedence: bulk

On 4/18/22 13:04, Amit Kapila wrote:
> On Thu, Apr 14, 2022 at 9:09 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Thu, Apr 14, 2022 at 8:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>>
>>>> The other part of the puzzle is the below check in the code:
>>>> /*
>>>> * If we reached the sync worker limit per subscription, just exit
>>>> * silently as we might get here because of an otherwise harmless race
>>>> * condition.
>>>> */
>>>> if (nsyncworkers >= max_sync_workers_per_subscription)
>>>>
>>>> It is not clear to me why this check is there, if this wouldn't be
>>>> there, the user would have got either a WARNING to increase the
>>>> max_logical_replication_workers or the apply worker would have been
>>>> restarted. Do you have any idea about this?
>>>
>>> Yeah, I'm also puzzled with this check. It seems that this function
>>> doesn't work well when the apply worker is not running and some
>>> tablesync workers are running. I initially thought that the apply
>>> worker calls to this function as many as tables that needs to be
>>> synced, but it checks the max_sync_workers_per_subscription limit
>>> before calling to logicalrep_worker_launch(). So I'm not really sure
>>> we need this check.
>>>
>>
>> I just hope that the original author Petr J. responds to this point. I
>> have added him to this email. This will help us to find the best
>> solution for this problem.
>>
> 
> I did some more investigation for this code. It is added by commit [1]
> and the patch that led to this commit is first time posted on -hackers
> in email [2]. Now, neither the commit message nor the patch (comments)
> gives much idea as to why this part of code is added but I think there
> is some hint in the email [2]. In particular, read the paragraph in
> the email [2] that has the lines: ".... and limiting sync workers per
> subscription theoretically wasn't either (although I don't think it
> could happen in practice).".
> 
> It seems that this check has been added to theoretically limit the
> sync workers even though that can't happen because apply worker
> ensures that before trying to launch the sync worker. Does this theory
> make sense to me? If so, I think we can change the check as: "if
> (OidIsValid(relid) && nsyncworkers >=
> max_sync_workers_per_subscription)" in launcher.c. This will serve the
> purpose of the original code and will solve the issue being discussed
> here. I think we can even backpatch this. What do you think?
> 

Sounds reasonable to me. It's unfortunate there's no explanation of what
exactly is the commit message fixing (and why), but I doubt anyone will
remember the details after 5 years.

+1 to backpatching, I consider this to be a bug


regards

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