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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
