Re: issue in pgfdw_report_error()?
On 2021/11/22 13:59, Bharath Rupireddy wrote:
On Mon, Nov 22, 2021 at 8:17 AM Fujii Masao wrote:
Well, in that case, why can't we get rid of "(message_primary != NULL"
and just have "message_primary[0] != '\0' ? errmsg_internal("%s",
message_primary) : errmsg("could not obtain message string for remote
error")" ?
That's possible if we can confirm that PQerrorMessage() never returns
NULL all the cases. I'm not sure how much it's worth doing that, though..
It seems more robust to check also NULL there.
Okay.
BTW, we might have to fix it in dblink_res_error too?
Yeah, that's good idea. I included that change in the patch. Attached.
Thanks. pgfdw_report_error_v2 patch looks good to me.
Pushed. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: issue in pgfdw_report_error()?
On Mon, Nov 22, 2021 at 8:17 AM Fujii Masao wrote:
> > Well, in that case, why can't we get rid of "(message_primary != NULL"
> > and just have "message_primary[0] != '\0' ? errmsg_internal("%s",
> > message_primary) : errmsg("could not obtain message string for remote
> > error")" ?
>
> That's possible if we can confirm that PQerrorMessage() never returns
> NULL all the cases. I'm not sure how much it's worth doing that, though..
> It seems more robust to check also NULL there.
Okay.
> > BTW, we might have to fix it in dblink_res_error too?
>
> Yeah, that's good idea. I included that change in the patch. Attached.
Thanks. pgfdw_report_error_v2 patch looks good to me.
Regards,
Bharath Rupireddy.
Re: issue in pgfdw_report_error()?
On 2021/11/20 1:16, Bharath Rupireddy wrote:
With the existing code, it emits "" for message_primary[0] == '\0'
cases but with the patch it emits "could not obtain message string for
remote error".
Yes.
Well, in that case, why can't we get rid of "(message_primary != NULL"
and just have "message_primary[0] != '\0' ? errmsg_internal("%s",
message_primary) : errmsg("could not obtain message string for remote
error")" ?
That's possible if we can confirm that PQerrorMessage() never returns
NULL all the cases. I'm not sure how much it's worth doing that, though..
It seems more robust to check also NULL there.
BTW, we might have to fix it in dblink_res_error too?
Yeah, that's good idea. I included that change in the patch. Attached.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index c8b0b4ff3f..d73c616f4f 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2801,7 +2801,8 @@ dblink_res_error(PGconn *conn, const char *conname,
PGresult *res,
ereport(level,
(errcode(sqlstate),
-message_primary ? errmsg_internal("%s",
message_primary) :
+(message_primary != NULL && message_primary[0] !=
'\0') ?
+errmsg_internal("%s", message_primary) :
errmsg("could not obtain message string for remote
error"),
message_detail ? errdetail_internal("%s",
message_detail) : 0,
message_hint ? errhint("%s", message_hint) : 0,
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 4aff315b7c..5c0137220a 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -824,7 +824,8 @@ pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
ereport(elevel,
(errcode(sqlstate),
-message_primary ? errmsg_internal("%s",
message_primary) :
+(message_primary != NULL && message_primary[0]
!= '\0') ?
+errmsg_internal("%s", message_primary) :
errmsg("could not obtain message string for
remote error"),
message_detail ? errdetail_internal("%s",
message_detail) : 0,
message_hint ? errhint("%s", message_hint) : 0,
Re: issue in pgfdw_report_error()?
On Fri, Nov 19, 2021 at 8:48 PM Fujii Masao wrote:
>
> On 2021/11/19 21:57, Bharath Rupireddy wrote:
> >> If this is a bug, IMO the following change needs to be applied. Thought?
> >>
> >> ---
> >> ereport(elevel,
> >> (errcode(sqlstate),
> >> -message_primary ? errmsg_internal("%s",
> >> message_primary) :
> >> +(message_primary != NULL &&
> >> message_primary[0] != '\0') ?
> >> +errmsg_internal("%s", message_primary) :
> >>errmsg("could not obtain message string
> >> for remote error"),
> >> ---
>
> I attached the patch.
With the existing code, it emits "" for message_primary[0] == '\0'
cases but with the patch it emits "could not obtain message string for
remote error".
- message_primary ? errmsg_internal("%s", message_primary) :
+ (message_primary != NULL && message_primary[0] != '\0') ?
+ errmsg_internal("%s", message_primary) :
>
> > What if conn->errorMessage.data is NULL and PQerrorMessage returns it?
> > The message_primary can still be NULL right?
>
> Since conn->errorMessage is initialized by initPQExpBuffer(),
> PQerrorMessage() seems not to return NULL. But *if* it returns NULL,
> pchomp(NULL) is executed and would cause a segmentation fault.
Well, in that case, why can't we get rid of "(message_primary != NULL"
and just have "message_primary[0] != '\0' ? errmsg_internal("%s",
message_primary) : errmsg("could not obtain message string for remote
error")" ?
BTW, we might have to fix it in dblink_res_error too?
/*
* If we don't get a message from the PGresult, try the PGconn. This is
* needed because for connection-level failures, PQexec may just return
* NULL, not a PGresult at all.
*/
if (message_primary == NULL)
message_primary = pchomp(PQerrorMessage(conn));
ereport(level,
(errcode(sqlstate),
message_primary ? errmsg_internal("%s", message_primary) :
errmsg("could not obtain message string for remote error"),
message_detail ? errdetail_internal("%s", message_detail) : 0,
message_hint ? errhint("%s", message_hint) : 0,
Regards,
Bharath Rupireddy.
Re: issue in pgfdw_report_error()?
On 2021/11/19 21:57, Bharath Rupireddy wrote:
If this is a bug, IMO the following change needs to be applied. Thought?
---
ereport(elevel,
(errcode(sqlstate),
-message_primary ? errmsg_internal("%s",
message_primary) :
+(message_primary != NULL && message_primary[0]
!= '\0') ?
+errmsg_internal("%s", message_primary) :
errmsg("could not obtain message string for
remote error"),
---
I attached the patch.
What if conn->errorMessage.data is NULL and PQerrorMessage returns it?
The message_primary can still be NULL right?
Since conn->errorMessage is initialized by initPQExpBuffer(),
PQerrorMessage() seems not to return NULL. But *if* it returns NULL,
pchomp(NULL) is executed and would cause a segmentation fault.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 4aff315b7c..5c0137220a 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -824,7 +824,8 @@ pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
ereport(elevel,
(errcode(sqlstate),
-message_primary ? errmsg_internal("%s",
message_primary) :
+(message_primary != NULL && message_primary[0]
!= '\0') ?
+errmsg_internal("%s", message_primary) :
errmsg("could not obtain message string for
remote error"),
message_detail ? errdetail_internal("%s",
message_detail) : 0,
message_hint ? errhint("%s", message_hint) : 0,
Re: issue in pgfdw_report_error()?
On Fri, Nov 19, 2021 at 1:48 PM Fujii Masao wrote:
>
> Hi,
>
> pgfdw_report_error() in postgres_fdw is implemented to report the message
> "could not obtain ..." if message_primary is NULL as follows.
> But, just before this ereport(), message_primary is set to
> pchomp(PQerrorMessage()) if it's NULL. So ISTM that message_primary is
> always not NULL in ereport() and the message "could not obtain ..." is
> never reported. Is this a bug?
>
> ---
> if (message_primary == NULL)
> message_primary = pchomp(PQerrorMessage(conn));
>
> ereport(elevel,
> (errcode(sqlstate),
> message_primary ? errmsg_internal("%s", message_primary) :
> errmsg("could not obtain message string for remote error"),
> ---
>
>
> If this is a bug, IMO the following change needs to be applied. Thought?
>
> ---
> ereport(elevel,
> (errcode(sqlstate),
> -message_primary ? errmsg_internal("%s",
> message_primary) :
> +(message_primary != NULL &&
> message_primary[0] != '\0') ?
> +errmsg_internal("%s", message_primary) :
> errmsg("could not obtain message string for
> remote error"),
> ---
What if conn->errorMessage.data is NULL and PQerrorMessage returns it?
The message_primary can still be NULL right?
I see the other places where PQerrorMessage is used, they do check for
the NULL value in some places the others don't do.
in dblink.c:
msg = PQerrorMessage(conn);
if (msg == NULL || msg[0] == '\0')
PG_RETURN_TEXT_P(cstring_to_text("OK"));
Regards,
Bharath Rupireddy.
issue in pgfdw_report_error()?
Hi,
pgfdw_report_error() in postgres_fdw is implemented to report the message
"could not obtain ..." if message_primary is NULL as follows.
But, just before this ereport(), message_primary is set to
pchomp(PQerrorMessage()) if it's NULL. So ISTM that message_primary is
always not NULL in ereport() and the message "could not obtain ..." is
never reported. Is this a bug?
---
if (message_primary == NULL)
message_primary = pchomp(PQerrorMessage(conn));
ereport(elevel,
(errcode(sqlstate),
message_primary ? errmsg_internal("%s", message_primary) :
errmsg("could not obtain message string for remote error"),
---
If this is a bug, IMO the following change needs to be applied. Thought?
---
ereport(elevel,
(errcode(sqlstate),
-message_primary ? errmsg_internal("%s",
message_primary) :
+(message_primary != NULL && message_primary[0]
!= '\0') ?
+errmsg_internal("%s", message_primary) :
errmsg("could not obtain message string for remote
error"),
---
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
