On 21/01/2020 12:45, Kyotaro Horiguchi wrote:
At Mon, 20 Jan 2020 17:24:07 +0900 (JST), Kyotaro Horiguchi 
<horikyota....@gmail.com> wrote in
Separating XLogBeginRead seems reasonable.  The annoying recptr trick
is no longer needed. In particular some logical-decoding stuff become
far cleaner by the patch.

fetching_ckpt of ReadRecord is a bit annoying but that coundn't be
moved outside to, say, ReadCheckpointRecord in a clean way. (Anyway
XLogBeginRead is not the place, of course).

As I looked through it, it looks good to me but give me some more time
to look closer.

It seems to me that it works perfectly, and everything looks good to
me except one point.

-                * In this case, the passed-in record pointer should already be
+                * In this case, EndRecPtr record pointer should already be

I'm not confident but isn't the "record pointer" redundant?  EndRecPtr
seems containing that meaning, and the other occurances of that kind
of variable names are not accompanied by that.

I fixed that, fixed the XLogFindNextRecord() function so that it positions the reader like XLogBeginRead() does (I had already adjusted the comments to say that, but the function didn't actually do it), and pushed. Thanks for the review!

I'll continue looking at the callback API changes on Monday.

- Heikki


Reply via email to