Hi Shveta, Thanks for reviewing. Please see attached patches.
shveta malik <shveta.ma...@gmail.com>, 2 Şub 2023 Per, 14:31 tarihinde şunu
yazdı:
> On Wed, Feb 1, 2023 at 5:37 PM Melih Mutlu <m.melihmu...@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.
[1]
https://github.com/postgres/postgres/blob/master/src/backend/replication/logical/worker.c#L4359
Thanks,
--
Melih Mutlu
Microsoft
v8-0001-Add-replication-protocol-cmd-to-create-a-snapsho.patch
Description: Binary data
v11-0002-Reuse-Logical-Replication-Background-worker.patch
Description: Binary data
