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.jelinek@2ndquadrant.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