Re: Sync Rep for 2011CF1

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