Обсуждение: Plain strdup() in frontend code

Reading code I noticed that we in a few rare instances use strdup() in frontend
utilities instead of pg_strdup().  Is there a reason for not using pg_strdup()
consistently as per the attached patch?

cheers ./daniel

On Mon, Apr 29, 2019 at 11:47:27AM +0000, Daniel Gustafsson wrote:
> Reading code I noticed that we in a few rare instances use strdup() in frontend
> utilities instead of pg_strdup().  Is there a reason for not using pg_strdup()
> consistently as per the attached patch?

I think that it is good practice to encourage its use, so making
things more consistent is a good idea.  While on it, we could also
switch psql's do_lo_import() which uses a malloc() to
pg_malloc_extended() with MCXT_ALLOC_NO_OOM.  GetPrivilegesToDelete()
in pg_ctl also has an instance of malloc() with a similar failure
mode.
--
Michael

On Monday, April 29, 2019 3:01 PM, Michael Paquier <michael@paquier.xyz> wrote:

> On Mon, Apr 29, 2019 at 11:47:27AM +0000, Daniel Gustafsson wrote:
>
> > Reading code I noticed that we in a few rare instances use strdup() in frontend
> > utilities instead of pg_strdup(). Is there a reason for not using pg_strdup()
> > consistently as per the attached patch?
>
> I think that it is good practice to encourage its use, so making
> things more consistent is a good idea. While on it, we could also
> switch psql's do_lo_import() which uses a malloc() to
> pg_malloc_extended() with MCXT_ALLOC_NO_OOM. GetPrivilegesToDelete()
> in pg_ctl also has an instance of malloc() with a similar failure
> mode.

Good point, I've updated the patch to include those as well.


cheers ./daniel

On Mon, Apr 29, 2019 at 01:35:12PM +0000, Daniel Gustafsson wrote:
> Good point, I've updated the patch to include those as well.

I have been reviewing this patch, and the change in pg_waldump is
actually a good thing, as we could finish with a crash if strdup()
returns NULL as the pointer gets directly used, and there would be an
assertion failure in open_file_in_directory().

parseAclItem() in dumputils.c gets changed so as we would not return
false on OOM anymore.  I think that the current code is a bug as
parseAclItem() should return false to the caller only on a parsing
error so the caller can get confused between the OOM on strdup() and a
parsing problem.

In short, as presented, the patch looks acceptable to me.  Are there
any objections to apply it on HEAD?
--
Michael

On Tue, Apr 30, 2019 at 10:23:51AM +0900, Michael Paquier wrote:
> In short, as presented, the patch looks acceptable to me.  Are there
> any objections to apply it on HEAD?

And committed.
--
Michael