From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
Cc: | "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de> |
Subject: | Re: persist logical slots to disk during shutdown checkpoint |
Date: | 2023-09-06 06:31:00 |
Message-ID: | CAA4eK1JFqbZAQeGVyqAFf4HD5kVQS_U4CCmUds=3Zy_2hfon3A@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Sep 6, 2023 at 9:57 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Wed, Sep 6, 2023 at 9:47 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Sep 5, 2023 at 6:00 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> >
> > Right, it can change and in that case, the check related to
> > confirm_flush LSN will fail during the upgrade. However, the point is
> > that if we don't take spinlock, we need to properly write comments on
> > why unlike in other places it is safe here to check these values
> > without spinlock.
>
> I agree with that, but now also it is not true that we are alway
> reading this under the spin lock for example[1][2], we can see we are
> reading this without spin lock.
> [1]
> StartLogicalReplication
> {
> /*
> * Report the location after which we'll send out further commits as the
> * current sentPtr.
> */
> sentPtr = MyReplicationSlot->data.confirmed_flush;
> }
> [2]
> LogicalIncreaseRestartDecodingForSlot
> {
> /* candidates are already valid with the current flush position, apply */
> if (updated_lsn)
> LogicalConfirmReceivedLocation(slot->data.confirmed_flush);
> }
>
These are accessed only in walsender and confirmed_flush is always
updated by walsender. So, this is clearly okay.
> We can do that but I feel we have to be careful for
> > all future usages of these variables, so, having spinlock makes them
> > follow the normal coding pattern which I feel makes it more robust.
> > Yes, marking dirty via common function also has merits but personally,
> > I find it better to follow the normal coding practice of checking the
> > required fields under spinlock. The other possibility is to first
> > check if we need to mark the slot dirty under spinlock, then release
> > the spinlock, and then call the common MarkDirty function, but again
> > that will be under the assumption that these flags won't change.
>
> Thats true, but we are already making the assumption because now also
> we are taking the spinlock and taking a decision of marking the slot
> dirty. And after that we are releasing the spin lock and if we do not
> have guarantee that it can not concurrently change the many things can
> go wrong no?
>
Also, note that invalidated field could be updated by startup process
but that is only possible on standby, so it is safe but again that
would be another assumption.
> Anyway said that, I do not have any strong objection against what we
> are doing now. There were discussion around making the code so that
> it can use common function and I was suggesting how it could be
> achieved but I am not against the current way either.
>
Okay, thanks for looking into it.
--
With Regards,
Amit Kapila.
From | Date | Subject | |
---|---|---|---|
Next Message | Dilip Kumar | 2023-09-06 06:38:57 | Re: persist logical slots to disk during shutdown checkpoint |
Previous Message | John Naylor | 2023-09-06 06:23:43 | Re: [PoC] Improve dead tuple storage for lazy vacuum |