Re: [PATCH] Tiny optmization.

On Fri, Nov 22, 2019 at 10:24:05PM +0000, Ranier Vilela wrote:
>Hi,
>pstate is touched here:
>a) BeginCopy line 1489:
> ProcessCopyOptions(pstate, cstate, is_from, options);
>b) ProcessCopyOptions line 1137:
>
> if (format_specified)
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("conflicting or redundant options"),
> parser_errposition(pstate, defel->location)));
>

And? The fact that you pass a (possibly NULL) pointer somewhere does not
make that a dereference. And parser_errposition() does this:

if (pstate == NULL || pstate->p_sourcetext == NULL)
return 0;

So I fail to see why this would be an issue?

>best regards.
>Ranier Vilela
>
>________________________________________
>De: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
>Enviado: sexta-feira, 22 de novembro de 2019 22:17
>Para: Ranier Vilela
>Cc: pgsql-hackers(at)postgresql(dot)org
>Assunto: Re: [PATCH] Tiny optmization.
>
>Ranier Vilela <ranier_gyn(at)hotmail(dot)com> writes:
>> Redudant because he it's been dereferenced here:
>> line 3410:
>> cstate = BeginCopy(pstate, true, rel, NULL, InvalidOid, attnamelist, options);
>
>Not necessarily ... the rel!=NULL code path there doesn't touch pstate,
>and that seems to be what contrib/file_fdw is relying on.
>
>Arguably, the rel==NULL code path in BeginCopy should be prepared to
>support pstate being null, too. But what you proposed here is certainly
>not OK.
>
> regards, tom lane
>
>

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