Re: Copy function for logical replication slots

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:

> +
> + /*
> + * 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?

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services