Re: Copy function for logical replication slots

From: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
To: Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Copy function for logical replication slots
Date: 2019-01-15 01:56:04
Message-ID: CAD21AoA10n5qXYkdLcUZj=2+DqZp2a0O-FRq6sfWAHiuhifT+A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Tue, Nov 27, 2018 at 3:46 AM Petr Jelinek
<petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
>
> Hi,
>
> On 26/11/2018 01:29, Masahiko Sawada wrote:
> > On Sun, Nov 25, 2018 at 12:27 AM Petr Jelinek
> > <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
> >>
> >> The more serious thing is:
> >>
> >>> + if (MyReplicationSlot)
> >>> + ReplicationSlotRelease();
> >>> +
> >>> + /* Release the saved slot if exist while preventing double releasing */
> >>> + if (savedslot && savedslot != MyReplicationSlot)
> >>
> >> This won't work as intended as the ReplicationSlotRelease() will set
> >> MyReplicationSlot to NULL, you might need to set aside MyReplicationSlot
> >> to yet another temp variable inside this function prior to releasing it.
> >>
> >
> > You're right. I've fixed it by checking if we need to release the
> > saved slot before releasing the origin slot. Is that right?
> > Attached an updated patch.
> >
>
> Sounds good.
>
> I do have one more minor gripe after reading though again:
>

Thank you for the review comment and sorry for the late response.

> > +
> > + /*
> > + * The requested wal lsn is no longer available. We don't want to retry
> > + * it, so raise an error.
> > + */
> > + if (!XLogRecPtrIsInvalid(requested_lsn))
> > + {
> > + char filename[MAXFNAMELEN];
> > +
> > + XLogFileName(filename, ThisTimeLineID, segno, wal_segment_size);
> > + ereport(ERROR,
> > + (errmsg("could not reserve WAL segment %s", filename)));
> > + }
>
> I would reword the comment to something like "The caller has requested a
> specific wal lsn which we failed to reserve. We can't retry here as the
> requested wal is no longer available." (It took me a while to understand
> this part).
>
> Also the ereport should have errcode as it's going to be thrown to user
> sessions and it might be better if the error itself used same wording as
> CheckXLogRemoved() and XLogRead() for consistency. What do you think?
>

I agreed your both comments. I've changed the above comment and
ereport. Attached the updated version patch.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Attachment Content-Type Size
v8-0001-Copy-function-for-logical-and-physical-replicatio.patch application/octet-stream 40.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tsunakawa, Takayuki 2019-01-15 02:00:57 RE: Libpq support to connect to standby server as priority
Previous Message Amit Langote 2019-01-15 01:51:54 Re: Query with high planning time at version 11.1 compared versions 10.5 and 11.0