From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
Cc: | Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Önder Kalacı <onderkalaci(at)gmail(dot)com> |
Subject: | Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL |
Date: | 2023-06-27 01:02:06 |
Message-ID: | CAHut+Ps8dnz-t1n8Rp2-x6xesmrW=HxO3U6b9pE-_N07Odr8AA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Jun 22, 2023 at 11:37 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Smith | 2023-06-27 01:06:25 | Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL |
Previous Message | Jacob Champion | 2023-06-26 23:55:47 | [PATCH] Honor PG_TEST_NOCLEAN for tempdirs |