On Fri, Dec 13, 2024 at 12:20 PM Peter Smith <smithpb2...@gmail.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.
v7-0001-Add-support-for-two-phase-commit-in-pg_createsubs.patch
Description: Binary data
