On Wed, Jan 6, 2016 at 8:14 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> Following are my observations on the latest patch.
Thanks for your review.
> + If no WAL receiver is present on the server queried,
> + a single tuple filled with <literal>NULL</> values is returned instead.
> + </para>
>
> The above documentation change is not required if we change the system
> view.
Affirmative.
> + s.received_up_to_lsn,
>
> The column name can be changed as "received_lsn" similar to "received_tli".
> up_to may not be required.
>
> + XLogRecPtr received_up_lsn;
> + TimeLineID received_up_tli;
>
> same as like above comment.
Indeed, let's make the variable names more simple and consistent by
removing this _up_ portion everywhere.
> + /* lock? */
>
> I find out that walrcv data is updated only under mutex. it is better
> to take that mutex to provide a consistent snapshot data to user.
The lock is taken, the comment is just incorrect:
+ /* lock? */
+ SpinLockAcquire(&walrcv->mutex);
[...]
+ SpinLockRelease(&walrcv->mutex);
I also found out that the description of those fields was not clear
enough actually: received_tli and received _lsn are related to what
has been received *and* flushed to disk, with an initial value being
their start equivalent. This deserves a clear description with all
those things addressed.
Attached is an updated patch.
--
Michael
