Re: valgrind error in subscription code

On 22/04/17 21:16, Andres Freund wrote:
> Hi,
>
> On 2017-04-22 21:08:18 +0200, Petr Jelinek wrote:
>> Thanks, here is patch to fix that - I also removed the individual
>> settings of everything to NULL/0/InvalidOid etc and just replaced it all
>> with memset.
>
> Cool.
>
>> diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
>> index 875a081..5bc54dd 100644
>> --- a/src/backend/replication/logical/relation.c
>> +++ b/src/backend/replication/logical/relation.c
>> @@ -141,19 +141,10 @@ logicalrep_relmap_free_entry(LogicalRepRelMapEntry *entry)
>> pfree(remoterel->attnames);
>> pfree(remoterel->atttyps);
>> }
>> - remoterel->attnames = NULL;
>> - remoterel->atttyps = NULL;
>> -
>> bms_free(remoterel->attkeys);
>> - remoterel->attkeys = NULL;
>>
>> if (entry->attrmap)
>> pfree(entry->attrmap);
>> -
>
> Btw, I think it's a good pattern to zero things like attrmap after
> freeing. Based on a minute of looking it's unclear to me if
> logicalrep_relmap_update() could be called again, if e.g. one of the
> pallocs after the logicalrep_relmap_free_entry() errors out. I think
> you essentially addressed that with the memset, so that's good.
>

Yes, memset is called immediately after logicalrep_relmap_free_entry()
which is why I found it redundant to set things to NULL there. We could
put the memset directly inside logicalrep_relmap_free_entry() and the
call either logicalrep_relmap_free_entry() or memset in the caller if
that would make you feel better about it.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services