Message-ID: <c4029baf-693d-4bb5-7c57-5bfcdc5572ff@enterprisedb.com>
Date: Mon, 4 Dec 2023 13:00:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: undetected deadlock in ALTER SUBSCRIPTION ... REFRESH PUBLICATION
To: Amit Kapila <amit.kapila16@gmail.com>,
 Shlok Kyal <shlok.kyal.oss@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <d291bb50-12c4-e8af-2af2-7bb9bb4d8e3e@enterprisedb.com>
 <CAA4eK1LHD3ZHqyK2GcSw=fqCPr26yP2GL9niQbY8ems68D1Z9A@mail.gmail.com>
 <8948e4a2-fec7-8a61-9ace-2537cee3ef6f@enterprisedb.com>
 <CAA4eK1Kj6THmgXZ98d8rxAJa+V1KGS79KgqVve5i0Oyn+=tbMw@mail.gmail.com>
 <69e9a1af-a315-8b05-0674-2cc53bf27682@enterprisedb.com>
 <CAA4eK1L4wWiFnhDYhGMv-M4GTR6Xg13xoaSjhUm+YzFaP3nxUQ@mail.gmail.com>
 <9afab261-94a7-3e58-f0fb-040e45f42aa5@enterprisedb.com>
 <CANhcyEWtf6h5Ati69nbPoFXvFjfFsfrCekajWC=7JVnD0=cEfw@mail.gmail.com>
 <CANhcyEWxjwHXka+A+9PLJqpvEa5fZXD5Du1cW1QqzAxjdBhrmg@mail.gmail.com>
 <CAA4eK1KgyHAvDOPQgUdehSwUqDr0WxWtjxR5BHOMYkYaD2QZmA@mail.gmail.com>
 <CANhcyEXKW7LjPrBBc_ttxxeFrJBQQ=GbGCfZKcy0-6_8ejnJcg@mail.gmail.com>
 <CAA4eK1KKgA+j4z2XYkAZh5cEzAUEdKw0-u96bu5a9PBbZumS7Q@mail.gmail.com>
Content-Language: en-US
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: 
 <CAA4eK1KKgA+j4z2XYkAZh5cEzAUEdKw0-u96bu5a9PBbZumS7Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/c4029baf-693d-4bb5-7c57-5bfcdc5572ff%40enterprisedb.com>
Precedence: bulk


On 12/4/23 12:37, Amit Kapila wrote:
> On Sat, Dec 2, 2023 at 9:52 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>>
>>> thread. I think you can compare the timing of regression tests in
>>> subscription, with and without the patch to show there is no
>>> regression. And probably some tests with a large number of tables for
>>> sync with very little data.
>>
>> I have tested the regression test timings for subscription with and
>> without patch. I also did the timing test for sync of subscription
>> with the publisher for 100 and 1000 tables respectively.
>> I have attached the test script and results of the timing test are as follows:
>>
>> Time taken for test to run in Linux VM
>> Summary                                |  Subscription Test (sec)
>> |    100 tables in pub and Sub (sec)    |  1000 tables in pub and Sub
>> (sec)
>> Without patch Release           |  95.564
>>  |     7.877                                             |   58.919
>> With patch Release                |  96.513
>>    |     6.533                                             |   45.807
>>
>> Time Taken for test to run in another Linux VM
>> Summary                                |  Subscription Test (sec)
>> |    100 tables in pub and Sub (sec)    |  1000 tables in pub and Sub
>> (sec)
>> Without patch Release           |  109.8145
>> |    6.4675                                           |    83.001
>> With patch Release                |  113.162
>>    |    7.947                                              |   87.113
>>
> 
> So, on some machines, it may increase the test timing although not too
> much. I think the reason is probably doing the work in multiple
> transactions for multiple relations. I am wondering that instead of
> committing and starting a new transaction before
> wait_for_relation_state_change(), what if we do it inside that
> function just before we decide to wait? It is quite possible that in
> many cases we don't need any wait at all.
> 

I'm not sure what you mean by "do it". What should the function do?

As for the test results, I very much doubt the differences are not
caused simply by random timing variations, or something like that. And I
don't understand what "Performance Machine Linux" is, considering those
timings are slower than the other two machines.

Also, even if it was a bit slower, does it really matter? I mean, the
current code is wrong, can lead to infinite duration if it happens to
hit the deadlock. And it's a one-time action, I don't think it's a very
sensitive in terms of performance.

regards

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