On Wed, Jun 25, 2025 at 7:27 PM Amit Kapila wrote:
>
> On Wed, Jun 25, 2025 at 8:38 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > Here is the V41 patch set which includes the following changes:
> >
>
> Few comments on 0004
> ===================
> 1.
> +
> +# Remember the next transaction ID to be assigned my $next_xid =
> +$node_A->safe_psql('postgres', "SELECT txid_current() + 1;");
> +
> +# Confirm that the xmin value is updated ok( $node_A->poll_query_until(
> +'postgres', "SELECT xmin = $next_xid from pg_replication_slots WHERE
> +slot_name =
> 'pg_conflict_detection'"
> + ),
> + "the xmin value of slot 'pg_conflict_detection' is updated on Node
> + A");
> +
>
> Why use an indirect way to verify that the vacuum can now remove rows?
> Even if we want to check that the conflict slot is getting updated properly, we
> should verify that the vacuum has removed the deleted rows. Also, please
> improve comments for this test, as it is not very clear why you are expecting the
> latest xid value of conflict_slot.
I agree that testing VACUUM is straightforward. But I think there is a gap
between applying remote changes and updating slot.xmin in the launcher.
Therefore, it's necessary to wait for the launcher to update the slot before
testing whether VACUUM can remove the dead tuple.
I have improved the comments and added the VACUUM test as
suggested after the slot.xmin test.
>
> 2.
> +# Alter failover for enabled subscription my ($cmdret, $stdout,
> +$stderr) = $node_A->psql('postgres', "ALTER SUBSCRIPTION
> $subname_AB
> +SET (retain_conflict_info = true)"); ok( $stderr =~
> + /ERROR: cannot set option \"retain_conflict_info\" for enabled
> subscription/,
> + "altering retain_conflict_info is not allowed for enabled
> + subscription");
> +
> +# Disable the subscription
> +($cmdret, $stdout, $stderr) = $node_A->psql('postgres', "ALTER
> +SUBSCRIPTION $subname_AB DISABLE;"); ok( $stderr =~
> + /WARNING: deleted rows to detect conflicts would not be removed
> until the subscription is enabled/,
> + "A warning is raised on disabling the subscription if
> retain_conflict_info is enabled");
> +
> +# Alter failover for disabled subscription ($cmdret, $stdout, $stderr)
> += $node_A->psql('postgres', "ALTER SUBSCRIPTION $subname_AB SET
> +(retain_conflict_info = true);"); ok( $stderr =~
> + /NOTICE: deleted rows to detect conflicts would not be removed
> until the subscription is enabled/,
> + "altering retain_conflict_info is allowed for disabled subscription");
>
> In all places, the comments use failover as an option name, whereas it is
> testing retain_conflict_info.
Changed.
>
> 3. It is better to merge the 0004 into 0001 as it tests the core part of the
> functionality added by 0001.
Merged.
Here is the V41 patch set which includes the following changes:
0001:
* ran pgindent
* Merge the original v41-0004 tap-test.
* Addressed the comments above.
* Addressed the Shveta's comments[1].
0002:
* ran pgindent
0003:
* ran pgindent
0004:
* ran pgindent
0005:
* ran pgindent
[1] https://www.postgresql.org/message-id/CAJpy0uAg1mTcy00nR%3DVAx1nTJYRkQF84YOY4_YKh8L53A1t6sA%40mail.gmail.com
Best Regards,
Hou zj