pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedissue in pgfdw_report_error()?
7+ messages / 2 participants
[nested] [flat]
* issue in pgfdw_report_error()?
@ 2021-11-19 08:18 Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 12:57 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Fujii Masao @ 2021-11-19 08:18 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: issue in pgfdw_report_error()?
2021-11-19 08:18 issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2021-11-19 12:57 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-11-19 15:18 ` Re: issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 7+ messages in thread
From: Bharath Rupireddy @ 2021-11-19 12:57 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri, Nov 19, 2021 at 1:48 PM Fujii Masao <masao.fujii@oss.nttdata.com> 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.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: issue in pgfdw_report_error()?
2021-11-19 08:18 issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 12:57 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
@ 2021-11-19 15:18 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 16:16 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Fujii Masao @ 2021-11-19 15:18 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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 CORPORATION
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,
Attachments:
[text/plain] pgfdw_report_error_v1.patch (690B, ../../41aae5d4-4d11-85f6-8f32-1bb5349ce959@oss.nttdata.com/2-pgfdw_report_error_v1.patch)
download | inline diff:
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,
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: issue in pgfdw_report_error()?
2021-11-19 08:18 issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 12:57 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-11-19 15:18 ` Re: issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2021-11-19 16:16 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-11-22 02:47 ` Re: issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 7+ messages in thread
From: Bharath Rupireddy @ 2021-11-19 16:16 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri, Nov 19, 2021 at 8:48 PM Fujii Masao <masao.fujii@oss.nttdata.com> 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.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: issue in pgfdw_report_error()?
2021-11-19 08:18 issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 12:57 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-11-19 15:18 ` Re: issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 16:16 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
@ 2021-11-22 02:47 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-22 04:59 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Fujii Masao @ 2021-11-22 02:47 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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 CORPORATION
diff --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,
Attachments:
[text/plain] pgfdw_report_error_v2.patch (1.3K, ../../192a2107-7e37-64d6-c5b3-c45448ba2e9e@oss.nttdata.com/2-pgfdw_report_error_v2.patch)
download | inline diff:
diff --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,
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: issue in pgfdw_report_error()?
2021-11-19 08:18 issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 12:57 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-11-19 15:18 ` Re: issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 16:16 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-11-22 02:47 ` Re: issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2021-11-22 04:59 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-12-03 08:41 ` Re: issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 7+ messages in thread
From: Bharath Rupireddy @ 2021-11-22 04:59 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Nov 22, 2021 at 8:17 AM Fujii Masao <masao.fujii@oss.nttdata.com> 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.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: issue in pgfdw_report_error()?
2021-11-19 08:18 issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 12:57 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-11-19 15:18 ` Re: issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 16:16 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-11-22 02:47 ` Re: issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-22 04:59 ` Re: issue in pgfdw_report_error()? Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
@ 2021-12-03 08:41 ` Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 0 replies; 7+ messages in thread
From: Fujii Masao @ 2021-12-03 08:41 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 2021/11/22 13:59, Bharath Rupireddy wrote:
> On Mon, Nov 22, 2021 at 8:17 AM Fujii Masao <masao.fujii@oss.nttdata.com> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2021-12-03 08:41 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-11-19 08:18 issue in pgfdw_report_error()? Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 12:57 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-11-19 15:18 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-19 16:16 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-11-22 02:47 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2021-11-22 04:59 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-12-03 08:41 ` Fujii Masao <masao.fujii@oss.nttdata.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox