RE: [PATCH] Tiny optmization.

Hi,
Redudant because he it's been dereferenced here:

line 3410:
cstate = BeginCopy(pstate, true, rel, NULL, InvalidOid, attnamelist, options);

Best regards.
Ranier Vilela

________________________________________
De: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
Enviado: sexta-feira, 22 de novembro de 2019 22:05
Para: Ranier Vilela
Cc: pgsql-hackers(at)postgresql(dot)org
Assunto: Re: [PATCH] Tiny optmization.

On Fri, Nov 22, 2019 at 09:41:44PM +0000, Ranier Vilela wrote:
>Hi,
>Maybe it doesn't matter, but, I think it's worth discussing.
>The expression "if(pstate)" is redundant or is possible null dereference.
>

Eh? Redundant with what? Why would it be a null dereference? It's a
parameter passed from outside, and we're not checking it before. And
the if condition is there exactly to prevent null dereference.

It's generally a good idea to inspect existing callers of the modified
function and try running tests before submitting a patch. In this case
there's a BeginCopyFrom() call in contrib/file_fdw, passing NULL as the
first parameter, and if you run `make check` for that module it falls
flat on it's face due to a segfault.

regards

>
>--- \dll\postgresql-12.0\a\backend\commands\copy.c Mon Sep 30 17:06:55 2019
>+++ copy.c Fri Nov 22 18:33:05 2019
>@@ -3426,8 +3426,7 @@
> cstate->raw_buf_index = cstate->raw_buf_len = 0;
>
> /* Assign range table, we'll need it in CopyFrom. */
>- if (pstate)
>- cstate->range_table = pstate->p_rtable;
>+ cstate->range_table = pstate->p_rtable;
>
> tupDesc = RelationGetDescr(cstate->rel);
> num_phys_attrs = tupDesc->natts;

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services