Обсуждение: Incorrect error message in InitializeSessionUserId

Hi all,

I have found incorrect error message in InitializeSessionUserId function
if you try to connect to database by role Oid (for example BackgroundWorkerInitializeConnectionByOid).
If role have no permissions to login, you will see error message like this:
FATAL:  role "(null)" is not permitted to log in

I changed few lines of code and fixed this.
Patch is attached.
I want to add this patch to commitfest.
Any objections?



Regards,
Dmitriy Sarafannikov

On Tue, Mar 1, 2016 at 10:21 PM, Dmitriy Sarafannikov
<d.sarafannikov@bk.ru> wrote:
> I have found incorrect error message in InitializeSessionUserId function
> if you try to connect to database by role Oid (for example
> BackgroundWorkerInitializeConnectionByOid).
> If role have no permissions to login, you will see error message like this:
> FATAL:  role "(null)" is not permitted to log in
>
> I changed few lines of code and fixed this.
> Patch is attached.
> I want to add this patch to commitfest.
> Any objections?

Not really. That looks like an oversight to me.
-- 
Michael

On Wed, Mar 2, 2016 at 12:21 AM, Dmitriy Sarafannikov
<d.sarafannikov@bk.ru> wrote:
> Hi all,
>
> I have found incorrect error message in InitializeSessionUserId function
> if you try to connect to database by role Oid (for example
> BackgroundWorkerInitializeConnectionByOid).
> If role have no permissions to login, you will see error message like this:
> FATAL:  role "(null)" is not permitted to log in
>
> I changed few lines of code and fixed this.
> Patch is attached.
> I want to add this patch to commitfest.
> Any objections?
>

The patch adds the support of taking the role name from the role tuple
instead of using the provided rolename variable, because it is possible
that rolename variable is NULL if the connection is from a background
worker.

The patch is fine, I didn't find any problems, I marked it as ready for
committer.

IMO this patch may need to backpatch supported branches as it is
a bug fix. Committer can decide.

Regards,
Hari Babu
Fujitsu Australia

On Fri, Mar 4, 2016 at 10:45 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Wed, Mar 2, 2016 at 12:21 AM, Dmitriy Sarafannikov
> <d.sarafannikov@bk.ru> wrote:
>> Hi all,
>>
>> I have found incorrect error message in InitializeSessionUserId function
>> if you try to connect to database by role Oid (for example
>> BackgroundWorkerInitializeConnectionByOid).
>> If role have no permissions to login, you will see error message like this:
>> FATAL:  role "(null)" is not permitted to log in
>>
>> I changed few lines of code and fixed this.
>> Patch is attached.
>> I want to add this patch to commitfest.
>> Any objections?
>>
>
> The patch adds the support of taking the role name from the role tuple
> instead of using the provided rolename variable, because it is possible
> that rolename variable is NULL if the connection is from a background
> worker.
>
> The patch is fine, I didn't find any problems, I marked it as ready for
> committer.
>
> IMO this patch may need to backpatch supported branches as it is
> a bug fix. Committer can decide.

+1 for the backpatch. The current error message should the rolename be
undefined in this context is misleading for users.
-- 
Michael

On Thu, Mar 3, 2016 at 9:23 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Mar 4, 2016 at 10:45 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> On Wed, Mar 2, 2016 at 12:21 AM, Dmitriy Sarafannikov
>> <d.sarafannikov@bk.ru> wrote:
>>> Hi all,
>>>
>>> I have found incorrect error message in InitializeSessionUserId function
>>> if you try to connect to database by role Oid (for example
>>> BackgroundWorkerInitializeConnectionByOid).
>>> If role have no permissions to login, you will see error message like this:
>>> FATAL:  role "(null)" is not permitted to log in
>>>
>>> I changed few lines of code and fixed this.
>>> Patch is attached.
>>> I want to add this patch to commitfest.
>>> Any objections?
>>>
>>
>> The patch adds the support of taking the role name from the role tuple
>> instead of using the provided rolename variable, because it is possible
>> that rolename variable is NULL if the connection is from a background
>> worker.
>>
>> The patch is fine, I didn't find any problems, I marked it as ready for
>> committer.
>>
>> IMO this patch may need to backpatch supported branches as it is
>> a bug fix. Committer can decide.
>
> +1 for the backpatch. The current error message should the rolename be
> undefined in this context is misleading for users.

Back-patched to 9.5.  I don't think this is relevant for earlier branches.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

>On Fri, Mar 4, 2016, 5:23 +03:00 от Michael Paquier < michael.paquier@gmail.com >:
>
>> The patch adds the support of taking the role name from the role tuple
>> instead of using the provided rolename variable, because it is possible
>> that rolename variable is NULL if the connection is from a background
>> worker.
>>
>> The patch is fine, I didn't find any problems, I marked it as ready for
>> committer.
>>
>> IMO this patch may need to backpatch supported branches as it is
>> a bug fix. Committer can decide.
>
>+1 for the backpatch. The current error message should the rolename be
>undefined in this context is misleading for users.
>--
>Michael

Thanks for feedback.

This patch successfully applies to the 9.5 branch.
In 9.4 and below versions InitializeSessionUserId function has other signature:
void InitializeSessionUserId(const char *rolename)
and it is impossible to pass role Oid to this function.

In this way, the patch is relevant only to the master and 9.5 branches

Regards,
Dmitriy Sarafannikov