On Tue, May 6, 2025 at 3:41 PM Nisha Moond <[email protected]> wrote: > > On Mon, May 5, 2025 at 11:39 AM David G. Johnston > <[email protected]> wrote: > > > > On Sun, May 4, 2025 at 8:45 PM Nisha Moond <[email protected]> wrote: > >> > >> Attached is the patch implementing the above proposed solution. > >> Reviews and feedback are most welcome. > > > > > > I feel like this is just papering over the issue - which is that these two > > drop functions are being used for multiple differently named > > publications/slots yet take no care to ensure they only change > > made_publication and made_repslot if the name of the object being passed in > > matches the name of the object those two booleans are specifically tracking > > (the application created objects on the publisher). > > > > Make it so they are only changed to false if the name matches the one the > > program created and the connection is the primary connection. That targets > > the real issue and avoids using a branching boolean parameter. > > > > It seems really odd to say: if (in_cleanup) "don't try again" - since by > > definition this is the last thing we are doing before we exit. So really > > what this patch can do more simply is just remove the > > dbinfo->made_replslot=false and *made_publication=false lines altogether - > > which might be a valid option. > > > > +1 to removing the dbinfo->made_replslot=false and > *made_publication=false lines. In my tests, I attempted to force > multiple failures, but couldn’t find any case where > cleanup_objects_atexit() would recurse or cause repeated cleanup if > these flags remain set to true. > > > I'm partial to the latter really, I don't think the error message output > > for retrying a drop that may have previously failed would be an issue. > > > > As of now, we don’t attempt to drop the same object more than once, so > the latter approach does seem reasonable to me. That said, I’m unsure > why the flags were being reset here in the first place. >
Hi Nisha, >From what I can tell, this flag resetting might have originated from old review comments of mine [1 #5b] and also [2 #6]. Apparently, it wasn't so much about preventing multiple errors for fdrops that had "previously failed"... it was more intended to prevent attempting to delete the same publication 2x (1st successfully, then 2nd time failing because it was already gone) Anyway, the code has probably changed lots since those review comments; I have no idea if my suspected double-delete at that time is still possible or not. Those old review comments might give you some clues on how to reproduce now that you have removed the flag resetting. ====== [1] https://www.postgresql.org/message-id/CAHut%2BPt_4f-%3Dh71qmfWhiFcqTcfqWQr1POnFdesZK1-fVOCaUA%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAHut%2BPtRZbuF_vAva6azC%2B6WVPyaoqT1fCeLNP0r5gwPEsfcwg%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia
