On Wed, Feb 1, 2023 at 5:37 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote: for (int64 i = 1; i <= lastusedid; i++) { char originname_to_drop[NAMEDATALEN] = {0}; snprintf(originname_to_drop, sizeof(originname_to_drop), "pg_%u_%lld", subid, (long long) i); ....... }
--Is it better to use the function 'ReplicationOriginNameForLogicalRep' here instead of sprintf, just to be consistent everywhere to construct origin-name?
ReplicationOriginNameForLogicalRep creates a slot name with current "lastusedid" and doesn't accept that id as parameter. Here the patch needs to check all possible id's.
/* Drop replication origin */ replorigin_drop_by_name(originname, true, false); }
--Are we passing missing_ok as true (second arg) intentionally here in replorigin_drop_by_name? Once we fix the issue reported in my earlier email (ASSERT), do you think it makes sense to pass missing_ok as false here?
Yes, missing_ok is intentional. The user might be concurrently refreshing the sub or the apply worker might already drop the origin at that point. So, missing_ok is set to true.
This is also how origin drops before the worker exits are handled on HEAD too. I only followed the same approach.
--Do we need to palloc for each relation separately? Shall we do it once outside the loop and reuse it? Also pfree is not done for rstate here.
Removed palloc from the loop. No need to pfree here since the memory context will be deleted with the next CommitTransactionCommand call.
Can you please review the above flow (I have given line# along with), I think it could be problematic. We alloced prev_slotname, assigned it to slotname, freed prev_slotname and used slotname after freeing the prev_slotname. Also slotname is allocated some memory too, that is not freed.
Right, I used memcpy instead of assigning prev_slotname to slotname. slotname is returned in the end and pfree'd later [1]
I also addressed your other reviews that I didn't explicitly mention in this email. I simply applied the changes you pointed out. Also added some more logs as well. I hope it's more useful now.
