On Mon, Sep 14, 2020 at 9:42 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapil...@gmail.com> writes:
> > The attached patch will fix the issue. What do you think?
>
> I think it'd be cleaner to separate the initialization of a new entry from
> validation altogether, along the lines of
>
> /* Find cached function info, creating if not found */
> oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> entry = (RelationSyncEntry *) hash_search(RelationSyncCache,
> (void *) &relid,
> HASH_ENTER, &found);
> MemoryContextSwitchTo(oldctx);
> Assert(entry != NULL);
>
> if (!found)
> {
> /* immediately make a new entry valid enough to satisfy callbacks */
> entry->schema_sent = false;
> entry->streamed_txns = NIL;
> entry->replicate_valid = false;
> /* are there any other fields we should clear here for safety??? */
> }
>
If we want to separate validation then we need to initialize other
fields like 'pubactions' and 'publish_as_relid' as well. I think it
will be better to arrange it the way you are suggesting. So, I will
change it along with other fields that required initialization.
> /* Fill it in if not valid */
> if (!entry->replicate_valid)
> {
> List *pubids = GetRelationPublications(relid);
> ...
>
> BTW, unless someone has changed the behavior of dynahash when I
> wasn't looking, those MemoryContextSwitchTos shown above are useless.
>
As far as I can see they are useless in this case but I think they
might be required in case the user provides its own allocator function
(using HASH_ALLOC). So, we can probably remove those from here?
> Also, why does the comment refer to a "function" entry?
>
It should be "relation" instead. I'll take care of changing this as well.
--
With Regards,
Amit Kapila.
