On Thu, Jun 22, 2023 at 11:37 AM Hayato Kuroda (Fujitsu) <kuroda.hay...@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