On Thu, Mar 16, 2023 at 2:15 PM Amit Kapila wrote:
>
> On Wed, Mar 15, 2023 at 9:12 AM Amit Kapila wrote:
> >
> > On Tue, Mar 14, 2023 at 3:18 PM Önder Kalacı wrote:
> > >>
> >
> > Pushed this patch but forgot to add a new testfile. Will do that soon.
> >
>
> The main patch is committed now. I
On Wed, Mar 15, 2023 at 9:12 AM Amit Kapila wrote:
>
> On Tue, Mar 14, 2023 at 3:18 PM Önder Kalacı wrote:
> >>
>
> Pushed this patch but forgot to add a new testfile. Will do that soon.
>
The main patch is committed now. I think the pending item in this
thread is to conclude whether we need a
On Tue, Mar 14, 2023 at 3:18 PM Önder Kalacı wrote:
>>
Pushed this patch but forgot to add a new testfile. Will do that soon.
--
With Regards,
Amit Kapila.
>
>
>
> Thanks for the updated patch.
> Few minor comments:
> 1) The extra line break after IsIndexOnlyOnExpression function can be
> removed:
>
removed
>
>
> 2) Generally we don't terminate with "." for single line comments
> +
> + /*
> + * Simple case, we already have a primary key or a
On Tue, 14 Mar 2023 at 14:36, Önder Kalacı wrote:
>
>
> Amit Kapila , 14 Mar 2023 Sal, 11:59 tarihinde şunu
> yazdı:
>>
>> On Tue, Mar 14, 2023 at 12:48 PM Önder Kalacı wrote:
>> >>
>> >> 2.
>> >> +# make sure that the subscriber has the correct data after the update
>> >> UPDATE
>> >>
>> >>
Amit Kapila , 14 Mar 2023 Sal, 11:59 tarihinde
şunu yazdı:
> On Tue, Mar 14, 2023 at 12:48 PM Önder Kalacı
> wrote:
> >>
> >> 2.
> >> +# make sure that the subscriber has the correct data after the update
> UPDATE
> >>
> >> "update UPDATE" seems to be a typo.
> >>
> >
> > thanks, fixed
> >
> >>
On Tue, Mar 14, 2023 at 12:48 PM Önder Kalacı wrote:
>>
>> 2.
>> +# make sure that the subscriber has the correct data after the update UPDATE
>>
>> "update UPDATE" seems to be a typo.
>>
>
> thanks, fixed
>
>>
>> 3.
>> +# now, drop the index with the expression, and re-create index on column
>>
Hi Amit, all
Amit Kapila , 14 Mar 2023 Sal, 09:50 tarihinde
şunu yazdı:
> On Mon, Mar 13, 2023 at 7:46 PM Önder Kalacı
> wrote:
> >
> > Attaching v47.
> >
>
> I have made the following changes in the attached patch (a) removed
> the function IsIdxSafeToSkipDuplicates() and used the check
Hi Shi Yu,
> in RemoteRelContainsLeftMostColumnOnIdx():
>
> + if (indexInfo->ii_NumIndexAttrs < 1)
> + return false;
>
> Did you see any cases that the condition is true? I think there is at
> least one
> column in the index. If so, we can use an Assert().
>
Actually, it was
On Mon, Mar 13, 2023 at 7:46 PM Önder Kalacı wrote:
>
> Attaching v47.
>
I have made the following changes in the attached patch (a) removed
the function IsIdxSafeToSkipDuplicates() and used the check directly
in the caller; (b) changed a few comments in the patch; (c) the test
file was
On Mon, Mar 13, 2023 10:16 PM Önder Kalacı wrote:
>
> Attaching v47.
>
Thanks for updating the patch. Here are some comments.
1.
in RemoteRelContainsLeftMostColumnOnIdx():
+ if (indexInfo->ii_NumIndexAttrs < 1)
+ return false;
Did you see any cases that the condition is
Hi Amit, all
> >
> > For that test, my goal was to ensure/show that the invalidation callback
> > is triggered after `DROP / CREATE INDEX` commands.
> >
>
> Fair point. I suggest in that case just keep one of the tests for Drop
> Index such that after that it will pick up a sequence scan.
On Mon, Mar 13, 2023 at 6:14 PM Önder Kalacı wrote:
>
>>
>>
>> 3. Removed the cases for dropping the index. This ensures that after
>> dropping the index on the table we switch to either an index scan (if
>> a new index is created) or to a sequence scan. It doesn't seem like a
>> very interesting
Hi Amit, Peter, all
> > If the reason for the stats polling was only to know if some index is
> > chosen or not, I was wondering if you can just convey the same
> > information to the TAP test via some conveniently placed (DEBUG?)
> > logging.
> >
>
> I had thought about it but didn't convince
On Mon, Mar 13, 2023 at 2:44 AM Peter Smith wrote:
>
> On Sat, Mar 11, 2023 at 6:05 PM Amit Kapila wrote:
> >
> > On Fri, Mar 10, 2023 at 7:58 PM Önder Kalacı wrote:
> > >
> > >
> > > I think one option could be to drop some cases altogether, but not sure
> > > we'd want that.
> > >
> > > As a
Hi Hou zj, Shi-san, all
> In this function, it used the local column number(keycol) to match the
> remote
> column number(attkeys), I think it will cause problem if the column order
> between pub/sub doesn't match. Like:
>
> ---
> - pub
> CREATE TABLE test_replica_id_full (x int, y int);
>
On Monday, March 13, 2023 2:23 PM Önder Kalacı wrote:
Hi,
> > >
> > >
> > > Reading [1], I think I can follow what you suggest. So, basically,
> > > if the leftmost column is not filtered, we have the following:
> > >
> > >> but the entire index would have to be scanned, so in most cases the
Hi Shi Yu,
> > >
> > >
> > > Reading [1], I think I can follow what you suggest. So, basically,
> > > if the leftmost column is not filtered, we have the following:
> > >
> > >> but the entire index would have to be scanned, so in most cases the
> planner
> > would prefer a sequential table
On Fri, Mar 10, 2023 8:17 PM Amit Kapila wrote:
>
> On Fri, Mar 10, 2023 at 5:16 PM Önder Kalacı wrote:
> >
> >>
> >> wip_for_optimize_index_column_match
> >> +static bool
> >> +IndexContainsAnyRemoteColumn(IndexInfo *indexInfo,
> >> + LogicalRepRelation *remoterel)
> >> +{
> >> + for (int i
On Sat, Mar 11, 2023 at 6:05 PM Amit Kapila wrote:
>
> On Fri, Mar 10, 2023 at 7:58 PM Önder Kalacı wrote:
> >
> >
> > I think one option could be to drop some cases altogether, but not sure
> > we'd want that.
> >
> > As a semi-related question: Are you aware of any setting that'd make
> >
Hi Amit, all
> >> I think we can add such a test (which relies on existing buggy
> >> behavior) later after fixing the existing bug. For now, it would be
> >> better to remove that test and add it after we fix dropped columns
> >> issue in HEAD.
> >
> >
> > Alright, when I push the next version
Hi Amit, all
>
> 1.
> +# Testcase start: SUBSCRIPTION USES INDEX WITH PUB/SUB DIFFERENT DATA VIA
> +# A UNIQUE INDEX THAT IS NOT PRIMARY KEY OR REPLICA IDENTITY
>
> No need to use Delete test separate for this.
>
Yeah, there is really no difference between update/delete for this patch,
so it
On Sun, Mar 12, 2023 at 1:34 AM Önder Kalacı wrote:
>
>>
>> I think we can add such a test (which relies on existing buggy
>> behavior) later after fixing the existing bug. For now, it would be
>> better to remove that test and add it after we fix dropped columns
>> issue in HEAD.
>
>
> Alright,
Hi Amit, all
> > This triggers tuples_equal to fail. To fix that, I improved the
> tuples_equal
> > such that it skips the dropped columns.
> >
>
> By any chance, have you tried with generated columns?
Yes, it shows the same behavior.
> See
>
On Fri, Mar 10, 2023 at 7:58 PM Önder Kalacı wrote:
>
>
> I think one option could be to drop some cases altogether, but not sure we'd
> want that.
>
> As a semi-related question: Are you aware of any setting that'd make
> pg_stat_all_indexes
> reflect the changes sooner? It is hard to debug
On Fri, Mar 10, 2023 at 3:25 PM Önder Kalacı wrote:
>
>>
>
> Done with an important caveat. I think this reorganization of the test helped
> us to find one edge case regarding dropped columns.
>
> I realized that the dropped columns also get into the tuples_equal()
> function. And,
> the remote
Hi Amit, all
> I'll look for more opportunities and reply to the thread. I wanted to send
> this mail so that you can have a look at (1) earlier.
>
>
> I merged SUBSCRIPTION CREATE/DROP INDEX WORKS WITHOUT ISSUES
into SUBSCRIPTION CAN USE INDEXES WITH EXPRESSIONS AND COLUMNS.
Also, merged
On Fri, Mar 10, 2023 at 5:16 PM Önder Kalacı wrote:
>
>>
>> wip_for_optimize_index_column_match
>> +static bool
>> +IndexContainsAnyRemoteColumn(IndexInfo *indexInfo,
>> + LogicalRepRelation *remoterel)
>> +{
>> + for (int i = 0; i < indexInfo->ii_NumIndexAttrs; i++)
>> + {
>>
>> Wouldn't it be
Hi Amit, all
> wip_for_optimize_index_column_match
> +static bool
> +IndexContainsAnyRemoteColumn(IndexInfo *indexInfo,
> + LogicalRepRelation *remoterel)
> +{
> + for (int i = 0; i < indexInfo->ii_NumIndexAttrs; i++)
> + {
>
> Wouldn't it be better to just check if the first column is not
Hi Peter, all
> src/backend/replication/logical/relation.c
>
> 1. FindUsableIndexForReplicaIdentityFull
>
> +static Oid
> +FindUsableIndexForReplicaIdentityFull(Relation localrel)
> +{
> + List*indexlist = RelationGetIndexList(localrel);
> + Oid usableIndex = InvalidOid;
> + ListCell *lc;
On Thu, Mar 9, 2023 at 5:47 PM Önder Kalacı wrote:
>
> Amit Kapila , 8 Mar 2023 Çar, 14:42 tarihinde şunu
> yazdı:
>>
>> On Wed, Mar 8, 2023 at 4:51 PM Önder Kalacı wrote:
>> >
>> >
>> >>
>> >> I just share this case and then we
>> >> can discuss should we pick the index which only contain the
Here are some review comments for patch v39-0001 (mostly the test code).
==
src/backend/replication/logical/relation.c
1. FindUsableIndexForReplicaIdentityFull
+static Oid
+FindUsableIndexForReplicaIdentityFull(Relation localrel)
+{
+ List*indexlist = RelationGetIndexList(localrel);
+
Hi Amit,
> >
> >>
> >> 4.
> >> +# Testcase start: SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN
> >>
> >> As we have removed enable_indexscan check, you should remove this test.
> >
> >
> > Hmm, I think my rebase problems are causing confusion here, which v38
> fixes.
> >
>
> I think it is still
Hi,
Amit Kapila , 8 Mar 2023 Çar, 14:42 tarihinde şunu
yazdı:
> On Wed, Mar 8, 2023 at 4:51 PM Önder Kalacı wrote:
> >
> >
> >>
> >> I just share this case and then we
> >> can discuss should we pick the index which only contain the extra
> columns on the
> >> subscriber.
> >>
> >
> > I think
On Thu, Mar 9, 2023 at 4:50 PM Amit Kapila wrote:
>
> On Thu, Mar 9, 2023 at 3:26 PM Önder Kalacı wrote:
> >
> >
> > So, I changed the first one to SUBSCRIPTION USES INDEX WITH MULTIPLE ROWS
> > AND COLUMNS
> > and dropped the second one. Let me know if it does not make sense to you.
> > If I
On Thu, Mar 9, 2023 at 3:26 PM Önder Kalacı wrote:
>
>>
>> 4.
>> +# Testcase start: SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN
>>
>> As we have removed enable_indexscan check, you should remove this test.
>
>
> Hmm, I think my rebase problems are causing confusion here, which v38 fixes.
>
I
Hi Amit, all
> >
>
> This new test takes ~9s on my machine whereas most other tests in
> subscription/t take roughly 2-5s. I feel we should try to reduce the
> test timing without sacrificing much of the functionality or code
> coverage.
Alright, that is reasonable.
> I think if possible we
Hi Vignesh C,
> > Hmm, can you please elaborate more on this? The declaration
> > and assignment are already on different lines.
> >
> > ps: pgindent changed this line a bit. Does that look better?
>
> I thought of changing it to something like below:
> bool isUsableIndex;
> Oid idxoid =
Hi Peter,
> 1.
> In my previous review [1] (comment #1) I wrote that only some of the
> "should" were misleading and gave examples where to change. But I
> didn't say that *every* usage of that word was wrong, so your global
> replace of "should" to "must" has modified a couple of places in
>
On Thu, Mar 9, 2023 at 10:37 AM Peter Smith wrote:
>
> On Thu, Mar 9, 2023 at 3:49 PM Amit Kapila wrote:
> >
> > On Wed, Mar 8, 2023 at 8:44 PM Önder Kalacı wrote:
> > >
> > >>
> > >> I felt that once you remove the create publication/subscription/wait
> > >> for sync steps, the test execution
On Thu, Mar 9, 2023 at 3:49 PM Amit Kapila wrote:
>
> On Wed, Mar 8, 2023 at 8:44 PM Önder Kalacı wrote:
> >
> >>
> >> I felt that once you remove the create publication/subscription/wait
> >> for sync steps, the test execution might become faster and save some
> >> time in the local execution,
On Wed, Mar 8, 2023 at 8:44 PM Önder Kalacı wrote:
>
>>
>> I felt that once you remove the create publication/subscription/wait
>> for sync steps, the test execution might become faster and save some
>> time in the local execution, cfbot and the various machines in
>> buildfarm. If the execution
On Wed, 8 Mar 2023 at 21:46, Önder Kalacı wrote:
>
> Hi Vignesh C,
>
>>
>>
>> Few comments
>> 1) Maybe this change is not required:
>> fallback if no other solution is possible. If a replica identity other
>> than full is set on the publisher side, a replica identity
>> - comprising
On Thu, Mar 9, 2023 at 6:34 AM Peter Smith wrote:
>
> 4. build_replindex_scan_key
>
> >
> > Based on the discussions below, I kept as-is. I really don't want to do
> > unrelated
> > changes in this patch, as I also got several feedback for not doing it,
> >
>
> Hmm, although this code
Here are my review comments for v37-0001.
==
General - should/must.
1.
In my previous review [1] (comment #1) I wrote that only some of the
"should" were misleading and gave examples where to change. But I
didn't say that *every* usage of that word was wrong, so your global
replace of
Hi Vignesh C,
>
> Few comments
> 1) Maybe this change is not required:
> fallback if no other solution is possible. If a replica identity other
> than full is set on the publisher side, a replica
> identity
> - comprising the same or fewer columns must also be set on the subscriber
>
On Tue, 7 Mar 2023 at 19:17, Önder Kalacı wrote:
>
> Hi Amit, Peter
>
>>
>> > > Let me give an example to demonstrate why I thought something is fishy
>> > > here:
>> > >
>> > > Imagine rel has a (non-default) REPLICA IDENTITY with Oid=.
>> > > Imagine the same rel has a PRIMARY KEY with
On Fri, 3 Mar 2023 at 18:40, Önder Kalacı wrote:
>
> Hi Vignesh,
>
> Thanks for the review
>
>>
>> 1) We are currently calling RelationGetIndexList twice, once in
>> FindUsableIndexForReplicaIdentityFull function and in the caller too,
>> we could avoid one of the calls by passing the indexlist
On Wed, Mar 8, 2023 at 4:51 PM Önder Kalacı wrote:
>
>
>>
>> I just share this case and then we
>> can discuss should we pick the index which only contain the extra columns on
>> the
>> subscriber.
>>
>
> I think its performance implications come down to the discussion on [1].
> Overall, I
Hi Shi Yu, all
> e.g.
> -- pub
> CREATE TABLE test_replica_id_full (x int, y int);
> ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL;
> CREATE PUBLICATION tap_pub_rep_full FOR TABLE test_replica_id_full;
> -- sub
> CREATE TABLE test_replica_id_full (x int, y int, z int);
> CREATE INDEX
Hi Hou zj, all
>
> IIRC, it invokes tuples_equal for all cases unless we are using replica
> identity key or primary key to scan. But there seem some other cases where
> the
> tuples_equal looks unnecessary.
>
> For example, if the table on subscriber don't have a PK or RI key but have
> a
>
Hi Peter, all
>
> 1.
> Saying the index "should" or "should not" do this or that sounds like
> it is still OK but just not recommended. TO remove this ambigity IMO
> most of the "should" ought to be changed to "must" because IIUC this
> patch will simply not consider indexes which do not obey
Hi Amit, all
> You have not given complete steps to reproduce the problem where
> instead of the index scan, a sequential scan would be picked. I have
> tried to reproduce by extending your steps but didn't see the problem.
> Let me know if I am missing something.
>
I think the steps you shared
On Wednesday, March 8, 2023 2:51 PM houzj.f...@fujitsu.com
wrote:
>
> On Tuesday, March 7, 2023 9:47 PM Önder Kalacı
> wrote:
>
> Hi,
>
> > > > Let me give an example to demonstrate why I thought something is fishy
> here:
> > > >
> > > > Imagine rel has a (non-default) REPLICA IDENTITY with
On Tuesday, March 7, 2023 9:47 PM Önder Kalacı wrote:
Hi,
> > > Let me give an example to demonstrate why I thought something is fishy
> > > here:
> > >
> > > Imagine rel has a (non-default) REPLICA IDENTITY with Oid=.
> > > Imagine the same rel has a PRIMARY KEY with Oid=.
> > >
>
>
On Wed, Mar 8, 2023 at 3:03 PM Amit Kapila wrote:
>
> On Wed, Mar 8, 2023 at 9:09 AM Peter Smith wrote:
> >
> >
> > ==
> > src/backend/executor/execReplication.c
> >
> > 3. build_replindex_scan_key
> >
> > {
> > Oid operator;
> > Oid opfamily;
> > RegProcedure regop;
> > - int
On Wed, Mar 8, 2023 at 9:09 AM Peter Smith wrote:
>
>
> ==
> src/backend/executor/execReplication.c
>
> 3. build_replindex_scan_key
>
> {
> Oid operator;
> Oid opfamily;
> RegProcedure regop;
> - int pkattno = attoff + 1;
> - int mainattno = indkey->values[attoff];
> - Oid optype =
Here are some review comments for v35-0001
==
General
1.
Saying the index "should" or "should not" do this or that sounds like
it is still OK but just not recommended. TO remove this ambigity IMO
most of the "should" ought to be changed to "must" because IIUC this
patch will simply not
On Tue, Mar 7, 2023 at 7:17 PM Önder Kalacı wrote:
>
>>
>> > > Let me give an example to demonstrate why I thought something is fishy
>> > > here:
>> > >
>> > > Imagine rel has a (non-default) REPLICA IDENTITY with Oid=.
>> > > Imagine the same rel has a PRIMARY KEY with Oid=.
>> > >
>
>
On Tue, Mar 7, 2023 9:47 PM Önder Kalacı wrote:
>
> I'm attaching v35.
>
I noticed that if the index column only exists on the subscriber side, this
index
can also be chosen. This seems a bit odd because the index column isn't sent
from publisher.
e.g.
-- pub
CREATE TABLE
On Mon, Mar 6, 2023 at 7:40 PM Amit Kapila wrote:
>
> On Mon, Mar 6, 2023 at 1:40 PM Peter Smith wrote:
> >
...
> >
> > Anyhow, if you feel those firstterm and FULL changes ought to be kept
> > separate from this RI patch, please let me know and I will propose
> > those changes in a new thread,
Hi,
On 2023-03-07 08:22:45 +0530, Amit Kapila wrote:
> On Tue, Mar 7, 2023 at 1:34 AM Andres Freund wrote:
> > I think even as-is it's reasonable to just use it. The sequential scan
> > approach is O(N^2), which, uh, is not good. And having an index over
> > thousands
> > of non-differing
Hi Amit, Peter
> > > Let me give an example to demonstrate why I thought something is fishy
> here:
> > >
> > > Imagine rel has a (non-default) REPLICA IDENTITY with Oid=.
> > > Imagine the same rel has a PRIMARY KEY with Oid=.
> > >
>
Hmm, alright, this is syntactically possible, but
On Tue, Mar 7, 2023 at 3:00 PM Peter Smith wrote:
>
> On Tue, Mar 7, 2023 at 8:01 PM Amit Kapila wrote:
> >
> > On Tue, Mar 7, 2023 at 1:19 PM Peter Smith wrote:
> > >
> > > Let me give an example to demonstrate why I thought something is fishy
> > > here:
> > >
> > > Imagine rel has a
Hi Amit, all
>
> Few comments:
> =
> 1.
> +get_usable_indexoid(ApplyExecutionData *edata, ResultRelInfo *relinfo)
> {
> ...
> + if (targetrelkind == RELKIND_PARTITIONED_TABLE)
> + {
> + /* Target is a partitioned table, so find relmapentry of the partition */
> + TupleConversionMap
Hi Shi Yu, all
> Thanks for updating the patch. Here are some comments on v33-0001 patch.
>
> 1.
> + if (RelationReplicaIdentityFullIndexScanEnabled(localrel) &&
> + remoterel->replident == REPLICA_IDENTITY_FULL)
>
> RelationReplicaIdentityFullIndexScanEnabled() is introduced
Hi Andres, Amit, all
I think the case in which the patch regresses performance in is irrelevant
> in
> practice.
>
This is similar to what I think in this context.
I appreciate the effort from Shi Yu, so that we have a clear understanding
on the overhead.
But the tests we do on [1] where we
On Tue, Mar 7, 2023 at 8:01 PM Amit Kapila wrote:
>
> On Tue, Mar 7, 2023 at 1:19 PM Peter Smith wrote:
> >
> > Let me give an example to demonstrate why I thought something is fishy here:
> >
> > Imagine rel has a (non-default) REPLICA IDENTITY with Oid=.
> > Imagine the same rel has a
On Tue, Mar 7, 2023 at 1:19 PM Peter Smith wrote:
>
> Let me give an example to demonstrate why I thought something is fishy here:
>
> Imagine rel has a (non-default) REPLICA IDENTITY with Oid=.
> Imagine the same rel has a PRIMARY KEY with Oid=.
>
> ---
>
> +/*
> + * Get replica identity
On Mon, Mar 6, 2023 at 10:18 PM Önder Kalacı wrote:
>
>> 4. IdxIsRelationIdentityOrPK
>>
>> +/*
>> + * Given a relation and OID of an index, returns true if the
>> + * index is relation's replica identity index or relation's
>> + * primary key's index.
>> + *
>> + * Returns false otherwise.
>> +
On Tue, Mar 7, 2023 at 9:16 AM shiy.f...@fujitsu.com
wrote:
>
> On Monay, Mar 6, 2023 7:19 PM Önder Kalacı wrote:
> >
> > Yeah, seems easier to follow to me as well. Reflected it in the comment as
> > well.
> >
Few comments:
=
1.
+get_usable_indexoid(ApplyExecutionData *edata,
On Monay, Mar 6, 2023 7:19 PM Önder Kalacı wrote:
>
> Yeah, seems easier to follow to me as well. Reflected it in the comment as
> well.
>
Thanks for updating the patch. Here are some comments on v33-0001 patch.
1.
+ if (RelationReplicaIdentityFullIndexScanEnabled(localrel) &&
+
On Tue, Mar 7, 2023 at 1:34 AM Andres Freund wrote:
>
> On 2023-03-01 14:10:07 +0530, Amit Kapila wrote:
> > On Wed, Mar 1, 2023 at 12:09 AM Andres Freund wrote:
> > >
> > > > 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
Hi,
On 2023-03-06 17:02:38 +0530, Amit Kapila wrote:
> Andres, do you have any thoughts on this? We seem to have figured out
> the cause of regression in the case Shi-San has reported and others
> also agree with it. We can't think of doing anything better than what
> the patch currently is
Hi,
On 2023-03-01 14:10:07 +0530, Amit Kapila wrote:
> On Wed, Mar 1, 2023 at 12:09 AM Andres Freund wrote:
> >
> > > 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
On Mon, Mar 6, 2023 at 4:45 PM Amit Kapila wrote:
>
> On Mon, Mar 6, 2023 at 4:18 PM Dilip Kumar wrote:
> >
> > On Mon, Mar 6, 2023 at 2:38 PM Önder Kalacı wrote:
> > >
> > I was going through the thread and patch, I noticed that in the
> > initial version, we were depending upon the planner
On Thu, Mar 2, 2023 at 2:45 PM Amit Kapila wrote:
>
> On Thu, Mar 2, 2023 at 1:37 PM shiy.f...@fujitsu.com
> wrote:
> > - results of `gprof`
> > case1:
> > master
> > % cumulative self self total
> > time seconds secondscalls ms/call ms/call name
> > 1.37
Hi Amit, all
Amit Kapila , 6 Mar 2023 Pzt, 12:40 tarihinde şunu
yazdı:
> On Fri, Mar 3, 2023 at 6:40 PM Önder Kalacı wrote:
> >
> > Hi Vignesh,
> >
> > Thanks for the review
> >
> >>
> >> 1) We are currently calling RelationGetIndexList twice, once in
> >> FindUsableIndexForReplicaIdentityFull
Hi Peter, all
>
> 1.
> A published table must have a replica identity configured in order to
> be able to replicate UPDATE and DELETE operations, so that appropriate
> rows to update or delete can be identified on the subscriber side. By
> default, this is the primary key, if there is one.
Hi Amit, all
>
> >
> > I think you've "simplified" this function in v28 but AFAICT now it has
> > a different logic to v27.
> >
> > PREVIOUSLY it was coded like
> > + return RelationGetReplicaIndex(rel) == idxoid ||
> > + RelationGetPrimaryKeyIndex(rel) == idxoid;
> >
> > You can see if 'idxoid'
On Mon, Mar 6, 2023 at 4:18 PM Dilip Kumar wrote:
>
> On Mon, Mar 6, 2023 at 2:38 PM Önder Kalacı wrote:
> >
> I was going through the thread and patch, I noticed that in the
> initial version, we were depending upon the planner to let it decide
> whether index scan is cheaper or not and which
On Mon, Mar 6, 2023 at 2:38 PM Önder Kalacı wrote:
>
I was going through the thread and patch, I noticed that in the
initial version, we were depending upon the planner to let it decide
whether index scan is cheaper or not and which index to pick. But in
the latest patch if a useful index
On Fri, Mar 3, 2023 at 6:40 PM Önder Kalacı wrote:
>
> Hi Vignesh,
>
> Thanks for the review
>
>>
>> 1) We are currently calling RelationGetIndexList twice, once in
>> FindUsableIndexForReplicaIdentityFull function and in the caller too,
>> we could avoid one of the calls by passing the indexlist
Hi Amit, all
> >
> > Given Amit's suggestion on [1], I'm planning to drop this check
> altogether, and
> > rely on table storage parameters.
> >
>
> This still seems to be present in the latest version. I think we can
> just remove this and then add the additional check as suggested by you
> as
On Mon, Mar 6, 2023 at 1:40 PM Peter Smith wrote:
>
> On Mon, Mar 6, 2023 at 5:44 PM Amit Kapila wrote:
> >
> > On Mon, Mar 6, 2023 at 10:12 AM Peter Smith wrote:
> > >
> > > 4. IdxIsRelationIdentityOrPK
> > >
> > > +/*
> > > + * Given a relation and OID of an index, returns true if the
> > > +
On Mon, Mar 6, 2023 at 5:44 PM Amit Kapila wrote:
>
> On Mon, Mar 6, 2023 at 10:12 AM Peter Smith wrote:
> >
> > 4. IdxIsRelationIdentityOrPK
> >
> > +/*
> > + * Given a relation and OID of an index, returns true if the
> > + * index is relation's replica identity index or relation's
> > + *
On Mon, Mar 6, 2023 at 10:12 AM Peter Smith wrote:
>
> 4. IdxIsRelationIdentityOrPK
>
> +/*
> + * Given a relation and OID of an index, returns true if the
> + * index is relation's replica identity index or relation's
> + * primary key's index.
> + *
> + * Returns false otherwise.
> + */
> +bool
Here are some review comments for v28-0001.
==
doc/src/sgml/logical-replication.sgml
1.
A published table must have a replica identity configured in order to
be able to replicate UPDATE and DELETE operations, so that appropriate
rows to update or delete can be identified on the subscriber
On Fri, Mar 3, 2023 at 1:09 PM Önder Kalacı wrote:
>
>>
>> 5.
>>
>> + /*
>> +* If index scans are disabled, use a sequential scan.
>> +*
>> +* Note that we do not use index scans below when enable_indexscan is
>> +* false. Allowing primary key or replica
Hi Hayato, Amit, all
>
>
> ```
> + /* Build scankey for every non-expression attribute in the index.
> */
> ```
>
> I think that single line comments should not terminated by ".".
>
Hmm, looking into execReplication.c, many of the single line comments
terminated by ".". Also, On the
Hi Vignesh,
Thanks for the review
> 1) We are currently calling RelationGetIndexList twice, once in
> FindUsableIndexForReplicaIdentityFull function and in the caller too,
> we could avoid one of the calls by passing the indexlist to the
> function or removing the check here, index list check
On Fri, Mar 3, 2023 at 3:02 PM Önder Kalacı wrote:
>>
>> 7.
>> - /* Build scankey for every attribute in the index. */
>> - for (attoff = 0; attoff <
>> IndexRelationGetNumberOfKeyAttributes(idxrel); attoff++)
>> + /* Build scankey for every non-expression attribute in the index. */
>> + for
Hi Amit, all
> Few comments on 0001
>
> 1.
> + such suitable indexes, the search on the subscriber s ide can be
> very inefficient,
>
> unnecessary space in 'side'
>
Fixed in v28
>
> 2.
> - identity. If the table does not have any suitable key, then it can be
> set
>
Dear Önder,
Thanks for updating the patch!
I played with your patch and I confirmed that parallel apply worker and
tablesync worker
could pick the index in typical case.
Followings are comments for v27-0001. Please ignore if it is fixed in newer one.
execReplication.c
```
+ /* Build
Hi Peter, all
>
> ==
> Commit Message
>
> 1.
> There is no smart mechanism to pick the index. Instead, we choose
> the first index that fulfils the requirements mentioned above.
>
> ~
>
> 1a.
> I think this paragraph should immediately follow the earlier one
> ("With this patch...") which
Hi Hou zj, all
>
> 1.
> + usableIndexContext = AllocSetContextCreate(CurrentMemoryContext,
> +
> "usableIndexContext",
> +
> ALLOCSET_DEFAULT_SIZES);
> + oldctx = MemoryContextSwitchTo(usableIndexContext);
> +
> + /* Get index list of the local
On Thu, 2 Mar 2023 at 20:53, Önder Kalacı wrote:
>
> Hi,
>
>>
>> Is it just a matter of testing other
>> index types
>
>
> Yes, there are more to it. build_replindex_scan_key()
> only works for btree indexes, as it does BTEqualStrategyNumber.
>
> I might expect a few more limitations like that.
On Thu, Mar 2, 2023 at 8:52 PM Önder Kalacı wrote:
>
> Alright, attached v27_0001_use_index_on_subs_when_pub_rep_ident_full.patch and
> v27_0002_use_index_on_subs_when_pub_rep_ident_full.patch.
>
Few comments on 0001
1.
+ such suitable indexes, the search on the subscriber
Here are some review comments for v27-0001 (not the tests)
==
Commit Message
1.
There is no smart mechanism to pick the index. Instead, we choose
the first index that fulfils the requirements mentioned above.
~
1a.
I think this paragraph should immediately follow the earlier one
("With
On Thursday, March 2, 2023 11:23 PM Önder Kalacı wrote:
> Both the patches are numbered 0001. It would be better to number them
> as 0001 and 0002.
>
> Alright, attached v27_0001_use_index_on_subs_when_pub_rep_ident_full.patch
> and
> v27_0002_use_index_on_subs_when_pub_rep_ident_full.patch.
1 - 100 of 189 matches
Mail list logo
