Обсуждение: 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

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.

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

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.

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

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.

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