Re: [BUG] Logical replica crash if there was an error in a function.
Anton A. Melnikov <aamelnikov@inbox.ru>
From: "Anton A. Melnikov" <aamelnikov@inbox.ru>
To: Tom Lane <tgl@sss.pgh.pa.us>, Amit Kapila <amit.kapila16@gmail.com>,
Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,
PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
Maxim Orlov <orlovmg@gmail.com>
Date: 2022-11-15T01:39:53Z
Lists: pgsql-hackers
Attachments
- v1-0001-Test-for-func-error-in-logrep-worker.patch (text/x-patch) patch v1-0001
Hello! On 02.11.2022 21:02, Tom Lane wrote: > So I'm now good with the idea of just not failing. I don't like > the patch as presented though. First, the cfbot is quite rightly > complaining about the "uninitialized variable" warning it draws. > Second, I don't see a good reason to tie the change to logical > replication in any way. Let's just change the Assert to an if(), > as attached. Fully agreed that is the most optimal solution for that case. Thanks! Surely it's very rare one but there was a real segfault at production server. Someone came up with the idea to modify function like public.test_selector() in repcmd.sql (see above) on the fly with adding to it :last_modification: field from current time and some other parameters with the help of replace() inside the creation of the rebuild_test() procedure. On 03.11.2022 18:29, Tom Lane wrote: > Amit Kapila <amit.kapila16@gmail.com> writes: >> LGTM. I don't know if it is a good idea to omit the test case for this >> scenario. If required, we can reuse the test case from Sawada-San's >> patch in the email [1]. > > I don't think the cost of that test case is justified by the tiny > probability that it'd ever catch anything. If we were just adding a > query or two to an existing scenario, that could be okay; but spinning > up and syncing a whole new primary and standby database is *expensive* > when you multiply it by the number of times developers and buildfarm > animals are going to run the tests in the future. > > There's also the little issue that I'm not sure it would actually > detect a problem if we had one. The case is going to fail, and > what we want to know is just how messily it fails, and I think the > TAP infrastructure isn't very sensitive to that ... especially > if the test isn't even checking for specific error messages. Seems it is possible to do a test without these remarks. The attached test uses existing nodes and checks the specific error message. Additionally i've tried to reduce overall number of nodes previously used in this test in a similar way. Would be glad for comments and remarks. With best wishes, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Commits
-
Avoid crash after function syntax error in a replication worker.
- dea834938005 16.0 landed
- f2dc7f9e35a2 15.1 landed
- ed019b5ef955 11.18 landed
- d9ffccf8db4b 12.13 landed
- b00f342ea0f0 13.9 landed
- 5f3cec77b1b5 10.23 landed
- 2489c38cdc58 14.6 landed