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

Hi Hayato, all

This is a follow-up thread of [1]. The commit allowed subscribers to use
indexes
other than PK and REPLICA IDENTITY when REPLICA IDENTITY is FULL on
publisher,
but the index must be a B-tree. In this proposal, I aim to extend this
functionality to allow
for hash indexes and potentially other types.

Cool, thanks for taking the time to work on this.

# Current problem

The current limitation comes from the function build_replindex_scan_key(),
specifically these lines:

When I last dealt with the same issue, I was examining it from a slightly
broader perspective. I think
my conclusion was that RelationFindReplTupleByIndex() is designed for the
constraints of UNIQUE INDEX
and Primary Key. Hence, btree limitation was given.

So, my main point is that it might be useful to check
RelationFindReplTupleByIndex() once more in detail
to see if there is anything else that is specific to btree indexes.

build_replindex_scan_key() is definitely one of the major culprits but see
below as well.

I think we should also be mindful about tuples_equal() function. When an
index returns more than
one tuple, we rely on tuples_equal() function to make sure non-relevant
tuples are skipped.

For btree indexes, it was safe to rely on that function as the columns that
are indexed using btree
always have equality operator. I think we can safely assume the same for
hash indexes.

However, say we indexed "point" type using "gist" index. Then, if we let
this logic to kick in,
I think tuples_equal() would fail saying that there is no equality operator
exists.

One might argue that it is already the case for RelationFindReplTupleSeq()
or when you
have index but the index on a different column. But still, it seems useful
to make sure
you are aware of this limitation as well.

## Current difficulties

The challenge with supporting other indexes is that they lack a fixed set
of strategies,
making it difficult to choose the correct strategy number based on the
index
access method. Even within the same index type, different operator classes
can
use different strategy numbers for the same operation.
E.g. [2] shows that number 6 can be used for the purpose, but other
operator classes
added by btree_gist [3] seem to use number 3 for the euqlaity comparison.

Also, build_replindex_scan_key() seems like a too late place to check this?
I mean, what
if there is no equality operator, how should code react to that? It
effectively becomes
RelationFindReplTupleSeq(), so maybe better follow that route upfront?

In other words, that decision should maybe
happen IsIndexUsableForReplicaIdentityFull()?

For the specific notes you raised about strategy numbers / operator
classes, I need to
study a bit :) Though, I'll be available to do that early next week.

Thanks,
Onder

Dear Önder,

Thank you for giving comments! The author's comment is quite helpful for us.

When I last dealt with the same issue, I was examining it from a slightly broader perspective. I think
my conclusion was that RelationFindReplTupleByIndex() is designed for the constraints of UNIQUE INDEX
and Primary Key.

I see. IIUC that's why you have added tuples_equal() in the RelationFindReplTupleByIndex().

I think we should also be mindful about tuples_equal() function. When an index returns more than
one tuple, we rely on tuples_equal() function to make sure non-relevant tuples are skipped.

For btree indexes, it was safe to rely on that function as the columns that are indexed using btree
always have equality operator. I think we can safely assume the same for hash indexes.

However, say we indexed "point" type using "gist" index. Then, if we let this logic to kick in,
I think tuples_equal() would fail saying that there is no equality operator exists.

Good point. Actually I have tested for "point" datatype but it have not worked well.
Now I understand the reason.
It seemed that when TYPECACHE_EQ_OPR_FINFO is reuqesed to lookup_type_cache(),
it could return valid value only if the datatype has operator class for Btree or Hash.
So tuples_equal() might not be able to use if tuples have point box circle types.

BTW, I have doubt that the restriction is not related with your commit.
In other words, if the table has attributes which the datatype is not for operator
class of Btree, we could not use REPLICA IDENTITY FULL. IIUC it is not documented.
Please see attched script to reproduce that. The final DELETE statement cannot be
replicated at the subscriber on my env.

For the specific notes you raised about strategy numbers / operator classes, I need to
study a bit :) Though, I'll be available to do that early next week.

Thanks! I'm looking forward to see your opinions...

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

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

On Mon, Jun 26, 2023 at 11:44 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
...

BTW, I have doubt that the restriction is not related with your commit.
In other words, if the table has attributes which the datatype is not for operator
class of Btree, we could not use REPLICA IDENTITY FULL. IIUC it is not documented.
Please see attched script to reproduce that. The final DELETE statement cannot be
replicated at the subscriber on my env.

Missing attachment?

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

Please see attched script to reproduce that. The final DELETE statement cannot
be
replicated at the subscriber on my env.

Sorry, I forgot to attach...

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Hi Hayato,

BTW, I have doubt that the restriction is not related with your commit.
In other words, if the table has attributes which the datatype is not for
operator
class of Btree, we could not use REPLICA IDENTITY FULL. IIUC it is not
documented.
Please see attched script to reproduce that. The final DELETE statement
cannot be
replicated at the subscriber on my env.

Yes, I agree, it is (and was before my patch as well)
un-documented limitation of REPLICA IDENTITY FULL.
And, as far as I can see, my patch actually didn't have any impact on the
limitation. The unsupported
cases are still unsupported, but now the same error is thrown in a slightly
different place.

I think that is a minor limitation, but maybe should be listed [1]https://www.postgresql.org/docs/current/logical-replication-restrictions.html?

For the specific notes you raised about strategy numbers / operator
classes, I need to
study a bit :) Though, I'll be available to do that early next week.

Thanks! I'm looking forward to see your opinions...

For this one, I did some research in the code, but I'm not very
comfortable with the answer. Still, I wanted to share my observations so
that it might be useful for the discussion.

First, I checked if the function get_op_btree_interpretation() could be
used here. But, I think that is again btree-only and I couldn't find
anything generic that does something similar.

Then, I tried to come up with a SQL query, actually based on the link [2]https://www.postgresql.org/docs/devel/xindex.html#XINDEX-STRATEGIES
you shared. I think we should always have an "equality" strategy (e.g.,
"same", "overlaps", "contains" etc sounds wrong to me).

And, it seems btree, hash and brin supports "equal". So, a query like the
following seems to provide the list of (index type, strategy_number,
data_type) that we might be allowed to use.

SELECT
am.amname AS index_type,
amop.amoplefttype::regtype,amop.amoprighttype::regtype,
op.oprname AS operator,
amop.amopstrategy AS strategy_number
FROM
pg_amop amop
JOIN
pg_am am ON am.oid = amop.amopmethod
JOIN
pg_operator op ON op.oid = amop.amopopr
WHERE
(am.amname = 'btree' and amop.amopstrategy = 3) OR
(am.amname = 'hash' and amop.amopstrategy = 1) OR
(am.amname = 'brin' and amop.amopstrategy = 3)
ORDER BY
index_type,
strategy_number;

What do you think?

[1]: https://www.postgresql.org/docs/current/logical-replication-restrictions.html
https://www.postgresql.org/docs/current/logical-replication-restrictions.html

[2]: https://www.postgresql.org/docs/devel/xindex.html#XINDEX-STRATEGIES

Dear Önder,

Thank you for your analysis!

Yes, I agree, it is (and was before my patch as well) un-documented limitation of REPLICA IDENTITY FULL.
And, as far as I can see, my patch actually didn't have any impact on the limitation. The unsupported
cases are still unsupported, but now the same error is thrown in a slightly different place.
I think that is a minor limitation, but maybe should be listed [1]/messages/by-id/CAHut+PsFdTZJ7DG1jyu7BpA_1d4hwEd-Q+mQAPWcj1ZLD_X5Dw@mail.gmail.com?

Yes, your modification did not touch the restriction. It has existed before the
commit. I (or my colleague) will post the patch to add the description, maybe
after [1]/messages/by-id/CAHut+PsFdTZJ7DG1jyu7BpA_1d4hwEd-Q+mQAPWcj1ZLD_X5Dw@mail.gmail.com is committed.

For this one, I did some research in the code, but I'm not very
comfortable with the answer. Still, I wanted to share my observations so
that it might be useful for the discussion.

First, I checked if the function get_op_btree_interpretation() could be
used here. But, I think that is again btree-only and I couldn't find
anything generic that does something similar.

Thanks for checking. The function seems to return the list of operator family and
its strategy number when the oid of the operator is given. But what we want to do
here is get the operator oid. I think that the input and output of the function
seems opposite. And as you mentioned, the index must be btree.

Then, I tried to come up with a SQL query, actually based on the link [2]https://www.postgresql.org/docs/current/index-functions.html
you shared. I think we should always have an "equality" strategy (e.g.,
"same", "overlaps", "contains" etc sounds wrong to me).

I could agree that "overlaps", "contains", are not "equal", but not sure about
the "same". Around here we must discuss, but not now.

And, it seems btree, hash and brin supports "equal". So, a query like the
following seems to provide the list of (index type, strategy_number,
data_type) that we might be allowed to use.

Note that strategy numbers listed in the doc are just an example - Other than BTree
and Hash do not have a fixed set of strategies at all.
E.g., operator classes for Btree, Hash and BRIN (Minmax) has "equal" and the
strategy number is documented. But other user-defined operator classes for BRIN
may have another number, or it does not have equality comparison.

SELECT
am.amname AS index_type,
amop.amoplefttype::regtype,amop.amoprighttype::regtype,
op.oprname AS operator,
amop.amopstrategy AS strategy_number
FROM
pg_amop amop
JOIN
pg_am am ON am.oid = amop.amopmethod
JOIN
pg_operator op ON op.oid = amop.amopopr
WHERE
(am.amname = 'btree' and amop.amopstrategy = 3) OR
(am.amname = 'hash' and amop.amopstrategy = 1) OR
(am.amname = 'brin' and amop.amopstrategy = 3)
ORDER BY
index_type,
strategy_number;

What do you think?

Good SQL. You have listed the equality operator and related strategy number for given
operator classes.

While analyzing more, however, I found that it might be difficult to support GIN, BRIN,
and bloom indexes in the first version. These indexes does not implement the
"amgettuple" function, which is called in RelationFindReplTupleByIndex()->index_getnext_slot()->index_getnext_tid().
For example, in the brinhandler():

```
/*
* BRIN handler function: return IndexAmRoutine with access method parameters
* and callbacks.
*/
Datum
brinhandler(PG_FUNCTION_ARGS)
{
...
amroutine->amgettuple = NULL;
amroutine->amgetbitmap = bringetbitmap;
...
```

According to the document [2]https://www.postgresql.org/docs/current/index-functions.html, all of index access methods must implement either
of amgettuple or amgetbitmap API. "amgettuple" is used when the table is scaned
and tuples are fetched one by one, RelationFindReplTupleByIndex() do that.
"amgetbitmap" is used when all tuples are fetched at once and RelationFindReplTupleByIndex()
does not support such indexes. To do that the implemented API must be checked and
the function must be changed depends on that. It may be difficult to add them in
the first step so that I want not to support them. Fortunately, Hash, GiST, and
SP-GiST has implemented then so we can focus on them.
In the next patch I will add the mechanism for rejecting such indexes.

Anyway, thank you for keeping the interest to the patch, nevertheless it is difficult theme.

[1]: /messages/by-id/CAHut+PsFdTZJ7DG1jyu7BpA_1d4hwEd-Q+mQAPWcj1ZLD_X5Dw@mail.gmail.com
[2]: https://www.postgresql.org/docs/current/index-functions.html

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Peter,

Thanks for reviewing. PSA new version.
I planned to post new version after supporting more indexes, but it may take more time.
So I want to address comments from you once.

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

According to atop comment in the syscache.c, it contains routines which access
system catalog cache. get_equal_strategy_number() does not check it, so I don't
think it should be at the file.

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

Changed.

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

Changed to "am".

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.

Changed to "eq_strategy".

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;

Added. How do you think?

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

Modified to "class".

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"?

After considering more, I thought we do not have to mention data.
So removed the part " within the range 0-9 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.

Clarified two rows were deleted.

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.

Right. I used meson build system and it said OK. Added.

Furthermore, I added a check to reject indexes which do not implement "amgettuple" API.
More detail, please see [1]/messages/by-id/TYAPR01MB5866E02638D40C4D198334B4F52DA@TYAPR01MB5866.jpnprd01.prod.outlook.com.

[1]: /messages/by-id/TYAPR01MB5866E02638D40C4D198334B4F52DA@TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Jul 7, 2023 at 1:31 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Thank you for your analysis!

Yes, I agree, it is (and was before my patch as well) un-documented limitation of REPLICA IDENTITY FULL.
And, as far as I can see, my patch actually didn't have any impact on the limitation. The unsupported
cases are still unsupported, but now the same error is thrown in a slightly different place.
I think that is a minor limitation, but maybe should be listed [1]?

+1.

Yes, your modification did not touch the restriction. It has existed before the
commit. I (or my colleague) will post the patch to add the description, maybe
after [1] is committed.

I don't think there is any dependency between the two. So, feel free
to post the patch separately.

--
With Regards,
Amit Kapila.

Dear Amit,

Yes, I agree, it is (and was before my patch as well) un-documented

limitation of REPLICA IDENTITY FULL.

And, as far as I can see, my patch actually didn't have any impact on the

limitation. The unsupported

cases are still unsupported, but now the same error is thrown in a slightly

different place.

I think that is a minor limitation, but maybe should be listed [1]?

+1.

Yes, your modification did not touch the restriction. It has existed before the
commit. I (or my colleague) will post the patch to add the description, maybe
after [1] is committed.

I don't think there is any dependency between the two. So, feel free
to post the patch separately.

Okay, thank you for your confirmation. I have started the fork thread [1]/messages/by-id/TYAPR01MB58662174ED62648E0D611194F530A@TYAPR01MB5866.jpnprd01.prod.outlook.com.

[1]: /messages/by-id/TYAPR01MB58662174ED62648E0D611194F530A@TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, Jun 26, 2023 at 7:14 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Thank you for giving comments! The author's comment is quite helpful for us.

When I last dealt with the same issue, I was examining it from a slightly broader perspective. I think
my conclusion was that RelationFindReplTupleByIndex() is designed for the constraints of UNIQUE INDEX
and Primary Key.

I see. IIUC that's why you have added tuples_equal() in the RelationFindReplTupleByIndex().

I think we should also be mindful about tuples_equal() function. When an index returns more than
one tuple, we rely on tuples_equal() function to make sure non-relevant tuples are skipped.

For btree indexes, it was safe to rely on that function as the columns that are indexed using btree
always have equality operator. I think we can safely assume the same for hash indexes.

However, say we indexed "point" type using "gist" index. Then, if we let this logic to kick in,
I think tuples_equal() would fail saying that there is no equality operator exists.

Good point. Actually I have tested for "point" datatype but it have not worked well.
Now I understand the reason.
It seemed that when TYPECACHE_EQ_OPR_FINFO is reuqesed to lookup_type_cache(),
it could return valid value only if the datatype has operator class for Btree or Hash.
So tuples_equal() might not be able to use if tuples have point box circle types.

I also think so. If this is true, how can we think of supporting
indexes other than hash like GiST, and SP-GiST as mentioned by you in
your latest email? As per my understanding if we don't have PK or
replica identity then after the index scan, we do tuples_equal which
will fail for GIST or SP-GIST. Am, I missing something?

--
With Regards,
Amit Kapila.

Hi,

I also think so. If this is true, how can we think of supporting

indexes other than hash like GiST, and SP-GiST as mentioned by you in
your latest email? As per my understanding if we don't have PK or
replica identity then after the index scan, we do tuples_equal which
will fail for GIST or SP-GIST. Am, I missing something?

I also don't think we can support anything other than btree, hash and brin
as those lack equality operators.

And, for BRIN, Hayato brought up the amgettuple issue, which is fair. So,
overall, as far as I can see, we can
easily add hash indexes but all others are either very hard to add or not
possible.

I think if someone one day works on supporting primary keys using other
index types, we can use it here :p

Thanks,
Onder

On Mon, Jul 10, 2023 at 7:43 PM Önder Kalacı <onderkalaci@gmail.com> wrote:

I also think so. If this is true, how can we think of supporting
indexes other than hash like GiST, and SP-GiST as mentioned by you in
your latest email? As per my understanding if we don't have PK or
replica identity then after the index scan, we do tuples_equal which
will fail for GIST or SP-GIST. Am, I missing something?

I also don't think we can support anything other than btree, hash and brin as those lack equality operators.

And, for BRIN, Hayato brought up the amgettuple issue, which is fair. So, overall, as far as I can see, we can
easily add hash indexes but all others are either very hard to add or not possible.

Agreed. So, let's update the patch with comments indicating the
challenges for supporting the other index types than btree and hash.

--
With Regards,
Amit Kapila.

Dear Amit, Önder

Thanks for giving discussions. IIUC all have agreed that the patch focus on extending
to Hash index. PSA the patch for that.
The basic workflow was not so changed, some comments were updated.

Regarding the test code, I think it should be combined into 032_subscribe_use_index.pl
because they have tested same feature. I have just copied tests to latter
part of 032.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Hi, here are some review comments for the v3 patch

======
Commit message

1.
89e46d allowed using indexes other than PRIMARY KEY or REPLICA IDENTITY on the
subscriber, but only the BTree index could be used. This commit extends the
limitation, now the Hash index can be also used.

~

Before giving details about the problems of the other index types it
might be good to summarize why these 2 types are OK.

SUGGESTION
These 2 types of indexes are the only ones supported because only these
- use fix strategy numbers
- implement the "equality" strategy
- implement the function amgettuple()

~~~

2.
I'm not sure why the next paragraphs are numbered 1) and 2). Is that
necessary? It seems maybe a cut/paste hangover from the similar code
comment.

~~~

3.
1) Other indexes do not have a fixed set of strategy numbers at all. In
build_replindex_scan_key(), the operator which corresponds to
"equality comparison"
is searched by using the strategy number, but it is difficult for such indexes.
For example, GiST index operator classes for two-dimensional geometric
objects have
a comparison "same", but its strategy number is different from the
gist_int4_ops,
which is a GiST index operator class that implements the B-tree equivalent.

~

Don't need to say "at all"

~~~

4.
2) Some other indexes do not implement "amgettuple".
RelationFindReplTupleByIndex()
assumes that tuples could be fetched one by one via
index_getnext_slot(), but such
indexes are not supported.

~

4a.
"Some other indexes..." --> Maybe give an example here (e.g. XXX, YYY)
of indexes that do not implement the function.

~

4b.
BEFORE
RelationFindReplTupleByIndex() assumes that tuples could be fetched
one by one via index_getnext_slot(), but such indexes are not
supported.

AFTER
RelationFindReplTupleByIndex() assumes that tuples will be fetched one
by one via index_getnext_slot(), but this currently requires the
"amgetuple" function.

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

5.
+ * 2) Some other indexes do not implement "amgettuple".
+ * RelationFindReplTupleByIndex() assumes that tuples could be fetched one by
+ * one via index_getnext_slot(), but such indexes are not supported. To make it
+ * use index_getbitmap() must be used, but not done yet because of the above
+ * reason.
+ */
+int
+get_equal_strategy_number_for_am(Oid am)

~

Change this text to better match exactly in the commit message (see
previous review comments above). Also I am not sure it is necessary to
say how it *might* be implemented in the future if somebody wanted to
enhance it to work also for "amgetbitmap" function. E.g. do we need
that sentence "To make it..."

~~~

6. get_equal_strategy_number_for_am

+ case GIST_AM_OID:
+ case SPGIST_AM_OID:
+ case GIN_AM_OID:
+ case BRIN_AM_OID:
+ default:

I am not sure it is necessary to spell out all these not-supported
cases in the switch. If seems sufficient just to say 'default:'
doesn't it?

~~~

7. get_equal_strategy_number

Two blank lines are following this function

~~~

8. build_replindex_scan_key

- * This is not generic routine, it expects the idxrel to be a btree,
non-partial
- * and have at least one column reference (i.e. cannot consist of only
- * expressions).
+ * This is not generic routine, it expects the idxrel to be a btree or a hash,
+ * non-partial and have at least one column reference (i.e. cannot consist of
+ * only expressions).

Take care. AFAIK this change will clash with changes Sawawa-san is
making to the same code comment in another thread here [1]Sawada-san doc for RI restriction - /messages/by-id/CAD21AoBzp9H2WV4kDagat2WUOsiYJLo10zJ1E5dZYnRTx1pc_g@mail.gmail.com.

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

9. FindUsableIndexForReplicaIdentityFull

  * Returns the oid of an index that can be used by the apply worker to scan
- * the relation. The index must be btree, non-partial, and have at least
- * one column reference (i.e. cannot consist of only expressions). These
+ * the relation. The index must be btree or hash, non-partial, and have at
+ * least one column reference (i.e. cannot consist of only expressions). These
  * limitations help to keep the index scan similar to PK/RI index scans.

~

Take care. AFAIK this change will clash with changes Sawawa-san is
making to the same code comment in another thread here [1]Sawada-san doc for RI restriction - /messages/by-id/CAD21AoBzp9H2WV4kDagat2WUOsiYJLo10zJ1E5dZYnRTx1pc_g@mail.gmail.com.

~

10.
+ /* Check whether the index is supported or not */
+ is_suitable_type = ((get_equal_strategy_number_for_am(indexInfo->ii_Am)
+ != InvalidStrategy));
+
+ is_partial = (indexInfo->ii_Predicate != NIL);
+ is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+
+ return is_suitable_type && !is_partial && !is_only_on_expression;

I am not sure if the function IsIndexOnlyExpression() even needed
anymore. Isn't it sufficient just to check up-front is the leftmost
INDEX field is a column and that covers this condition also? Actually,
this same question is also open in the Sawada-san thread [1]Sawada-san doc for RI restriction - /messages/by-id/CAD21AoBzp9H2WV4kDagat2WUOsiYJLo10zJ1E5dZYnRTx1pc_g@mail.gmail.com.

======
.../subscription/t/032_subscribe_use_index.pl

11.
AFAIK there is no test to verify that the leftmost index field must be
a column (e.g. cannot be an expression). Yes, there are some tests for
*only* expressions like (expr, expr, expr), but IMO there should be
another test for an index like (expr, expr, column) which should also
fail because the column is not the leftmost field.

------
[1]: Sawada-san doc for RI restriction - /messages/by-id/CAD21AoBzp9H2WV4kDagat2WUOsiYJLo10zJ1E5dZYnRTx1pc_g@mail.gmail.com
/messages/by-id/CAD21AoBzp9H2WV4kDagat2WUOsiYJLo10zJ1E5dZYnRTx1pc_g@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Austalia

Dear Peter,

Thank you for reviewing! PSA new version.

1.
89e46d allowed using indexes other than PRIMARY KEY or REPLICA IDENTITY
on the
subscriber, but only the BTree index could be used. This commit extends the
limitation, now the Hash index can be also used.

~

Before giving details about the problems of the other index types it
might be good to summarize why these 2 types are OK.

SUGGESTION
These 2 types of indexes are the only ones supported because only these
- use fix strategy numbers
- implement the "equality" strategy
- implement the function amgettuple()

Added.

2.
I'm not sure why the next paragraphs are numbered 1) and 2). Is that
necessary? It seems maybe a cut/paste hangover from the similar code
comment.

Yeah, this was just copied from code comments. Numbers were removed.

3.
1) Other indexes do not have a fixed set of strategy numbers at all. In
build_replindex_scan_key(), the operator which corresponds to
"equality comparison"
is searched by using the strategy number, but it is difficult for such indexes.
For example, GiST index operator classes for two-dimensional geometric
objects have
a comparison "same", but its strategy number is different from the
gist_int4_ops,
which is a GiST index operator class that implements the B-tree equivalent.

~

Don't need to say "at all"

Removed.

4.
2) Some other indexes do not implement "amgettuple".
RelationFindReplTupleByIndex()
assumes that tuples could be fetched one by one via
index_getnext_slot(), but such
indexes are not supported.

~

4a.
"Some other indexes..." --> Maybe give an example here (e.g. XXX, YYY)
of indexes that do not implement the function.

I clarified like "BRIN and GIN indexes do not implement...", which are the built-in
indexes. Actually the bloom index cannot be supported due to the same reason, but
I did not mention because it is not in core.

4b.
BEFORE
RelationFindReplTupleByIndex() assumes that tuples could be fetched
one by one via index_getnext_slot(), but such indexes are not
supported.

AFTER
RelationFindReplTupleByIndex() assumes that tuples will be fetched one
by one via index_getnext_slot(), but this currently requires the
"amgetuple" function.

Changed.

src/backend/executor/execReplication.c

5.
+ * 2) Some other indexes do not implement "amgettuple".
+ * RelationFindReplTupleByIndex() assumes that tuples could be fetched one by
+ * one via index_getnext_slot(), but such indexes are not supported. To make it
+ * use index_getbitmap() must be used, but not done yet because of the above
+ * reason.
+ */
+int
+get_equal_strategy_number_for_am(Oid am)

~

Change this text to better match exactly in the commit message (see
previous review comments above).

Copied from commit message.

Also I am not sure it is necessary to
say how it *might* be implemented in the future if somebody wanted to
enhance it to work also for "amgetbitmap" function. E.g. do we need
that sentence "To make it..."

Added, how do you think?

6. get_equal_strategy_number_for_am

+ case GIST_AM_OID:
+ case SPGIST_AM_OID:
+ case GIN_AM_OID:
+ case BRIN_AM_OID:
+ default:

I am not sure it is necessary to spell out all these not-supported
cases in the switch. If seems sufficient just to say 'default:'
doesn't it?

Yeah, it's sufficient. This is a garbage for previous PoC.

7. get_equal_strategy_number

Two blank lines are following this function

Removed.

8. build_replindex_scan_key

- * This is not generic routine, it expects the idxrel to be a btree,
non-partial
- * and have at least one column reference (i.e. cannot consist of only
- * expressions).
+ * This is not generic routine, it expects the idxrel to be a btree or a hash,
+ * non-partial and have at least one column reference (i.e. cannot consist of
+ * only expressions).

Take care. AFAIK this change will clash with changes Sawawa-san is
making to the same code comment in another thread here [1].

Thanks for reminder. I thought that this change seems not needed anymore if the patch
is pushed. But anyway I kept it because this may be pushed first.

src/backend/replication/logical/relation.c

9. FindUsableIndexForReplicaIdentityFull

* Returns the oid of an index that can be used by the apply worker to scan
- * the relation. The index must be btree, non-partial, and have at least
- * one column reference (i.e. cannot consist of only expressions). These
+ * the relation. The index must be btree or hash, non-partial, and have at
+ * least one column reference (i.e. cannot consist of only expressions). These
* limitations help to keep the index scan similar to PK/RI index scans.

~

Take care. AFAIK this change will clash with changes Sawawa-san is
making to the same code comment in another thread here [1].

Thanks for reminder. I thought that this change must be kept even if it will be
pushed. We must check the thread...

10.
+ /* Check whether the index is supported or not */
+ is_suitable_type = ((get_equal_strategy_number_for_am(indexInfo->ii_Am)
+ != InvalidStrategy));
+
+ is_partial = (indexInfo->ii_Predicate != NIL);
+ is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+
+ return is_suitable_type && !is_partial && !is_only_on_expression;

I am not sure if the function IsIndexOnlyExpression() even needed
anymore. Isn't it sufficient just to check up-front is the leftmost
INDEX field is a column and that covers this condition also? Actually,
this same question is also open in the Sawada-san thread [1].

======
.../subscription/t/032_subscribe_use_index.pl

11.
AFAIK there is no test to verify that the leftmost index field must be
a column (e.g. cannot be an expression). Yes, there are some tests for
*only* expressions like (expr, expr, expr), but IMO there should be
another test for an index like (expr, expr, column) which should also
fail because the column is not the leftmost field.

I'm not sure they should be fixed in the patch. You have raised these points
at the Sawada-san's thread[1]/messages/by-id/CAHut+Pv3AgAnP+JTsPseuU-CT+OrSGiqzxqw4oQmWeKLkAea4Q@mail.gmail.com and not sure he has done.
Furthermore, these points are not related with our initial motivation.
Fork, or at least it should be done by another patch.

[1]: /messages/by-id/CAHut+Pv3AgAnP+JTsPseuU-CT+OrSGiqzxqw4oQmWeKLkAea4Q@mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, Jul 12, 2023 at 8:37 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

10.
+ /* Check whether the index is supported or not */
+ is_suitable_type = ((get_equal_strategy_number_for_am(indexInfo->ii_Am)
+ != InvalidStrategy));
+
+ is_partial = (indexInfo->ii_Predicate != NIL);
+ is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+
+ return is_suitable_type && !is_partial && !is_only_on_expression;

I am not sure if the function IsIndexOnlyExpression() even needed
anymore. Isn't it sufficient just to check up-front is the leftmost
INDEX field is a column and that covers this condition also? Actually,
this same question is also open in the Sawada-san thread [1].

======
.../subscription/t/032_subscribe_use_index.pl

11.
AFAIK there is no test to verify that the leftmost index field must be
a column (e.g. cannot be an expression). Yes, there are some tests for
*only* expressions like (expr, expr, expr), but IMO there should be
another test for an index like (expr, expr, column) which should also
fail because the column is not the leftmost field.

I'm not sure they should be fixed in the patch. You have raised these points
at the Sawada-san's thread[1] and not sure he has done.
Furthermore, these points are not related with our initial motivation.
Fork, or at least it should be done by another patch.

I think it is reasonable to discuss the existing code-related
improvements in a separate thread rather than trying to change this
patch. I have some other comments about your patch:

1.
+ * 1) Other indexes do not have a fixed set of strategy numbers.
+ * In build_replindex_scan_key(), the operator which corresponds to "equality
+ * comparison" is searched by using the strategy number, but it is difficult
+ * for such indexes. For example, GiST index operator classes for
+ * two-dimensional geometric objects have a comparison "same", but its strategy
+ * number is different from the gist_int4_ops, which is a GiST index operator
+ * class that implements the B-tree equivalent.
+ *
...
+ */
+int
+get_equal_strategy_number_for_am(Oid am)
...

I think this comment is slightly difficult to understand. Can we make
it a bit generic by saying something like: "The index access methods
other than BTREE and HASH don't have a fixed strategy for equality
operation. Note that in other index access methods, the support
routines of each operator class interpret the strategy numbers
according to the operator class's definition. So, we return
InvalidStrategy number for index access methods other than BTREE and
HASH."

2.
+ * 2) Moreover, BRIN and GIN indexes do not implement "amgettuple".
+ * RelationFindReplTupleByIndex() assumes that tuples will be fetched one by
+ * one via index_getnext_slot(), but this currently requires the "amgetuple"
+ * function. To make it use index_getbitmap() must be used, which fetches all
+ * the tuples at once.
+ */
+int
+get_equal_strategy_number_for_am(Oid am)
{
..

I don't think this is a good place for such a comment. We can probably
move this atop IsIndexUsableForReplicaIdentityFull(). I think you need
to mention two reasons in IsIndexUsableForReplicaIdentityFull() why we
support only BTREE and HASH index access methods (a) Refer to comments
atop get_equal_strategy_number_for_am(); (b) mention the reason
related to tuples_equal() as discussed in email [1]/messages/by-id/CAA4eK1Jv8+8rax-bejd3Ej+T2fG3tuqP8v89byKCBPVQj9C9pg@mail.gmail.com. Then you can say
that we also need to ensure that the index access methods that we
support must have an implementation "amgettuple" as later while
searching tuple via RelationFindReplTupleByIndex, we need the same. We
can probably have an assert for this as well.

[1]: /messages/by-id/CAA4eK1Jv8+8rax-bejd3Ej+T2fG3tuqP8v89byKCBPVQj9C9pg@mail.gmail.com

--
With Regards,
Amit Kapila.

Here are my review comments for the patch v4.

======
General

1.
FYI, this patch also needs some minor PG docs updates [1]https://www.postgresql.org/docs/devel/logical-replication-publication.html because
section "31.1 Publication" currently refers to only btree - e.g.
"Candidate indexes must be btree, non-partial, and have..."

(this may also clash with Sawada-san's other thread as previously mentioned)

======
Commit message

2.
Moreover, BRIN and GIN indexes do not implement "amgettuple".
RelationFindReplTupleByIndex()
assumes that tuples will be fetched one by one via
index_getnext_slot(), but this
currently requires the "amgetuple" function.

~

Typo:
/"amgetuple"/"amgettuple"/

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

3.
+ * 2) Moreover, BRIN and GIN indexes do not implement "amgettuple".
+ * RelationFindReplTupleByIndex() assumes that tuples will be fetched one by
+ * one via index_getnext_slot(), but this currently requires the "amgetuple"
+ * function. To make it use index_getbitmap() must be used, which fetches all
+ * the tuples at once.
+ */
+int

3a.
Typo:
/"amgetuple"/"amgettuple"/

~

3b.
I think my previous comment about this comment was misunderstood. I
was questioning why that last sentence ("To make it...") about
"index_getbitmap()" is even needed in this patch. I thought it may be
overreach to describe details how to make some future enhancement that
might never happen.

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

4. IsIndexUsableForReplicaIdentityFull

 IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo)
 {
- bool is_btree = (indexInfo->ii_Am == BTREE_AM_OID);
- bool is_partial = (indexInfo->ii_Predicate != NIL);
- bool is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+ bool is_suitable_type;
+ bool is_partial;
+ bool is_only_on_expression;

- return is_btree && !is_partial && !is_only_on_expression;
+ /* Check whether the index is supported or not */
+ is_suitable_type = ((get_equal_strategy_number_for_am(indexInfo->ii_Am)
+ != InvalidStrategy));
+
+ is_partial = (indexInfo->ii_Predicate != NIL);
+ is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+
+ return is_suitable_type && !is_partial && !is_only_on_expression;

4a.
The code can be written more optimally, because if it is deemed
already that 'is_suitable_type' is false, then there is no point to
continue to do the other checks and function calls.

~~~

4b.

+ /* Check whether the index is supported or not */
+ is_suitable_type = ((get_equal_strategy_number_for_am(indexInfo->ii_Am)
+ != InvalidStrategy));

The above statement has an extra set of nested parentheses for some reason.

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

5.
/*
* get_opclass_method
*
* Returns the OID of the index access method operator class is for.
*/
Oid
get_opclass_method(Oid opclass)

IMO the comment should be worded more like the previous one in this source file.

SUGGESTION
Returns the OID of the index access method the opclass belongs to.

------
[1]: https://www.postgresql.org/docs/devel/logical-replication-publication.html

Kind Regards,
Peter Smith.
Fujitsu Australia

Dear Peter,

Thanks for giving comment.

1.
FYI, this patch also needs some minor PG docs updates [1] because
section "31.1 Publication" currently refers to only btree - e.g.
"Candidate indexes must be btree, non-partial, and have..."

(this may also clash with Sawada-san's other thread as previously mentioned)

Fixed that, but I could not find any other points. Do you have in mind others?
I checked related commits like 89e46d and adedf5, but only the part was changed.

Commit message

2.
Moreover, BRIN and GIN indexes do not implement "amgettuple".
RelationFindReplTupleByIndex()
assumes that tuples will be fetched one by one via
index_getnext_slot(), but this
currently requires the "amgetuple" function.

~

Typo:
/"amgetuple"/"amgettuple"/

Fixed.

src/backend/executor/execReplication.c

3.
+ * 2) Moreover, BRIN and GIN indexes do not implement "amgettuple".
+ * RelationFindReplTupleByIndex() assumes that tuples will be fetched one by
+ * one via index_getnext_slot(), but this currently requires the "amgetuple"
+ * function. To make it use index_getbitmap() must be used, which fetches all
+ * the tuples at once.
+ */
+int

3a.
Typo:
/"amgetuple"/"amgettuple"/

Per suggestion from Amit [1]/messages/by-id/CAA4eK1++R3WSfsGH0yFR9WEbkDfF__OccADR7qXDnHGTww+kvQ@mail.gmail.com, the paragraph was removed.

3b.
I think my previous comment about this comment was misunderstood. I
was questioning why that last sentence ("To make it...") about
"index_getbitmap()" is even needed in this patch. I thought it may be
overreach to describe details how to make some future enhancement that
might never happen.

Removed.

src/backend/replication/logical/relation.c

4. IsIndexUsableForReplicaIdentityFull

IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo)
{
- bool is_btree = (indexInfo->ii_Am == BTREE_AM_OID);
- bool is_partial = (indexInfo->ii_Predicate != NIL);
- bool is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+ bool is_suitable_type;
+ bool is_partial;
+ bool is_only_on_expression;

- return is_btree && !is_partial && !is_only_on_expression;
+ /* Check whether the index is supported or not */
+ is_suitable_type = ((get_equal_strategy_number_for_am(indexInfo->ii_Am)
+ != InvalidStrategy));
+
+ is_partial = (indexInfo->ii_Predicate != NIL);
+ is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+
+ return is_suitable_type && !is_partial && !is_only_on_expression;

4a.
The code can be written more optimally, because if it is deemed
already that 'is_suitable_type' is false, then there is no point to
continue to do the other checks and function calls.

Right, is_suitable_type is now removed.

4b.

+ /* Check whether the index is supported or not */
+ is_suitable_type = ((get_equal_strategy_number_for_am(indexInfo->ii_Am)
+ != InvalidStrategy));

The above statement has an extra set of nested parentheses for some reason.

This part was removed per above comment.

src/backend/utils/cache/lsyscache.c

5.
/*
* get_opclass_method
*
* Returns the OID of the index access method operator class is for.
*/
Oid
get_opclass_method(Oid opclass)

IMO the comment should be worded more like the previous one in this source file.

SUGGESTION
Returns the OID of the index access method the opclass belongs to.

Fixed.

[1]: /messages/by-id/CAA4eK1++R3WSfsGH0yFR9WEbkDfF__OccADR7qXDnHGTww+kvQ@mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED