Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

Hi Wang, all


> 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.

Do you have a strong opinion on this?


> ===
>
> Here are some comments for test file  032_subscribe_use_index.pl on v18
> patch:
>
> 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'".
>
fixed as well.


Attached v19.

Thanks,
Onder KALACI