Re: [PATCH] Add pg_get_role_ddl() functions for role recreation

On Wed, 07 Jan 2026 at 16:28, Mario González Troncoso <gonzalemario@gmail.com> wrote:
> On Tue, 6 Jan 2026 at 03:27, Japin Li <japinli@hotmail.com> wrote:
>>
> [...]
>>
>> Thanks for updating the patch.  Some comments on v4
>>
>> 1.
>>
>> +       const char *separator = " ";
>> +
>> ...
>> +       appendStringInfo(&buf, "%s%s", separator,
>> +                                        roleform->rolcanlogin ? "LOGIN" : "NOLOGIN");
>> +
>>
>> The separator is never changed in pg_get_role_ddl_internal(), so we can remove
>> the variable and hard-code it in appendStringInfo().
>>
>
> Is that what you mean by "remove the variable and hard-code"?
>
> @@ -578,7 +578,6 @@ pg_get_role_ddl_internal(Oid roleid)
> - const char *separator = " ";
>
> tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
> if (!HeapTupleIsValid(tuple))
> @@ -605,34 +604,34 @@ pg_get_role_ddl_internal(Oid roleid)
> * you'd typically write them in a CREATE ROLE command, though any order
> * is actually acceptable to the parser.
> */
>
>
> - appendStringInfo(&buf, "%s%s", separator,
> -                                roleform->rolcanlogin ? "LOGIN" : "NOLOGIN");
> -
> - appendStringInfo(&buf, "%s%s", separator,
> + appendStringInfo(&buf, " %s",
>
> The lines above are a snippet of the latest commit `WIP: removing
> "separator"` on   https://cirrus-ci.com/build/4621719253549056
> Would you be able to see the whole change over there? If that's what
> you mean, I'll squash afterwards and attach a new patch version to
> this thread.
>

Yeah, you read my mind.

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.