Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL

On Thu, Jun 22, 2023 at 11:37 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear hackers,
> (CC: Önder because he owned the related thread)
>
...
> The current patch only includes tests for hash indexes. These are separated into
> the file 032_subscribe_use_index.pl for convenience, but will be integrated in a
> later version.
>

Hi Kuroda-san.

I am still studying the docs references given in your initial post.

Meanwhile, FWIW, here are some minor review comments for the patch you gave.

======
src/backend/executor/execReplication.c

1. get_equal_strategy_number

+/*
+ * Return the appropriate strategy number which corresponds to the equality
+ * comparisons.
+ *
+ * TODO: support other indexes: GiST, GIN, SP-GiST, BRIN
+ */
+static int
+get_equal_strategy_number(Oid opclass)
+{
+ Oid am_method = get_opclass_method(opclass);
+ int ret;
+
+ switch (am_method)
+ {
+ case BTREE_AM_OID:
+ ret = BTEqualStrategyNumber;
+ break;
+ case HASH_AM_OID:
+ ret = HTEqualStrategyNumber;
+ break;
+ case GIST_AM_OID:
+ case GIN_AM_OID:
+ case SPGIST_AM_OID:
+ case BRIN_AM_OID:
+ /* TODO */
+ default:
+ /* XXX: Do we have to support extended indexes? */
+ ret = InvalidStrategy;
+ break;
+ }
+
+ return ret;
+}

1a.
In the file syscache.c there are already some other functions like
get_op_opfamily_strategy so I am wondering if this new function also
really belongs in that file.

~

1b.
Should that say "operator" instead of "comparisons"?

~

1c.
"am" stands for "access method" so "am_method" is like "access method
method" – is it correct?

~~~

2. build_replindex_scan_key

  int table_attno = indkey->values[index_attoff];
+ int strategy_number;


Ought to say this is a strategy for "equality", so a better varname
might be "equality_strategy_number" or "eq_strategy" or similar.

======
src/backend/replication/logical/relation.c

3. IsIndexUsableForReplicaIdentityFull

It seems there is some overlap between this code which hardwired 2
valid AMs and the switch statement in your other
get_equal_strategy_number function which returns a strategy number for
those 2 AMs.

Would it be better to make another common function like
get_equality_strategy_for_am(), and then you don’t have to hardwire
anything? Instead, you can say:

is_usable_type = get_equality_strategy_for_am(indexInfo->ii_Am) !=
InvalidStrategy;

======
src/backend/utils/cache/lsyscache.c

4. get_opclass_method

+/*
+ * get_opclass_method
+ *
+ * Returns the OID of the index access method operator family is for.
+ */
+Oid
+get_opclass_method(Oid opclass)
+{
+ HeapTuple tp;
+ Form_pg_opclass cla_tup;
+ Oid result;
+
+ tp = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclass));
+ if (!HeapTupleIsValid(tp))
+ elog(ERROR, "cache lookup failed for opclass %u", opclass);
+ cla_tup = (Form_pg_opclass) GETSTRUCT(tp);
+
+ result = cla_tup->opcmethod;
+ ReleaseSysCache(tp);
+ return result;
+}

Is the comment correct? IIUC, this function is not doing anything for
"families".

======
src/test/subscription/t/034_hash.pl

5.
+# insert some initial data within the range 0-9 for y
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO test_replica_id_full SELECT i, (i%10)::text FROM
generate_series(0,10) i"
+);

Why does the comment only say "for y"?

~~~

6.
+# wait until the index is used on the subscriber
+# XXX: the test will be suspended here
+$node_publisher->wait_for_catchup($appname);
+$node_subscriber->poll_query_until('postgres',
+ q{select (idx_scan = 4) 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 4 rows via index";
+

AFAIK this is OK but it was slightly misleading because it says
"updates 4 rows" whereas the previous UPDATE was only for 2 rows. So
here I think the 4 also counts the 2 DELETED rows. The comment can be
expanded slightly to clarify this.

~~~

7.
FYI, when I ran the TAP test the result was this:

t/034_hash.pl ...................... 1/? # Tests were run but no plan
was declared and done_testing() was not seen.
t/034_hash.pl ...................... All 2 subtests passed
t/100_bugs.pl ...................... ok

Test Summary Report
-------------------
t/034_hash.pl                    (Wstat: 0 Tests: 2 Failed: 0)
  Parse errors: No plan found in TAP output
Files=36, Tests=457, 338 wallclock secs ( 0.29 usr  0.07 sys + 206.73
cusr 47.94 csys = 255.03 CPU)
Result: FAIL

~

Just adding the missing done_testing() seemed to fix this.

------
Kind Regards,
Peter Smith.
Fujitsu Australia