Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility.

On Fri, Dec 13, 2024 at 12:20 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham.
>
> Here are my review comments for v6-0001.
>
> ======
> 1.
> +# Verify that the subtwophase is enabled ('e') in the pg_subscription catalog
> +$node_s->poll_query_until('postgres',
> + "SELECT count(1) = 2 FROM pg_subscription WHERE subtwophasestate = 'e';")
> + or die "Timed out while waiting for subscriber to enable twophase";
> +
>
> This form of the SQL is probably OK but it's a bit tricky; Probably it
> should have been explained in the comment about where that count "2"
> has come from.
>
> ~~
>
> I think it was OK as before (v5) to be querying until nothing was NOT
> 'e'. In other words, until everything was enabled 'e'.
> SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');
>
> ~~
>
> OTOH, to save execution time we really would be satisfied with both
> 'p' and 'e' states here. (we don't strictly need to wait for the
> transition from 'p' to 'e' to occur).
>
> So, SQL like the one below might be the best:
>
> # Verify that all subtwophase states are pending or enabled,
> # e.g. there are no subscriptions where subtwophase is disabled ('d').
> SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate IN ('d')
>

I have fixed the given comment. Since prepared transactions are not
being used anymore, I have removed it from the test file.
The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.