On Mon, Feb 14, 2011 at 12:08 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> I committed the patch with those changes, and some minor comment tweaks and
>> other kibitzing.
>
> + * 'd' means a standby reply wrapped in a COPY BOTH packet.
> + */
>
> Typo: s/COPY BOTH/CopyData
Fixed.
> + msgtype = pq_getmsgbyte(&input_message);
> + if (msgtype != 'r')
> + ereport(COMMERROR,
> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
> + errmsg("unexpected message type %c", msgtype)));
>
> I think that proc_exit(0) needs to be called in error case.
Fixed.
> + static StringInfoData input_message;
> + StandbyReplyMessage reply;
> + char msgtype;
> +
> + initStringInfo(&input_message);
>
> Doesn't the repeat of initStringInfo() cause the memory leak?
Yeah. Fixed, I hope.
> @@ -518,6 +584,7 @@ WalSndLoop(void)
> {
> if (!XLogSend(output_message, &caughtup))
> break;
> + ProcessRepliesIfAny();
>
> Why is ProcessRepliesIfAny() required there?
I'm not sure if that's 100% necessary, but it seems harmless enough.
> We added new columns "write_location", "flush_location" and
> "apply_location". So, for the sake of consistency, the column
> name "sent_location" should be changed to "send_location"?
I was thinking about stream_location or streaming_location, per
discussion on the other thread.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company