On Thu, Jul 28, 2022 at 11:56 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for the v2-0001 patch:
>
> ======
>
> 1. Commit message
>
> The commit message should give some reason why relocating the origin
> slot drop code is expected to fix the reported problem.
>
> ======
>
Updated.
> 2. src/backend/replication/logical/tablesync.c
>
> + /* reset the origin session before dropping */
> + replorigin_session_reset();
> +
> + replorigin_session_origin = InvalidRepOriginId;
> + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> + replorigin_session_origin_timestamp = 0;
>
> 2a.
> Uppercase comment
Fixed.
>
> 2b.
> IIUC you are not doing the reset because you particularly want to do a
> reset, but really because this is the only (?) way to dis-associate
> the current process from owning the slot. Otherwise, the slot would be
> considered still "busy" and the subsequent replorigin_drop_by_name
> would be rejected. I thought the current comment should be expanded to
> explain all this background.
Updated.
>
> 2c.
> Perhaps it is non-standard to do so, but I wondered if you can just
> call pg_replication_origin_session_reset instead of
> replorigin_session_reset here so that there would only be 1 LOC
> instead of 4.
That is a psql function, invoking that indirectly is not really a
standard practice.
>
> ~~~
>
> 3. src/backend/replication/logical/tablesync.c
>
> + /*
> * Cleanup the tablesync slot.
> *
> * This has to be done after updating the state because otherwise if
>
> Doesn't the same note ("This has to be done after...") also apply to
> the code dropping the origin slot? Maybe this note needs to be either
> duplicated or just put a common note about this above both of those
> slot drops.
>
Updated.
regards,
Ajin Cherian
Fujitsu Australia
