And I have another confusion about function GetCheapestReplicaIdentityFullPath: If rel->pathlist is NIL, could we return NULL directly from this function, and then set idxoid to InvalidOid in function FindUsableIndexForReplicaIdentityFull in that case?
We could, but then we need to move some other checks to some other places. I find the current flow easier to follow, where all happens via cheapest_total_path, which is a natural field for this purpose.
1. ``` +# Basic test where the subscriber uses index +# and only updates 1 row for and deletes +# 1 other row ``` There seems to be an extra "for" here.
Fixed
2. Typos for subscription name in the error messages. tap_sub_rep_full_0 -> tap_sub_rep_full
Fixed
3. Typo in comments ``` +# use the newly created index (provided that it fullfils the requirements). ``` fullfils -> fulfils
Fixed
4. Some extra single quotes at the end of the error message ('"). For example: ``` # wait until the index is used on the subscriber $node_subscriber->poll_query_until( 'postgres', q{select (idx_scan = 200) from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx';} ) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates 200 rows via index'"; ```
All fixed, thanks
5. The column names in the error message appear to be a typo. ``` +) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates two rows via index scan with index on high cardinality column-1"; ... +) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates two rows via index scan with index on high cardinality column-3"; ... +) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates two rows via index scan with index on high cardinality column-4"; ``` It seems that we need to do the following change: 'column-3' -> 'column-1' and 'column-4' -> 'column-2'. Or we could use the column names directly like this: 'column-1' -> 'column a', 'column_3' -> 'column a' and 'column_4' -> 'column b'.
I think the latter is easier to follow, thanks.
6. DELETE action is missing from the error message. ``` +# 2 rows from first command, another 2 from the second command +# overall index_on_child_1_a is used 4 times +$node_subscriber->poll_query_until( + 'postgres', q{select idx_scan=4 from pg_stat_all_indexes where indexrelname = 'index_on_child_1_a';} +) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates child_1 table'"; ``` I think we execute both UPDATE and DELETE for child_1 here. Could we add DELETE action to this error message?
makes sense, added
7. Table name in the error message. ``` # check if the index is used even when the index has NULL values $node_subscriber->poll_query_until( 'postgres', q{select idx_scan=2 from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx';} ) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates parent table'"; ``` It seems to be "test_replica_id_full" here instead of "parent'".
