From: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)oss(dot)ntt(dot)co(dot)jp> |
---|---|
To: | markokr(at)gmail(dot)com |
Cc: | greg(at)2ndquadrant(dot)com, pgsql-hackers(at)postgresql(dot)org, mmoncure(at)gmail(dot)com, shigeru(dot)hanada(at)gmail(dot)com |
Subject: | Re: Speed dblink using alternate libpq tuple storage |
Date: | 2012-02-28 08:04:44 |
Message-ID: | 20120228.170444.122386056.horiguchi.kyotaro@oss.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
This is the new version of the patch.
It is not rebased to the HEAD because of a build error.
> It's better to restore old two-path error handling.
I restorerd "OOM and save result" route. But it seems needed to
get back any amount of memory on REAL OOM as the comment in
original code says.
So I restored the meaning of rp == 0 && errMsg == NULL as REAL
OOM which is to throw the async result away and the result will
be preserved if errMsg is not NULL. `unknown error' has been
removed.
As the result, if row processor returns 0 the parser skips to the
end of rows and returns the working result or an error result
according to whether errMsg is set or not in the row processor.
> I don't think that should be required. Just use a dummy msg.
Considering the above, pqAddRow is also restored to leave errMsg
NULL on OOM.
> There is still one EOF in v3 getAnotherTuple() -
> pqGetInt(tupnfields), please turn that one also to
> protocolerror.
pqGetInt() returns EOF only when it wants additional reading from
network if the parameter `bytes' is appropreate. Non-zero return
from it seems should be handled as EOF, not a protocol error. The
one point I had modified bugilly is also restored. The so-called
'protocol error' has been vanished eventually.
> Instead use ("%s", errmsg) as argument there. libpq code
> is noisy enough, no need to add more.
done
Is there someting left undone?
By the way, I noticed that dblink always says that the current
connection is 'unnamed' in messages the errors in
dblink_record_internal(at)dblink(dot) I could see that
dblink_record_internal defines the local variable conname = NULL
and pass it to dblink_res_error to display the error message. But
no assignment on it in the function.
It seemed properly shown when I added the code to set conname
from PG_GETARG_TEXT_PP(0) if available, in other words do that
just after DBLINK_GET_CONN/DBLINK_GET_NAMED_CONN's. It seems the
dblink's manner... This is not included in this patch.
Furthurmore dblink_res_error looks only into returned PGresult to
display the error and always says only `Error occurred on dblink
connection..: could not execute query'..
Is it right to consider this as follows?
- dblink is wrong in error handling. A client of libpq should
see PGconn by PQerrorMessage() if (or regardless of whether?)
PGresult says nothing about error.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachment | Content-Type | Size |
---|---|---|
libpq_rowproc_20120228.patch | text/x-patch | 23.8 KB |
libpq_rowproc_doc_20120228.patch | text/x-patch | 9.6 KB |
dblink_use_rowproc_20120228.patch | text/x-patch | 13.7 KB |
early_exit_20120228.diff | text/x-patch | 2.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Fujii Masao | 2012-02-28 08:22:39 | Re: pg_basebackup -x stream from the standby gets stuck |
Previous Message | Simon Riggs | 2012-02-28 07:55:31 | Re: foreign key locks, 2nd attempt |