Re: Copy function for logical replication slots

Hi,

On 15/01/2019 02:56, Masahiko Sawada wrote:
> On Tue, Nov 27, 2018 at 3:46 AM Petr Jelinek
> <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
>>> +
>>> + /*
>>> + * 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.
>

I went through this again and I am pretty much happy with the current
version. So I am going to mark it as RFC.

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