On Thu, Jul 13, 2023 at 8:07 PM Önder Kalacı wrote:
>
> Looks good to me. I have also done some testing, which works as
> documented/expected.
>
Thanks, I have pushed this after minor changes in the comments.
--
With Regards,
Amit Kapila.
Hi Amit, Hayato, all
> +1 for the readability. I think we can as you suggest or I feel it
> > would be better if we return false as soon as we found any condition
> > is false. The current patch has a mixed style such that for
> > InvalidStrategy, it returns immediately but for others, it does a
>
On Thu, Jul 13, 2023 at 8:51 AM Amit Kapila wrote:
>
> On Wed, Jul 12, 2023 at 8:14 PM Önder Kalacı wrote:
> >
> >>
> >> > - return is_btree && !is_partial && !is_only_on_expression;
> >> > + /* Check whether the index is supported or not */
> >> > + is_suitable_type = ((get_equal_strategy_number
On Wed, Jul 12, 2023 at 8:14 PM Önder Kalacı wrote:
>
>>
>> > - 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));
>> > +
>> >
Here are some review comments for the v5 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. These 2 types of
ind
Hi Hayato, all
> > - 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
Dear Amit,
Thanks for checking my patch! The patch can be available at [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 s
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 thr
Here are my review comments for the patch v4.
==
General
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 ot
On Wed, Jul 12, 2023 at 8:37 AM Hayato Kuroda (Fujitsu)
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);
> >
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
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
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
On Mon, Jul 10, 2023 at 7:43 PM Önder Kalacı 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
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 GI
On Mon, Jun 26, 2023 at 7:14 PM Hayato Kuroda (Fujitsu)
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 RelationFindRep
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
On Fri, Jul 7, 2023 at 1:31 PM Hayato Kuroda (Fujitsu)
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
> limitati
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 ap
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
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 reprodu
> 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
test_point.sh
Description: test_point.sh
On Mon, Jun 26, 2023 at 11:44 PM Hayato Kuroda (Fujitsu)
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
On Thu, Jun 22, 2023 at 11:37 AM Hayato Kuroda (Fujitsu)
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 integra
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
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 in
Dear hackers,
(CC: Önder because he owned the related thread)
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
func
27 matches
Mail list logo
