Re: Speed dblink using alternate libpq tuple storage

On Fri, Feb 24, 2012 at 07:53:14PM +0900, Kyotaro HORIGUCHI wrote:
> Hello, this is new version of the patch.
>
> > > By the way, I would like to ask you one question. What is the
> > > reason why void* should be head or tail of the parameter list?
> >
> > Aesthetical reasons:
>
> I got it. Thank you.
>
> > Last comment - if we drop the plan to make PQsetRowProcessorErrMsg()
> > usable outside of handler, we can simplify internal usage as well:
> > the PGresult->rowProcessorErrMsg can be dropped and let's use
> > ->errMsg to transport the error message.
> >
> > The PGresult is long-lived structure and adding fields for such
> > temporary usage feels wrong. There is no other libpq code between
> > row processor and getAnotherTuple, so the use is safe.
>
> I almost agree with it. Plus, I think it is no reason to consider
> out of memory as particular because now row processor becomes
> generic. But the previous patch does different process for OOM
> and others, but I couldn't see obvious reason to do so.

On OOM, the old result is freed to have higher chance that
constructing new result succeeds. But if we want to transport
error message, we need to keep old PGresult around. Thus
two separate paths.

This also means your new code is broken, the errmsg becomes
invalid after pqClearAsyncResult().

It's better to restore old two-path error handling.

> - Now row processors should set message for any error status
> occurred within. pqAddRow and dblink.c is modified to do so.

I don't think that should be required. Just use a dummy msg.

> - getAnotherTuple() sets the error message `unknown error' for
> the condition rp == 0 && ->errMsg == NULL.

Ok. I think most client will want to drop connection
on error from rowproc, so exact message does not matter.

> - Always forward input cursor and do pqClearAsyncResult() and
> pqSaveErrorResult() when rp == 0 in getAnotherTuple()
> regardless whether ->errMsg is NULL or not in fe-protocol3.c.

Ok. Although skipping packet on OOM does is dubious,
we will skip all further packets anyway, so let's be
consistent on problems.

There is still one EOF in v3 getAnotherTuple() - pqGetInt(tupnfields),
please turn that one also to protocolerror.

> - conn->inStart is already moved to the start point of the next
> message when row processor is called. So forwarding inStart in
> outOfMemory1 seems redundant. I removed it.
>
> - printfPQExpBuffer() compains for variable message. So use
> resetPQExpBuffer() and appendPQExpBufferStr() instead.

Instead use ("%s", errmsg) as argument there. libpq code
is noisy enough, no need to add more.

> - I have no idea how to do test for protocol 2...

I have a urge to test with "rm fe-protocol2.c"...

--
marko