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