On Fri, Apr 28, 2023 at 6:01 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada <sawada.m...@gmail.com> > wrote: > > > > On Fri, Apr 28, 2023 at 11:51 AM Amit Kapila <amit.kapil...@gmail.com> > > wrote: > > > > > > On Wed, Apr 26, 2023 at 4:11 PM Zhijie Hou (Fujitsu) > > > <houzj.f...@fujitsu.com> wrote: > > > > > > > > On Wednesday, April 26, 2023 5:00 PM Alexander Lakhin > > > > <exclus...@gmail.com> wrote: > > > > > > > > > > IIUC, that assert will fail in case of any error raised between > > > > > ApplyWorkerMain()->logicalrep_worker_attach()->before_shmem_exit() and > > > > > ApplyWorkerMain()->InitializeApplyWorker()->BackgroundWorkerInitializeC > > > > > onnectionByOid()->InitPostgres(). > > > > > > > > Thanks for reporting the issue. > > > > > > > > I think the problem is that it tried to release locks in > > > > logicalrep_worker_onexit() before the initialization of the process is > > > > complete > > > > because this callback function was registered before the init phase. So > > > > I think we > > > > can add a conditional statement before releasing locks. Please find an > > > > attached > > > > patch. > > > > > > > > > > Alexander, does the proposed patch fix the problem you are facing? > > > Sawada-San, and others, do you see any better way to fix it than what > > > has been proposed? > > > > I'm concerned that the idea of relying on IsNormalProcessingMode() > > might not be robust since if we change the meaning of > > IsNormalProcessingMode() some day it would silently break again. So I > > prefer using something like InitializingApplyWorker, > > > > I think if we change the meaning of IsNormalProcessingMode() then it > could also break the other places the similar check is being used.
Right, but I think it's unclear the relationship between the processing modes and releasing session locks. If non-normal-processing mode means we're still in the process initialization phase, why we don't skip other cleanup works such as walrcv_disconnect() and FileSetDeleteAll()? > However, I am fine with InitializingApplyWorker as that could be used > at other places as well. I just want to avoid adding another variable > by using IsNormalProcessingMode. I think it's less confusing. > > > or another idea > > would be to do cleanup work (e.g., fileset deletion and lock release) > > in a separate callback that is registered after connecting to the > > database. > > > > Yeah, but not sure if it's worth having multiple callbacks for cleanup work. Fair point. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com