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

From: Andres Freund <[email-masked]>
Date: Mon, 9 Jan 2023 12:03:47 -0800

Hi,

On 2023-01-07 13:50:04 -0500, Tom Lane wrote:
> I think we should either live within the constraints set by this
> overarching design, or else nuke execReplication.c from orbit and
> start using the real planner and executor.  Perhaps the foreign
> key enforcement mechanisms could be a model --- although if you
> don't want to buy into using SPI as well, you probably should look
> at Amit L's work at [1].

I don't think using the full executor for every change is feasible from an
overhead perspective. But it might make sense to bail out to using the full
executor in a bunch of non-fastpath paths.

I think this is basically similar to COPY not using the full executor.

But that doesn't mean that all of this has to be open coded in
execReplication.c. Abstracting pieces so that COPY, logical rep and perhaps
even nodeModifyTable.c can share code makes sense.

> Also ... maybe I am missing something, but is REPLICA IDENTITY FULL
> sanely defined in the first place?  It looks to me that
> RelationFindReplTupleSeq assumes without proof that there is a unique
> full-tuple match, but that is only reasonable to assume if there is at
> least one unique index (and maybe not even then, if nulls are involved).

If the table definition match between publisher and standby, it doesn't matter
which tuple is updated, if all columns are used to match. Since there's
nothing distinguishing two rows with all columns being equal, it doesn't
matter which we update.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Mon, 09 Jan 2023 15:37:10 -0500

> nothing distinguishing two rows with all columns being equal, it doesn't

Yeah, but the point here is precisely that they might *not* match;
for example there could be extra columns in the subscriber's table.
This may be largely a documentation problem, though --- I think my
beef is mainly that there's nothing in our docs explaining the
semantic pitfalls of FULL, we only say "it's slow".

Anyway, to get back to the point at hand: if we do have a REPLICA IDENTITY
FULL situation then we can make use of any unique index over a subset of
the transmitted columns, and if there's more than one candidate index
it's unlikely to matter which one we pick.  Given your comment I guess
we have to also compare the non-indexed columns, so we can't completely
convert the FULL case to the straight index case.  But still it doesn't
seem to me to be appropriate to use the planner to find a suitable index.

			regards, tom lane

---

From: Marco Slot <[email-masked]>
Date: Fri, 20 Jan 2023 15:35:51 +0300

> seem to me to be appropriate to use the planner to find a suitable index.

The main purpose of REPLICA IDENTITY FULL seems to be to enable logical
replication for tables that may have duplicates and therefore cannot have a
unique index that can be used as a replica identity.

For those tables the user currently needs to choose between update/delete
erroring (bad) or doing a sequential scan on the apply side per
updated/deleted tuple (often worse). This issue currently prevents a lot of
automation around logical replication, because users need to decide whether
and when they are willing to accept partial downtime. The current REPLICA
IDENTITY FULL implementation can work in some cases, but applying the
effects of an update that affected a million rows through a million
sequential scans will certainly not end well.

This patch solves the problem by allowing the apply side to pick a
non-unique index to find any matching tuple instead of always using a
sequential scan, but that either requires some planning/costing logic to
avoid picking a lousy index, or allowing the user to manually preselect the
index to use, which is less convenient.

An alternative might be to construct prepared statements and using the
regular planner. If applied uniformly that would also be nice from the
extensibility point-of-view, since there is currently no way for an
extension to augment the apply side. However, I assume the current approach
of using low-level functions in the common case was chosen for performance
reasons.

I suppose the options are:
1. use regular planner uniformly
2. use regular planner only when there's no replica identity (or
configurable?)
3. only use low-level functions
4. keep using sequential scans for every single updated row
5. introduce a hidden logical row identifier in the heap that is guaranteed
unique within a table and can be used as a replica identity when no unique
index exists

Any thoughts?

cheers,
Marco

---

From: Önder Kalacı <[email-masked]>
Date: Fri, 27 Jan 2023 16:02:13 +0300

Hi Marco, Tom,

> But still it doesn't seem to me to be appropriate to use the planner to
find a suitable index.

As Marco noted, here we are trying to pick an index that is non-unique. We
could pick the index based on information extracted from pg_index (or
such), but then, it'd be a premature selection. Before sending the patch to
pgsql-hackers, I initially tried to find a suitable one with such an
approach.

But then, I still ended up using costing functions (and some other low
level functions). Overall, it felt like the planner is the module that
makes this decision best. Why would we try to invent another immature way
of doing this? With that reasoning, I ended up using the related planner
functions directly.

However, I assume the current approach of using low-level functions in the
> common case was chosen for performance reasons.
>

That's partially the reason. If you look at the patch, we use the planner
(or the low level functions) infrequently. It is only called when the
logical replication relation cache is rebuilt. As far as I can see, that
happens with (auto) ANALYZE or DDLs etc. I expect these are infrequent
operations. Still, I wanted to make sure we do not create too much overhead
even if there are frequent invalidations.

The main reason for using the low level functions over the planner itself
is to have some more control over the decision. For example, due to the
execution limitations, we currently cannot allow an index that consists of
only expressions (similar to pkey restriction). With the current approach,
we can easily filter those out.

Also, another minor reason is that, if we use planner, we'd get a
PlannedStmt back. It also felt weird to check back the index used from a
PlannedStmt. In the current patch, we iterate over Paths, which seems more
intuitive to me.

> 5. introduce a hidden logical row identifier in the heap that is
> guaranteed unique within a table and can be used as a replica identity when
> no unique index exists
>

One other option I considered was to ask the index explicitly on the
subscriber side from the user when REPLICA IDENTITY is FULL. But, it is a
pretty hard choice for any user, even a planner sometimes fails to pick the
right index :)  Also, it is probably controversial to change any of the
APIs for this purpose?

I'd be happy to hear from more experienced hackers on the trade-offs for
the above, and I'd be open to work on that if there is a clear winner. For
me (3) is a decent solution for the problem.

Thanks,
Onder

---

From: Amit Kapila <[email-masked]>
Date: Mon, 30 Jan 2023 18:46:07 +0530

On Fri, Jan 27, 2023 at 6:32 PM Önder Kalacı <[email-masked]> wrote:
>> 2. use regular planner only when there's no replica identity (or configurable?)
>> 5. introduce a hidden logical row identifier in the heap that is guaranteed unique within a table and can be used as a replica identity when no unique index exists
>
>
> One other option I considered was to ask the index explicitly on the subscriber side from the user when REPLICA IDENTITY is FULL. But, it is a pretty hard choice for any user, even a planner sometimes fails to pick the right index :)  Also, it is probably controversial to change any of the APIs for this purpose?
>

I agree that it won't be a very convenient option for the user but how
about along with asking for an index from the user (when the user
didn't provide an index), we also allow to make use of any unique
index over a subset of the transmitted columns, and if there's more
than one candidate index pick any one. Additionally, we can allow
disabling the use of an index scan for this particular case. If we are
too worried about API change for allowing users to specify the index
then we can do that later or as a separate patch.

> I'd be happy to hear from more experienced hackers on the trade-offs for the above, and I'd be open to work on that if there is a clear winner. For me (3) is a decent solution for the problem.
>

>From the discussion above it is not very clear that adding maintenance
costs in this area is worth it even though that can give better
results as far as this feature is concerned.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Thu, 2 Feb 2023 11:33:36 +0300

Hi all,

Thanks for the feedback!

> index over a subset of the transmitted columns,

Tbh, I cannot follow why you would use REPLICA IDENTITY FULL if you can
already
create a unique index? Aren't you supposed to use REPLICA IDENTITY .. USING
INDEX
in that case (if not simply pkey)?

That seems like a potential expansion of this patch, but I don't consider
it as essential. Given it
is hard to get even small commits in, I'd rather wait to see what you think
before doing such
a change.

> and if there's more
On v23, I dropped the planner support for picking the index. Instead, it
simply
iterates over the indexes and picks the first one that is suitable.

I'm currently thinking on how to enable users to override this decision.
One option I'm leaning towards is to add a syntax like the following:

*ALTER SUBSCRIPTION .. ALTER TABLE ... SET INDEX ...*

Though, that should probably be a seperate patch. I'm going to work
on that, but still wanted to share v23 given picking the index sounds
complementary, not strictly required at this point.

Thanks,
Onder

---

From: Amit Kapila <[email-masked]>
Date: Sat, 4 Feb 2023 16:54:10 +0530

On Thu, Feb 2, 2023 at 2:03 PM Önder Kalacı <[email-masked]> wrote:
> On v23, I dropped the planner support for picking the index. Instead, it simply
> ALTER SUBSCRIPTION .. ALTER TABLE ... SET INDEX ...
>

I agree that it could be a separate patch. However, do you think we
need some way to disable picking the index scan? This is to avoid
cases where sequence scan could be better or do we think there won't
exist such a case?

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 13 Feb 2023 11:00:31 +0000

> complementary, not strictly required at this point.

Thanks for your patch. Here are some comments.

1.
I noticed that get_usable_indexoid() is called in apply_handle_update_internal()
and apply_handle_delete_internal() to get the usable index. Could usableIndexOid
be a parameter of these two functions? Because we have got the
LogicalRepRelMapEntry when calling them and if we do so, we can get
usableIndexOid without get_usable_indexoid(). Otherwise for partitioned tables,
logicalrep_partition_open() is called in get_usable_indexoid() and searching
the entry via hash_search() will increase cost.

2.
+			 * This attribute is an expression, and
+			 * SuitableIndexPathsForRepIdentFull() was called earlier when the
+			 * index for subscriber was selected. There, the indexes
+			 * comprising *only* expressions have already been eliminated.

The comment looks need to be updated:
SuitableIndexPathsForRepIdentFull
->
FindUsableIndexForReplicaIdentityFull

3.

 	/* Build scankey for every attribute in the index. */
-	for (attoff = 0; attoff < IndexRelationGetNumberOfKeyAttributes(idxrel); attoff++)
+	for (index_attoff = 0; index_attoff < IndexRelationGetNumberOfKeyAttributes(idxrel);
+		 index_attoff++)
 	{

Should the comment be changed? Because we skip the attributes that are expressions.

4.
+			Assert(RelationGetReplicaIndex(rel) != RelationGetRelid(idxrel) &&
+				   RelationGetPrimaryKeyIndex(rel) != RelationGetRelid(idxrel));

Maybe we can call the new function idxIsRelationIdentityOrPK()?

Regards,
Shi Yu

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 14 Feb 2023 09:35:58 +0000

Hi,

Here are some comments on the test cases.

1. in test case "SUBSCRIPTION RE-CALCULATES INDEX AFTER CREATE/DROP INDEX"
+# now, ingest more data and create index on column y which has higher cardinality
+# so that the future commands use the index on column y
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO test_replica_id_full SELECT 50, i FROM generate_series(0,3100)i;");
+$node_subscriber->safe_psql('postgres',
+	"CREATE INDEX test_replica_id_full_idy ON test_replica_id_full(y)");

We don't pick the cheapest index in the current patch, so should we modify this
part of the test?

BTW, the following comment in FindLogicalRepUsableIndex() need to be changed,
too.

+		 * We are looking for one more opportunity for using an index. If
+		 * there are any indexes defined on the local relation, try to pick
+		 * the cheapest index.

2. Is there any reasons why we need the test case "SUBSCRIPTION USES INDEX WITH
DROPPED COLUMNS"? Has there been a problem related to dropped columns before?

3. in test case "SUBSCRIPTION USES INDEX ON PARTITIONED TABLES"
+# deletes rows and moves between partitions
+$node_publisher->safe_psql('postgres',
+	"DELETE FROM users_table_part WHERE user_id = 1 and value_1 = 1;");
+$node_publisher->safe_psql('postgres',
+	"DELETE FROM users_table_part WHERE user_id = 12 and value_1 = 12;");

"moves between partitions" in the comment seems wrong.

4. in test case "SUBSCRIPTION DOES NOT USE INDEXES WITH ONLY EXPRESSIONS"
+# update 2 rows
+$node_publisher->safe_psql('postgres',
+	"UPDATE people SET firstname = 'Nan' WHERE firstname = 'first_name_1';");
+$node_publisher->safe_psql('postgres',
+	"UPDATE people SET firstname = 'Nan' WHERE firstname = 'first_name_2' AND lastname = 'last_name_2';");
+
+# make sure the index is not used on the subscriber
+$result = $node_subscriber->safe_psql('postgres',
+	"select idx_scan from pg_stat_all_indexes where indexrelname = 'people_names'");
+is($result, qq(0), 'ensure subscriber tap_sub_rep_full updates two rows via seq. scan with index on expressions');
+

I think it would be better to call wait_for_catchup() before the check because
we want to check the index is NOT used. Otherwise the check may pass because the
rows have not yet been updated on subscriber.

5. in test case "SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN"
+# show that index is not used even when enable_indexscan=false
+$result = $node_subscriber->safe_psql('postgres',
+	"select idx_scan from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx'");
+is($result, qq(0), 'ensure subscriber has not used index with enable_indexscan=false');

Should we remove the word "even" in the comment?

6. 
In each test case we re-create publications, subscriptions, and tables. Could we
create only one publication and one subscription at the beginning, and use them
in all test cases? I think this can save some time running the test file.

Regards,
Shi Yu

---

From: Peter Smith <[email-masked]>
Date: Wed, 15 Feb 2023 13:16:31 +1100

Here are some review comments for patch v23.

======
General

1.
IIUC the previous logic for checking "cost" comparisons and selecting
the "cheapest" strategy is no longer present in the latest patch.

In that case, I think there are some leftover stale comments that need
changing. For example,

1a. Commit message:
"let the planner sub-modules compare the costs of index versus
sequential scan and choose the cheapest."

~

1b. Commit message:
"Finally, pick the cheapest `Path` among."

~

1c. FindLogicalRepUsableIndex function:
+ * We are looking for one more opportunity for using an index. If
+ * there are any indexes defined on the local relation, try to pick
+ * the cheapest index.

======
doc/src/sgml/logical-replication.sgml

If replica identity "full" is used, indexes can be used on the
subscriber side for seaching the rows. The index should be btree,
non-partial and have at least one column reference (e.g., should not
consists of only expressions). If there are no suitable indexes, the
search on the subscriber side is very inefficient and should only be
used as a fallback if no other solution is possible

2a.
Fixed typo "seaching", and minor rewording.

SUGGESTION
When replica identity "full" is specified, indexes can be used on the
subscriber side for searching the rows. These indexes should be btree,
non-partial and have at least one column reference (e.g., should not
consist of only expressions). If there are no such suitable indexes,
the search on the subscriber side can be very inefficient, therefore
replica identity "full" should only be used as a fallback if no other
solution is possible.

~

2b.
I know you are just following some existing text here, but IMO this
should probably refer to replica identity <literal>FULL</literal>
instead of "full".

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

3. IdxIsRelationIdentityOrPK

+/*
+ * Given a relation and OID of an index, returns true if
+ * the index is relation's primary key's index or
+ * relaton's replica identity index.
+ *
+ * Returns false otherwise.
+ */
+static bool
+IdxIsRelationIdentityOrPK(Relation rel, Oid idxoid)
+{
+ Assert(OidIsValid(idxoid));
+
+ if (RelationGetReplicaIndex(rel) == idxoid ||
+ RelationGetPrimaryKeyIndex(rel) == idxoid)
+ return true;
+
+ return false;

3a.
Comment typo "relaton"

~

3b.
Code could be written using single like below if you wish (but see #2c)

return RelationGetReplicaIndex(rel) == idxoid ||
RelationGetPrimaryKeyIndex(rel) == idxoid;

~

3c.
Actually, RelationGetReplicaIndex and RelationGetPrimaryKeyIndex
implementations are very similar so it seemed inefficient to be
calling both of them. IMO it might be better to just make a new
relcache function IdxIsRelationIdentityOrPK(Relation rel, Oid idxoid).
This implementation will be similar to those others, but now you need
only to call the workhorse RelationGetIndexList *one* time.

~~~

4. RelationFindReplTupleByIndex

  bool found;
+ TypeCacheEntry **eq = NULL; /* only used when the index is not repl. ident
+ * or pkey */
+ bool idxIsRelationIdentityOrPK;

If you change the comment to say "RI" instead of "repl. Ident" then it
can all fit on one line, which would be an improvement.

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

5.
 #include "replication/logicalrelation.h"
 #include "replication/worker_internal.h"
+#include "optimizer/cost.h"
 #include "utils/inval.h"

Can that #include be added in alphabetical order like the others or not?

~~~

6. logicalrep_partition_open

+ /*
+ * Finding a usable index is an infrequent task. It occurs when an
+ * operation is first performed on the relation, or after invalidation of
+ * of the relation cache entry (such as ANALYZE or CREATE/DROP index on
+ * the relation).
+ */
+ entry->usableIndexOid = FindLogicalRepUsableIndex(partrel, remoterel);
+

Typo "of of the relation"

~~~

7. FindUsableIndexForReplicaIdentityFull

+static Oid
+FindUsableIndexForReplicaIdentityFull(Relation localrel)
+{
+ MemoryContext usableIndexContext;
+ MemoryContext oldctx;
+ Oid usableIndex;
+ Oid idxoid;
+ List *indexlist;
+ ListCell   *lc;
+ Relation        indexRelation;
+ IndexInfo  *indexInfo;
+ bool is_btree;
+ bool is_partial;
+ bool is_only_on_expression;

It looks like some of these variables are only used within the scope
of the foreach loop, so I think that is where they should be declared.

~~~

8.
+ usableIndex = InvalidOid;

Might as well do that assignment at the declaration.

~~~

9. FindLogicalRepUsableIndex

+ /*
+ * Simple case, we already have a primary key or a replica identity index.
+ *
+ * Note that we do not use index scans below when enable_indexscan is
+ * false. Allowing primary key or replica identity even when index scan is
+ * disabled is the legacy behaviour. So we hesitate to move the below
+ * enable_indexscan check to be done earlier in this function.
+ */
+ idxoid = GetRelationIdentityOrPK(localrel);
+ if (OidIsValid(idxoid))
+ return idxoid;
+
+ /* If index scans are disabled, use a sequential scan */
+ if (!enable_indexscan)
+ return InvalidOid;

~

IMO that "Note" really belongs with the if (!enable)indexscan) more like this:

SUGGESTION
/*
* Simple case, we already have a primary key or a replica identity index.
*/
idxoid = GetRelationIdentityOrPK(localrel);
if (OidIsValid(idxoid))
return idxoid;

/*
* If index scans are disabled, use a sequential scan.
*
* Note we hesitate to move this check to earlier in this function
* because allowing primary key or replica identity even when index scan
* is disabled is the legacy behaviour.
*/
if (!enable_indexscan)
return InvalidOid;

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

10. get_usable_indexoid

+/*
+ * Decide whether we can pick an index for the relinfo (e.g., the relation)
+ * we're actually deleting/updating from. If it is a child partition of
+ * edata->targetRelInfo, find the index on the partition.
+ *
+ * Note that if the corresponding relmapentry has invalid usableIndexOid,
+ * the function returns InvalidOid.
+ */

"(e.g., the relation)" --> "(i.e. the relation)"

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

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 15 Feb 2023 03:53:34 +0000

> cases where sequence scan could be better or do we think there won't

I think such a case exists. I tried the following cases based on v23 patch.

# Step 1.
Create publication, subscription and tables.
-- on publisher
create table tbl (a int);
alter table tbl replica identity full;
create publication pub for table tbl;

-- on subscriber
create table tbl (a int);
create index idx_a on tbl(a);
create subscription sub connection 'dbname=postgres port=5432' publication pub;

# Step 2.
Setup synchronous replication.

# Step 3.
Execute SQL query on publisher.

-- case 1 (All values are duplicated)
truncate tbl;
insert into tbl select 1 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 2
truncate tbl;
insert into tbl select i%3 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 3
truncate tbl;
insert into tbl select i%5 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 4
truncate tbl;
insert into tbl select i%10 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 5
truncate tbl;
insert into tbl select i%100 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 6
truncate tbl;
insert into tbl select i%1000 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 7 (No duplicate value)
truncate tbl;
insert into tbl select i from generate_series(0,10000)i;
update tbl set a=a+1;

# Result
The time executing update (the average of 3 runs is taken, the unit is
milliseconds):

+--------+---------+---------+
|        | patched |  master |
+--------+---------+---------+
| case 1 | 3933.68 | 1298.32 |
| case 2 | 1803.46 | 1294.42 |
| case 3 | 1380.82 | 1299.90 |
| case 4 | 1042.60 | 1300.20 |
| case 5 |  691.69 | 1297.51 |
| case 6 |  578.50 | 1300.69 |
| case 7 |  566.45 | 1302.17 |
+--------+---------+---------+

In case 1~3, there's an overhead after applying the patch. In other cases, the
patch improved the performance. As more duplicate values, the greater the
overhead after applying the patch.

Regards,
Shi Yu

---

From: Amit Kapila <[email-masked]>
Date: Wed, 15 Feb 2023 10:07:02 +0530

> I think such a case exists. I tried the following cases based on v23 patch.
...
> The time executing update (the average of 3 runs is taken, the unit is

I think this overhead seems to be mostly due to the need to perform
tuples_equal multiple times for duplicate values. I don't know if
there is any simple way to avoid this without using the planner stuff
as was used in the previous approach. So, this brings us to the
question of whether just providing a way to disable/enable the use of
index scan for such cases is sufficient or if we need any other way.

Tom, Andres, or others, do you have any suggestions on how to move
forward with this patch?

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Fri, 17 Feb 2023 17:57:28 +1100

FYI, I accidentally left this (v23) patch's TAP test
t/032_subscribe_use_index.pl still lurking even after removing all
other parts of this patch.

In this scenario, the t/032 test gets stuck (build of latest HEAD)

IIUC the patch is only meant to affect performance, so I expected this
032 test to work regardless of whether the rest of the patch is
applied.

Anyway,  it hangs every time for me. I didn't dig looking for the
cause, but if it requires patched code for this new test to pass, I
thought it indicates something wrong either with the test or something
more sinister the new test has exposed. Maybe I am mistaken

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

---

From: Önder Kalacı <[email-masked]>
Date: Tue, 21 Feb 2023 17:25:10 +0300

Hi Amit, all

Amit Kapila <[email-masked]>, 15 Şub 2023 Çar, 07:37 tarihinde

> Tom, Andres, or others, do you have any suggestions on how to move
Thanks for the feedback and testing. Due to personal circumstances,
I could not reply the thread in the last 2 weeks, but I'll be more active
going forward.

 I also agree that we should have a way to control the behavior.

I created another patch (v24_0001_optionally_disable_index.patch) which can
be applied
on top of v23_0001_use_index_on_subs_when_pub_rep_ident_full.patch.

The new patch adds a new *subscription_parameter* for both CREATE and ALTER
subscription
named: *enable_index_scan*. The setting is valid only when REPLICA IDENTITY
is full.

What do you think about such a patch to control the behavior? It does not
give a per-relation
level of control, but still useful for many cases.

(Note that I'll be working on the other feedback in the email thread,
wanted to send this earlier
to hear some early thoughts on v24_0001_optionally_disable_index.patch).

---

From: Amit Kapila <[email-masked]>
Date: Sat, 25 Feb 2023 16:00:05 +0530

On Tue, Feb 21, 2023 at 7:55 PM Önder Kalacı <[email-masked]> wrote:
> I created another patch (v24_0001_optionally_disable_index.patch) which can be applied
> The new patch adds a new subscription_parameter for both CREATE and ALTER subscription
> named: enable_index_scan. The setting is valid only when REPLICA IDENTITY is full.
>
> What do you think about such a patch to control the behavior? It does not give a per-relation

Wouldn't a table-level option like 'apply_index_scan' be better than a
subscription-level option with a default value as false? Anyway, the
bigger point is that we don't see a better way to proceed here than to
introduce some option to control this behavior.

I see this as a way to provide this feature for users but I would
prefer to proceed with this if we can get some more buy-in from senior
community members (at least one more committer) and some user(s) if
possible. So, I once again request others to chime in and share their
opinion.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Mon, 27 Feb 2023 10:05:38 +0300

Hi Amit, all

> introduce some option to control this behavior.

What would be a good API for adding such an option for table-level?
To be more specific, I cannot see any table level sub/pub options in the
docs.

My main motivation for doing it for subscription-level is that (a) it might
be
too much work for users to control the behavior if it is table-level (b) I
couldn't
find a good API for table-level, and inventing a new one seemed
like a big change.

Overall, I think it makes sense to disable the feature by default. It is
enabled by default, and that's good for test coverage for now, but
let me disable it when I push a version next time.

> possible. So, I once again request others to chime in and share their
Agreed, it would be great to hear some other perspectives on this.

Thanks,
Onder

---

From: Amit Kapila <[email-masked]>
Date: Mon, 27 Feb 2023 13:35:29 +0530

On Mon, Feb 27, 2023 at 12:35 PM Önder Kalacı <[email-masked]> wrote:
> To be more specific, I cannot see any table level sub/pub options in the docs.
>

I was thinking something along the lines of "Storage Parameters" [1]
for a table. See parameters like autovacuum_enabled that decide the
autovacuum behavior for a table. These can be set via CREATE/ALTER
TABLE commands.

[1] - https://www.postgresql.org/docs/devel/sql-createtable.html#SQL-CREATETABLE-STORAGE-PARAMETERS

-- 
With Regards,
Amit Kapila.