Plain strdup() in frontend code

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