Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

On Mon, Sep 14, 2020 at 9:42 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.